fix: only skip mtime check on `~/.cargo/{git,registry}` #12369

jo1gi · 2023-07-17T21:50:57Z

Fixes #12090

Make cargo only skip mtime checks on $CARGO_HOME/{git, registry}.

rustbot · 2023-07-17T21:51:01Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

weihanglo

Thanks for the contribution! Could you add a test to verify that only registry and git path are ignored? Maybe by ensuring mtime check won't skip ~/.cargo/bin.

weihanglo · 2023-07-17T22:05:53Z

src/cargo/core/compiler/fingerprint/mod.rs

+            let is_readonly = ["git", "registry"]
+                .into_iter()
+                .map(|subfolder| cargo_home.join(subfolder))
+                .any(|readonly_dir| path.starts_with(readonly_dir));


Maybe just a micro optimization. This cargo_home.join is in a hot loop over paths. Could we do the allocation out of the loop upfront?

I am a bit wary that we now duplicate the path construction logic. Do you feel it better if we reuse the code here in Config?

I will take a look at it. Should I combine the commits, when I'm done?

My personal preference is making each commit atomic. Given the size of this change is not huge, it makes sense to me just rebase and leave one or two commits.

More personally, I may have the first commit to have a test first asserting the "bug behavior". Then the second contains the actual patch and the fixup of the test.

weihanglo · 2023-07-17T22:18:41Z

Pardon my title editing 🙇🏾.

tamird · 2023-07-17T22:51:26Z

src/cargo/core/compiler/fingerprint/mod.rs

-        if let Ok(true) = home::cargo_home().map(|home| path.starts_with(home)) {
-            continue;
+        // Assuming anything in cargo_home/{git, registry} is immutable
+        // (see also #9455 about marking cargo_home readonly) which avoids rebuilds when CI caches


FWIW: #9455 is not about marking CARGO_HOME readonly, but rather only a subdirectory of it.

weihanglo

Thanks for adding tests!

Could we move these two tests to tests/testsuite/freshness.rs. It makes more sense to put them there.

tests/testsuite/build.rs

weihanglo

We're almost there! Could you run cargo fmt --all to address the format error?

Closes rust-lang#12090 Commit Fix formatting Fix tests Fix formatting Fix formatting Fix formatting

weihanglo · 2023-07-19T11:16:10Z

Thanks @jo1gi

@bors r+

bors · 2023-07-19T11:16:12Z

📌 Commit 839069f has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-07-19T11:16:18Z

⌛ Testing commit 839069f with merge fc6cf8c...

bors · 2023-07-19T12:17:26Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing fc6cf8c to master...

bors · 2023-07-19T12:17:26Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing fc6cf8c to master...

bors · 2023-07-19T12:17:29Z

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

Update cargo 8 commits in 1b15556767f4b78a64e868eedf4073c423f02b93..7ac9416d82cd4fc5e707c9ec3574d22dff6466e5 2023-07-18 14:44:47 +0000 to 2023-07-24 14:29:38 +0000 - fix(cargo-credential): should enable feature `serde/derive` (rust-lang/cargo#12396) - fix: encode URL params correctly for SourceId in Cargo.lock (rust-lang/cargo#12280) - docs: format config override caveat as a note (rust-lang/cargo#12392) - credential provider implementation (rust-lang/cargo#12334) - feat(crates-io): expose HTTP headers and Error type (rust-lang/cargo#12310) - chore: Don't update test data (rust-lang/cargo#12380) - fix: only skip mtime check on `~/.cargo/{git,registry}` (rust-lang/cargo#12369) - Update docs for artifact JSON debuginfo levels. (rust-lang/cargo#12376) Since rust-lang/cargo#12334 makes built-in credential providers part of the cargo binary, it's no longer needed to build them in bootstrap.

rust-lang/cargo#12369 fixed this bug and was picked up in rust-lang/rust#114027.

rustbot assigned weihanglo Jul 17, 2023

rustbot added A-rebuild-detection Area: rebuild detection and fingerprinting S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 2023

weihanglo reviewed Jul 17, 2023

View reviewed changes

weihanglo changed the title ~~Make user made folders in CARGO_HOME writable~~ fix: only skip mtime check on ~/.cargo/{git,registry} Jul 17, 2023

tamird mentioned this pull request Jul 17, 2023

Improve CI caching by skipping mtime checks for paths in $CARGO_HOME #11613

Merged

tamird reviewed Jul 17, 2023

View reviewed changes

jo1gi force-pushed the readonly-cargo-home-subdirs branch 2 times, most recently from 97181c8 to ccccfda Compare July 18, 2023 17:24

jo1gi requested a review from weihanglo July 18, 2023 20:15

weihanglo reviewed Jul 18, 2023

View reviewed changes

tests/testsuite/build.rs Outdated Show resolved Hide resolved

tests/testsuite/build.rs Outdated Show resolved Hide resolved

tests/testsuite/build.rs Outdated Show resolved Hide resolved

tests/testsuite/build.rs Outdated Show resolved Hide resolved

jo1gi force-pushed the readonly-cargo-home-subdirs branch 3 times, most recently from 03fe631 to a4ecceb Compare July 18, 2023 21:54

weihanglo reviewed Jul 19, 2023

View reviewed changes

only skip mtime check on $CARGO_HOME/{git,registry}

839069f

Closes rust-lang#12090 Commit Fix formatting Fix tests Fix formatting Fix formatting Fix formatting

jo1gi force-pushed the readonly-cargo-home-subdirs branch from a4ecceb to 839069f Compare July 19, 2023 10:39

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 19, 2023

bors merged commit fc6cf8c into rust-lang:master Jul 19, 2023

arlosi mentioned this pull request Jul 24, 2023

Update cargo rust-lang/rust#114027

Merged

ehuss added this to the 1.73.0 milestone Jul 30, 2023

tamird mentioned this pull request Aug 22, 2023

integration-test: Remove cargo symlink workaround aya-rs/aya#762

Merged

tamird added a commit to aya-rs/aya that referenced this pull request Aug 22, 2023

integration-test: Remove cargo symlink workaround

08816d9

rust-lang/cargo#12369 fixed this bug and was picked up in rust-lang/rust#114027.

tamird added a commit to aya-rs/aya that referenced this pull request Aug 23, 2023

integration-test: Remove cargo symlink workaround

ca2e43d

rust-lang/cargo#12369 fixed this bug and was picked up in rust-lang/rust#114027.

weihanglo mentioned this pull request Oct 12, 2024

Documented CARGO_HOME cache paths may result in rebuilding git dependencies #8845

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: only skip mtime check on `~/.cargo/{git,registry}` #12369

fix: only skip mtime check on `~/.cargo/{git,registry}` #12369

jo1gi commented Jul 17, 2023 •

edited

Loading

rustbot commented Jul 17, 2023

weihanglo left a comment

weihanglo Jul 17, 2023 •

edited

Loading

jo1gi Jul 17, 2023

weihanglo Jul 17, 2023

weihanglo commented Jul 17, 2023

tamird Jul 17, 2023

weihanglo left a comment

weihanglo left a comment

weihanglo commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 19, 2023

fix: only skip mtime check on ~/.cargo/{git,registry} #12369

fix: only skip mtime check on ~/.cargo/{git,registry} #12369

Conversation

jo1gi commented Jul 17, 2023 • edited Loading

rustbot commented Jul 17, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

jo1gi Jul 17, 2023

Choose a reason for hiding this comment

weihanglo Jul 17, 2023

Choose a reason for hiding this comment

weihanglo commented Jul 17, 2023

tamird Jul 17, 2023

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 19, 2023

bors commented Jul 19, 2023

fix: only skip mtime check on `~/.cargo/{git,registry}` #12369

fix: only skip mtime check on `~/.cargo/{git,registry}` #12369

jo1gi commented Jul 17, 2023 •

edited

Loading

weihanglo Jul 17, 2023 •

edited

Loading