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

[CI] Make Ubuntu 20.04 and 22.04 use different rust cache #197

Merged
merged 1 commit into from
Jul 22, 2023

Conversation

jprendes
Copy link
Collaborator

Currently Ubuntu 20.04 and 22.04 are sharing the same cache.
This is a problem as 20.04 and 22.04 ship differente glibc versions.
This prevents the buil scripts compiled and cached in 22.04 from running in 20.04.

actions-rust-lang/setup-rust-toolchain@v1 uses Swatinem/rust-cache@v2, which uses all environment variables matching RUST* as hash for the cache key. See the description of env-vars here.

This PR adds an environment variable RUST_CACHE_KEY_OS so that different OS use different caches.

This fixes #193.

@jprendes
Copy link
Collaborator Author

@Mossaka PTAL 🙂

Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

Wow nice catch! I was wondering why a cache messed up the glibc library on the build and this is exactly why!! 🤯

Just made one comment on the use of env

@@ -32,6 +32,8 @@ jobs:
run: "mount | grep cgroup"
- uses: actions/checkout@v3
- uses: actions-rust-lang/setup-rust-toolchain@v1
env:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be env-vars according to this doc

@Mossaka
Copy link
Member

Mossaka commented Jul 22, 2023

I am confused how previous builds didn't hit this problem before #189 removed the cache as we weren't using any env vars to prefix the cache key to a verison of ubuntu before.

@Mossaka
Copy link
Member

Mossaka commented Jul 22, 2023

It looks like it's still using the old cache key at this run

@jprendes
Copy link
Collaborator Author

IIUC, by default Swatinem/rust-cache does use the os as part of the hash, but actions-rust-lang/setup-rust-toolchain specifies a key that doesn't. But I may be misinterpreting the doc.

This was not a problem for #187 because it was bumping the crate with a build script, so it wasn't hitting the cache. #189 did hit the issue.

@jprendes
Copy link
Collaborator Author

It looks like it's still using the old cache key at this run

The job is part of the cache key, so the fmt job doesn't share the cache with the builds. I didn't add any env-var there as it doesn't run in multiple OSes.

@Mossaka
Copy link
Member

Mossaka commented Jul 22, 2023

I don't understand how setup-rust-toolchain specifies a key that doesn't use the os as part of the hash... Could you show me some clues? And I can't find where in the doc says that "RUST_CACHE_KEY_OS" could make it that differnt OS uses different cache.

I checked the action source code of setup-rust-toolchain and found that it just uses rust-cache https://github.com/actions-rust-lang/setup-rust-toolchain/blob/main/action.yml#L159-L161

@jprendes
Copy link
Collaborator Author

I'm not sure how setup-rust-toolchain sets the cache key for rust-cache. I believe that's done here, and uses just rustc's commit and build date. But that way of setting the key is not documented in rust-cache as far as I can tell.

setup-rust-toolchain doesn't provide options to configure rust-cache other than an on/off switch. But it does leaves most options with their default. In particular, the env-vars option is a list of env-var prefixes that will be used for the hash. One of those prefixes is RUST and that's how RUST_CACHE_KEY_OS ends up being used for the hash.

Maybe another cleaner approach would be to disable cache in setup-rust-toolchain and configure rust-cache manually like you did in release.yml

@Mossaka
Copy link
Member

Mossaka commented Jul 22, 2023

Thanks for the explaination! That makes a lot of sense.

The env-vars are matched by prefix, so the default RUST var will
match all of RUSTC, RUSTUP_*, RUSTFLAGS, RUSTDOC_*, etc.

This is the comment that explains it, which I missed! Thanks again.

I can merge it right now if you want

@Mossaka Mossaka merged commit 17fea9e into containerd:main Jul 22, 2023
@Mossaka Mossaka mentioned this pull request Jul 22, 2023
@jprendes jprendes deleted the fix-cache branch July 23, 2023 05:26
@jsturtevant
Copy link
Contributor

@jprendes good catch and thanks for helping track that one down 💯

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.

CI might be broken after #187
3 participants