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

Test getting started guide with ARM && AMD #293

Merged
merged 1 commit into from
Jun 24, 2024
Merged

Conversation

schneems
Copy link
Contributor

@schneems schneems commented Jun 14, 2024

Previously, all Heroku runners used AMD (x86) CPU architecture. We added the ability to build and run on an ARM machine (aarch64, like an M3 Mac), but building binaries for ARM required a beta GitHub Action (GHA) runner. The GHA runner beta required extra invocations to setup tools on the machine (such as installing docker).

The GHA ARM runners are now out of beta. This PR adds a test that exercises the ARM codepath on CI in addition to the existing AMD codepath.

GUS-W-15805220

@schneems schneems force-pushed the schneems/cnb-arm64 branch from 8c0f7d3 to ee1ed38 Compare June 14, 2024 15:54
@schneems schneems force-pushed the schneems/cnb-arm64 branch 3 times, most recently from 4cbafe3 to 2fe7f2a Compare June 14, 2024 17:25
Previously, all Heroku runners used AMD (x86) CPU architecture. We added the ability to build and run on an ARM machine (aarch64, like an M3 Mac), but building binaries for ARM required a beta GitHub Action (GHA) runner. The GHA runner beta required extra invocations to setup tools on the machine (such as installing docker). 

The GHA ARM runners are now out of beta. This PR adds a test that exercises the ARM codepath on CI in addition to the existing AMD codepath.
@schneems schneems force-pushed the schneems/cnb-arm64 branch from 2fe7f2a to fa59c7c Compare June 14, 2024 17:28
@schneems schneems marked this pull request as ready for review June 14, 2024 18:18
@schneems schneems requested a review from a team as a code owner June 14, 2024 18:18
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Thank you for looking into ARM64 testing!

Whilst this change does ensure that the buildpack build succeeds with an ARM64 platform (which confirms a few things, like: is the buildpack.toml [[targets]] metadata correct, and does the buildpack at least try and run on ARM vs failing with an error during the build), it doesn't actually confirm that the resultant app image is valid.

For example, if the Ruby CNB implementation forgot to check context.target during the build, and hardcoded an AMD64 path for some binary - the build could still succeed, but the app image then fail to run at runtime?

Instead, I think it might be best to add matrix testing to the integration test suite, rather than this printing build output step?

@schneems
Copy link
Contributor Author

For example, if the Ruby CNB implementation forgot to check context.target during the build, and hardcoded an AMD64 path for some binary - the build could still succeed, but the app image then fail to run at runtime?

The ruby binary is exercised several times in the buildpack on this app via commands. The commands gem install bundler, gem list and bundle exec rake assets:precompile all invoke the Ruby interpreter. They will fail if they're the wrong version or the wrong architecture as bundler ensures the Ruby version matches the Gemfile.lock.

I don't think we need the added complexity of adding an extra dimension to the matrix. Or rather, I'm wary of it.

@edmorley
Copy link
Member

Makes sense - happy to land this for now - though longer term I still think there is some benefit of testing a subset of the libcnb-test integration tests on multiple builder image/arch variants :-)

@schneems schneems enabled auto-merge (squash) June 24, 2024 20:01
@schneems schneems merged commit 230e8dd into main Jun 24, 2024
7 checks passed
@schneems schneems deleted the schneems/cnb-arm64 branch June 24, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants