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

fix: better caching of Rust deps #733

Merged
merged 5 commits into from
Sep 20, 2024
Merged

fix: better caching of Rust deps #733

merged 5 commits into from
Sep 20, 2024

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Sep 18, 2024

I chored up the CI file by making sure all jobs (minus the cargo-test job) use the same approach:

  • Running the whole job INSIDE the Parity-provided Docker container
  • Make sure the right cargo path as configured inside the container is used to cache cargo artifacts
  • Use the docker run approach only for the cargo-test job, since the worker still runs out of memory and crashes

Main changes

  • Use of container: component in all jobs but cargo-test
  • Use of the /usr/local/cargo path for caching cargo artifacts. This is the path configured by the container itself. Using other paths won't have the expected effect.
  • Addition of save-always: true for the caching of artifacts, since even if, for example a test fails, we still don't want to throw away the compilation work done to run that test
  • The try-runtime job had a cache configured by it was not caching anything, since we were caching the wrong path. That's fixed by using the same approach all other jobs use.

@ntn-x2 ntn-x2 self-assigned this Sep 18, 2024
@ntn-x2 ntn-x2 changed the title fix: caching of Rust deps in try-runtime job fix: better caching of Rust deps Sep 19, 2024
@ntn-x2 ntn-x2 marked this pull request as ready for review September 19, 2024 08:59
@ntn-x2 ntn-x2 requested review from ggera and Ad96el September 19, 2024 08:59
Copy link
Member

@Ad96el Ad96el left a comment

Choose a reason for hiding this comment

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

Love it

@@ -33,6 +33,13 @@ jobs:
cargo-clippy:
name: Run Clippy checks
runs-on: ubuntu-latest
container:
image: paritytech/ci-unified:bullseye-1.74.0
Copy link
Member

Choose a reason for hiding this comment

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

It's out of scope for this PR, but do you think it would be worth changing the image to an environment variable? We have multiple workflow files, and using an environment variable would be less error-prone.

Copy link
Member Author

Choose a reason for hiding this comment

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

This would need to be a repo-wide variable, which should be updated once we bump our dependencies to a new version, if we want to share it across workflows. I agree it's out of scope, but I cc @ggera to see if there's anything low effort we can do to minimize duplication.

@ntn-x2 ntn-x2 merged commit 9416693 into develop Sep 20, 2024
11 of 13 checks passed
@ntn-x2 ntn-x2 deleted the aa/ci-fixes branch September 20, 2024 08:29
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.

2 participants