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

feat: enable CI to build base image for arm64 and amd64 #96

Merged
merged 7 commits into from
Mar 18, 2024

Conversation

guillaumeloftorbital
Copy link
Contributor

@guillaumeloftorbital guillaumeloftorbital commented Feb 22, 2024

Description

  • Enable ci to build dev image
  • Enable arm64 and and64 image
  • Update CI to build package for arm64
  • slip the image build in 3 step to be able to build with the 6h job limitation

Testing

@guillaumeloftorbital guillaumeloftorbital force-pushed the users/guillaume/multi-arch branch 6 times, most recently from d1cf1b0 to caa3ace Compare February 22, 2024 20:30

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@guillaumeloftorbital guillaumeloftorbital changed the title feat:build for arm64 feat: enable CI to build base image for arm64 and amd64 Feb 24, 2024
Copy link
Contributor

@apaletta3 apaletta3 left a comment

Choose a reason for hiding this comment

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

Couple nits, but otherwise LGTM! Thanks for this

Copy link
Contributor

@apaletta3 apaletta3 left a comment

Choose a reason for hiding this comment

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

Can you matrix the branch as well so that we only run the arm package building for both cpp and python on main instead of on every pull request? Because the arm package building lengthens the pipeline, and we just put effort into reducing its length recently

Co-authored-by: Antoine Paletta <98616558+apaletta3@users.noreply.github.com>
Copy link
Contributor

@vishwa2710 vishwa2710 left a comment

Choose a reason for hiding this comment

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

Just 1 Q. Also you might need to increase the timeout as building the docker image is taking > 6 hours O_O

@Derollez
Copy link
Contributor

Derollez commented Mar 6, 2024

@guillaumeloftorbital How long does it take using the cache in the CI? Also were you able to confirm that it is able to use the cache from one build to another? 🤔 Looks great man, thanks for the awesome addition

@guillaumeloftorbital
Copy link
Contributor Author

@Derollez the CI takes ~6h but with cache is faster this job only take 1min for example https://github.com/open-space-collective/open-space-toolkit/actions/runs/8038419090

@guillaumeloftorbital
Copy link
Contributor Author

e branch as well so that we only run the arm package building for both cpp and python on main instead of on every pull request? Because the arm package building lengthens the pipeline, and we just put effort into reducing its length recently

my issue with disabling the build for arm64 is that it could be easy to break the build but we will only see on release

@apaletta3
Copy link
Contributor

e branch as well so that we only run the arm package building for both cpp and python on main instead of on every pull request? Because the arm package building lengthens the pipeline, and we just put effort into reducing its length recently

my issue with disabling the build for arm64 is that it could be easy to break the build but we will only see on release

@guillaumeloftorbital No, I don't mean only on release, I mean only on main and also on release. So if it breaks on main after we merge we'll see it right away and not have to wait for a release. We already do this in ostk-astro with validation tests (kind of like integration tests) that only run on main to keep the length of the CI down.

So could you make this change please?

@guillaumeloftorbital guillaumeloftorbital force-pushed the users/guillaume/multi-arch branch from 2011d87 to 5501a1e Compare March 12, 2024 09:23
@apaletta3 apaletta3 self-requested a review March 13, 2024 14:13
@Derollez Derollez requested a review from lucas-bremond March 14, 2024 09:34
@guillaumeloftorbital guillaumeloftorbital enabled auto-merge (squash) March 18, 2024 15:49
Copy link
Contributor

@apaletta3 apaletta3 left a comment

Choose a reason for hiding this comment

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

LGTM

@guillaumeloftorbital guillaumeloftorbital merged commit 2ae5a2d into main Mar 18, 2024
2 checks passed
@guillaumeloftorbital guillaumeloftorbital deleted the users/guillaume/multi-arch branch March 18, 2024 15:51
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.

4 participants