Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for ARM builds #278

Merged
merged 21 commits into from
Jun 16, 2024
Merged

Add support for ARM builds #278

merged 21 commits into from
Jun 16, 2024

Conversation

favrik
Copy link
Contributor

@favrik favrik commented May 31, 2024

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.

@favrik favrik marked this pull request as ready for review May 31, 2024 00:43
src/setup-beam.js Outdated Show resolved Hide resolved
@paulo-ferraz-oliveira
Copy link
Collaborator

paulo-ferraz-oliveira commented May 31, 2024

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.

Copy link
Collaborator

@paulo-ferraz-oliveira paulo-ferraz-oliveira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have full knowledge of how Bob's generating the builds (or where) so trust @ericmj's judgement. My approval is for the overall structure of the rest of change, even though I understand we may not have GitHub-hosted runners in ARM.

@paulo-ferraz-oliveira paulo-ferraz-oliveira linked an issue May 31, 2024 that may be closed by this pull request
@paulo-ferraz-oliveira
Copy link
Collaborator

paulo-ferraz-oliveira commented May 31, 2024

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
Copy link
Contributor Author

favrik commented Jun 1, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@favrik
Copy link
Contributor Author

favrik commented Jun 16, 2024

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

spec = '26'
osVersion = 'ubuntu-24.04'
expected = 'maint-26'
got = await setupBeam.getOTPVersion(spec, osVersion, hexMirrors)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this hexMirrors is wrong everywhere. Would you be so kind and remove it? (at least, from your changes, but also from all the other tests if it's Ok with you) Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge as-is and do the fix, no worries.

@paulo-ferraz-oliveira
Copy link
Collaborator

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

@paulo-ferraz-oliveira paulo-ferraz-oliveira merged commit 6ab5719 into erlef:main Jun 16, 2024
65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ARM builds
3 participants