erlef / setup-beam

Set up your BEAM-based GitHub Actions workflow (Erlang, Elixir, Gleam, ...)

MIT License

373 stars 50 forks source link

Add support for ARM builds #278

Closed favrik closed 2 months ago

favrik commented 3 months ago

Description

Support new OTP builds for ARM64: https://github.com/hexpm/bob/pull/174/files. Ubuntu 16.04 and 18.04 do not appear to have ARM64 builds.

[x] I have performed a self-review of my changes
[x] I have read and understood the contributing guidelines

paulo-ferraz-oliveira commented 3 months ago

Good stuff 👍.

~Could you add some GHA tests that show this running in Linux ARM, 🙏? Updateable file is ubunty.yml, in folder .github/workflows.~ Apparently not possible, so we'll have to trust usage report on self-hosted runners.

paulo-ferraz-oliveira commented 3 months ago

I've approved the test run, but there's a failing test: https://github.com/erlef/setup-beam/actions/runs/9316480861/job/25657891440?pr=278#step:6:55. It seems unrelated to the proposed changes (and it passes in the main branch: I just re-executed it there).

@favrik, could you rebase your branch over main to see if anything changes? Thanks.

favrik commented 3 months ago

Thanks for the reviews!! I'm testing on an ARM64 self-hosted runner. There is an issue with the current version of the code, but already solved, will update PR and tests soon.

The issue: the OS Architecture information is needed in different functions: install, installOTP, and getOTPVersions. Do you think it's ok to just use getRunnerOSArchitecture in all of them, or set a variable in main and pass it around? 🤔

paulo-ferraz-oliveira commented 3 months ago

Do you think it's ok to just use getRunnerOSArchitecture in all of them, or set a variable in main and pass it around? 🤔

I'd say "whatever means less code". From a functional perspective, though, and since this is not a time-critical path, I'd probably go with using the function. (though I haven't tried the code, so don't quite know how it'd look)

Btw, tests are failing, but I'm not sure those are the ones you're referring to 😄

paulo-ferraz-oliveira commented 2 months ago

Re-ran the tests just now. They're still failing. I didn't look into the "why".

favrik commented 2 months ago

Updated: verified tests run without errors on Ubuntu, and added updated logic to actually make it work in self-hosted runners with ARM64 arch.

paulo-ferraz-oliveira commented 2 months ago

Thanks, @favrik. I'm merging this. Should get released soon after two minor pull requests are accepted/merged.