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

Use a standard Cargo caching action #17169

Merged
merged 1 commit into from
Oct 13, 2022
Merged

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Oct 9, 2022

It will automatically use correct keys and take care of trimming the cache entries.

However, we first had to patch that action to not wipe ~/.cargo/bin, see
Swatinem/rust-cache@master...benjyw:rust-cache:master

I will send that patch, or similar, upstream for consideration, so that in the future we can
use the standard action instead of the forked one.

@benjyw benjyw added the category:internal CI, fixes for not-yet-released features, etc. label Oct 9, 2022
@benjyw
Copy link
Contributor Author

benjyw commented Oct 9, 2022

I cargo-culted this from Toolchain's internal repo, so let me know if this is plain wrong.

@benjyw benjyw force-pushed the trim_cargo branch 2 times, most recently from 985c0c6 to ae5e5af Compare October 9, 2022 22:13
@@ -227,28 +227,6 @@ def install_rustup() -> Step:
}


def rust_caches() -> Sequence[Step]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was unused, a relic from before we moved it under Helper.

@benjyw
Copy link
Contributor Author

benjyw commented Oct 10, 2022

So I don't think this is right - that just installs the cargo-trim package, but doesn't invoke cargo trim --query or whatever. And that install takes long enough that it might not be worth it?

@benjyw
Copy link
Contributor Author

benjyw commented Oct 10, 2022

Yeah, I don't think this is the right way to go about this at all. Will retool.

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

@benjyw
Copy link
Contributor Author

benjyw commented Oct 11, 2022

Is it worth migrating to an existing action for this? Maybe https://github.com/marketplace/actions/rust-cache or https://github.com/marketplace/actions/rust-cargo ?

Probably? I'll give it a shot

@benjyw benjyw force-pushed the trim_cargo branch 2 times, most recently from 3e2f42d to 6f8457b Compare October 12, 2022 04:46
@benjyw benjyw changed the title Trim the cached Cargo state. Use a standard Cargo caching action Oct 12, 2022
@benjyw benjyw force-pushed the trim_cargo branch 4 times, most recently from 922c006 to a23fba6 Compare October 13, 2022 06:43
@benjyw benjyw marked this pull request as ready for review October 13, 2022 14:36
@benjyw
Copy link
Contributor Author

benjyw commented Oct 13, 2022

OK, this is now ready for re-review, thanks!

@benjyw
Copy link
Contributor Author

benjyw commented Oct 13, 2022

PS I have verified that it did not nuke rustup from the self-hosted runners...

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

},
},
{
"name": "Cache Cargo",
"uses": "actions/cache@v3",
"uses": "benjyw/rust-cache@461b9f8eee66b575bce78977bf649b8b7a8d53f1",
Copy link
Member

Choose a reason for hiding this comment

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

Should probably fork this to the pantsbuild org and tag it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My hope is to upstream it, but that is the fallback.

@benjyw benjyw merged commit c2166c5 into pantsbuild:main Oct 13, 2022
@benjyw benjyw deleted the trim_cargo branch October 13, 2022 16:31
benjyw added a commit to benjyw/pants that referenced this pull request Oct 13, 2022
It will automatically use correct keys and take care of trimming the cache entries.

However, we first had to patch that action to not wipe ~/.cargo/bin, see
Swatinem/rust-cache@master...benjyw:rust-cache:master

I will send that patch, or similar, upstream for consideration, so that in the future we can
use the standard action instead of the forked one.
benjyw added a commit that referenced this pull request Oct 14, 2022
Cherrypicks of #17218, #17169, #17194, #17172, #16806, #16796. 

Since this branch is still being released from, we want it to benefit from recent and upcoming CI improvements.

Cherry-picking them all in one PR saves on CI resources and time. And since no Pants source code is modified, only CI config, it seems fine to not cherry-pick separately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants