Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Always install Rust and use Leafwing-Studios/cargo-cache #1405

Closed
wants to merge 7 commits into from
Closed

Always install Rust and use Leafwing-Studios/cargo-cache #1405

wants to merge 7 commits into from

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Jun 14, 2024

This PR has two parts:

  1. Some jobs don't actually install Rust before using it. This doesn't error because Github's hosted runners come pre-installed with Rust, though it is frequently outdated.
  2. Use Leafwing-Studios/cargo-cache for all jobs that use Rust. This action is wonderful: I've been using it successfully for a month or so now. (Also it's by our friends at Leafwing, so it can be trusted!) This should speed up many jobs without the usual tradeoffs of caching.

@BD103 BD103 requested a review from mockersf June 14, 2024 16:17
@BD103 BD103 added C-Bug A problem with the code that runs the site C-Feature A new feature, making something new possible A-Build-System S-Needs-Review X-Contentious There are nontrivial implications that should be thought through labels Jun 14, 2024
@BD103
Copy link
Member Author

BD103 commented Jun 14, 2024

I'm marking this as contentious because it is pulling in an outside dependency, but I feel like it's from a trusted source. If this gets major pushback, we can fork Leafwing-Studios/cargo-cache under the Bevy organization and frequently update it.

@BD103 BD103 force-pushed the rust-and-caching branch from 56940d5 to b44b291 Compare June 14, 2024 16:20
@BD103
Copy link
Member Author

BD103 commented Jun 14, 2024

I'm rebasing this on top of #1403 so CI passes. Consider this blocked until that PR is merged.

@rparrett
Copy link
Contributor

Just linking in discussion from the twin PR for the bevy repo. bevyengine/bevy#13040

@alice-i-cecile
Copy link
Member

Yeah no objection to handing off ownership to the Bevy org if this appeals to y'all :)

@BD103 BD103 removed the S-Blocked label Jun 14, 2024
BD103 added 2 commits June 14, 2024 18:19
@BD103
fix: install rust before using it
0e4e22d 
Github hosted runners come preinstalled with Rust[^0], though it is often outdated. We should prefer to manually install it, since it gives us control over the version being used.

[^0]: https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2204-Readme.md#rust-tools
@BD103
feat: use Leafwing-Studios/cargo-cache for caching
edf4544
@BD103 BD103 force-pushed the rust-and-caching branch from b44b291 to edf4544 Compare June 14, 2024 22:19
@janhohenheim janhohenheim added S-Ready-For-Final-Review Ready for a maintainer to consider for merging and removed S-Needs-Review labels Jul 3, 2024
@BD103
Copy link
Member Author

BD103 commented Jul 16, 2024

@mockersf what are your thoughts on merging this? It would provide a good testing ground for introducing this action to the main Bevy repository, and Alice even offered transferring the repository.

@janhohenheim
Copy link
Member

Note that cargo-cache is about to release v2: Leafwing-Studios/cargo-cache#17

janhohenheim
janhohenheim reviewed Jul 16, 2024
View reviewed changes
BD103
BD103 commented Jul 16, 2024
View reviewed changes
@BD103 @janhohenheim
feat: bump to cargo-cache@v2
a150712 
Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR and removed X-Contentious There are nontrivial implications that should be thought through labels Aug 17, 2024
@BD103
Copy link
Member Author

BD103 commented Sep 10, 2024

Let me enable the sweep-cache option before this gets merged :)

@BD103 BD103 removed the S-Ready-For-Final-Review Ready for a maintainer to consider for merging label Sep 10, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review Ready for a maintainer to consider for merging and removed S-Waiting-On-Author labels Sep 10, 2024
@BD103
Copy link
Member Author

BD103 commented Sep 10, 2024

Hah! Turns out this found a bug with cargo-sweep: BD103/cargo-sweep#7. Let me fix that real quick...

@BD103 BD103 marked this pull request as draft September 11, 2024 02:51
@BD103 BD103 added S-Waiting-On-Author and removed S-Ready-For-Final-Review Ready for a maintainer to consider for merging labels Sep 11, 2024
@BD103
Copy link
Member Author

BD103 commented Oct 8, 2024

I'm going to close this. I no longer have the time / energy to work on it, though anyone else is free to pick it up!

I'll create an issue for this, though, so it's not lost.

@BD103 BD103 closed this Oct 8, 2024
@BD103 BD103 deleted the rust-and-caching branch October 8, 2024 00:22
@BD103 BD103 mentioned this pull request Oct 8, 2024
Open
@TrialDragon TrialDragon added S-Adopt-Me This PR could use some love: make a new PR with this work! and removed S-Waiting-On-Author labels Oct 8, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@janhohenheim janhohenheim janhohenheim approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@TimJentzsch TimJentzsch TimJentzsch approved these changes

@TrialDragon TrialDragon TrialDragon approved these changes

@mockersf mockersf Awaiting requested review from mockersf

Assignees
No one assigned
Labels
A-Build-System C-Bug A problem with the code that runs the site C-Feature A new feature, making something new possible S-Adopt-Me This PR could use some love: make a new PR with this work! X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@BD103 @rparrett @alice-i-cecile @janhohenheim @TimJentzsch @TrialDragon