Skip to content

Conversation

@LtdSauce
Copy link
Contributor

@LtdSauce LtdSauce commented Oct 11, 2024

Description

Enable the arm64 image creation again in the docker workflow.

Motivation and Context

The arm64 image builds were previously disabled because they ran into timeouts.
The cause seemed to be, that cargo-chef accidentally build the dependencies twice. And the second build managed to hit the timeout. This solves this, by no longer using the toolchain specified in rust-toolchain.toml but instead relying on the already installed rust version from the image.

This speeds up the build twice:

  • No longer building the dependencies twice
  • No longer installing the nightly rust toolchain twice

This should fix #879 and make #888 no longer needed.

How Has This Been Tested?

I have tested this on my fork by changing the workflow in a way that it only builds but does not push.
In this commit i removed all unnecessary workflows and made the docker workflow not try to push. The result can be seen here

Screenshots / Logs (if applicable)

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (no code change)
  • Refactor (refactoring production code)
  • Other

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have formatted the code with rustfmt.
  • I checked the lints with clippy.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@LtdSauce LtdSauce requested a review from orhun as a code owner October 11, 2024 20:02
@welcome
Copy link

welcome bot commented Oct 11, 2024

Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️

@LtdSauce
Copy link
Contributor Author

LtdSauce commented Oct 11, 2024

Do i have to check all boxes in the checklist even if i have the feeling they do not apply here?
Also i hope i did the commit messages correct, if not please advise me how to change them :)

And i have the feeling the build errors regarding linting and NodeJS are not related to my change? 🤔 I will check if ignoring the toolchain has an effect on the other CI builds.

Also I explain why I think ignoring the toolchain instead of waiting until it is fixed in cargo-chef or working around the issue with cargo-chef not using the toolchain in the commit message. Hopefully that is the correct place.

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2024

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 41.74%. Comparing base (b604883) to head (cdbada7).
⚠️ Report is 154 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #919   +/-   ##
=======================================
  Coverage   41.74%   41.74%           
=======================================
  Files          21       21           
  Lines        1670     1670           
=======================================
  Hits          697      697           
  Misses        973      973           
Flag Coverage Δ
unit-tests 41.74% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@LtdSauce
Copy link
Contributor Author

Nope that seems like it is comming from somewhere else.

@LtdSauce LtdSauce changed the title Build arm64 images again feat(docker): build arm64 images again Oct 11, 2024
This commit adds the known names of the rust-toolchain files to the
.dockerignore file. This has two reasons why it makes sense:

- The initial docker layer already has a set up rust toolchain that is
  sufficient to build the project. Thus, by providing a toolchain file,
  the toolchain would be installed again during docker build.
- Currently cargo-chef only copies the toolchain files during cooking
  but it gets not used during the building of the dependencies in the
  cook call, see
  LukeMathWalker/cargo-chef#271.
  With this in mind, currently the dependencies were actually build
  twice. Once with the installed toolchain from the image itself, and
  then in the actual cargo build call with the toolchain speciefied in
  the toolchain file. Building them twice resulted in timeouts when
  building the arm64 images as they are emulated using qemu, which is
  itself already slower than building natively.

Now one could argue, that as soon as the mentioned issue is solved using
the toolchain again would be fine. But then it would be still needed to
assemble the Dockerfile in a way that the toolchain is not build twice.
Because the current structure of the Dockerfile builds the toolchain
once in the cargo-chef prepare step and once during the cargo build step
(and would later build it during the cargo-chef cook instead of cargo
build).

With all this in mind using no toolchain file but instead just using
the sufficient rust installation from the base image makes sense.
…ly (orhun#879)"

This reverts commit cde2a8e.
Commit 73f75d5 made it possible to
build the arm64 image again without running into timeouts.
@LtdSauce LtdSauce force-pushed the build-arm64-images-again branch from eac0881 to cdbada7 Compare October 14, 2024 17:10
@LtdSauce
Copy link
Contributor Author

As i have seen you pushed commits fixing the clippy lints and the NodeJS checks i rebased my branch on the current main branch. Now all checks should pass :)

@LtdSauce LtdSauce changed the title feat(docker): build arm64 images again feat(docker): build arm64 images again (#879) Oct 14, 2024
Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

🚀🚀🚀

@orhun orhun merged commit 84771f6 into orhun:main Oct 17, 2024
@welcome
Copy link

welcome bot commented Oct 17, 2024

Congrats on merging your first pull request! ⛰️

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.

ARM64 docker images take 6h+ to build

3 participants