Don't re-use rustc cache when RUSTC_WRAPPER changes #9348

matklad · 2021-04-12T10:09:41Z

We cache initial rustc --version invocations, to speed up noop builds.

We check the mtime of rustc to bust the cache if the complier changed.
However, before this PR, we didn't look at mtimes of RUSTC_WRAPPER /
RUSTC_WORKSPACE_WRAPPER, so we could've re-use old cache with new
wrapper.

rust-highfive · 2021-04-12T10:09:44Z

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

matklad · 2021-04-12T10:12:02Z

tests/testsuite/rustc_info_cache.rs

+        // FIXME: the test currently fails with RUSTC_WORKSPACE_WRAPPER, because
+        // cargo doesn't rebuild the package at all. That is, something in the
+        // fingerprint calculation is broken as well?
+        //
+        // "RUSTC_WORKSPACE_WRAPPER",


I'll return to this later, but, I am wondering @ehuss if you know right of the bat if we indeed might not account for workspace_wrapper changes when deciding if we need to re-run the build?

It looks like the rustc-info cache uses process instead of workspace_process to determine the information. That means it will use RUSTC_WRAPPER but not RUSTC_WORKSPACE_WRAPPER.

I think it should be fine to change it to workspace_process. Cargo generally assumes that the wrapper will have the same behavior. That should bust the cache and trigger an error the same.

Yeah, that did the trick!

We cache initial `rustc --version` invocations, to speed up noop builds. We check the mtime of `rustc` to bust the cache if the complier changed. However, before this PR, we didn't look at mtimes of `RUSTC_WRAPPER` / `RUSTC_WORKSPACE_WRAPPER`, so we could've re-use old cache with new wrapper.

ehuss · 2021-04-17T21:17:56Z

I'm trying to better understand the motivation for this change. Can you explain more what comes up in practice?

If wrappers return different values for the output, that seems like it would cause trouble since cargo may not always use the same rustc for every invocation. For example, with RUSTC_WORKSPACE_WRAPPER, that will only be used for workspace members, and then regular rustc will be used for dependencies. Cargo assumes the different rustc executables will return the same version and capability information. If they don't, then it is likely it will get confused and have the wrong behavior.

I guess as long as RUSTC_WORKSPACE_WRAPPER returns the same output as the "other" rustc, it should be ok, but that seems somewhat brittle.

matklad · 2021-04-17T22:29:04Z

So the problem I am originally solving is solely about RUSTC_WRAPPER: I was unaware of WORKSPACE_WRAPPER before making this PR.

I was developing my own wrapper and the first version was buggy. Fixing that was hard, because cargo cached the buggy behavior, so all my bug fixes didn’t seemingly fix the problem. I thought the fixes were wrong, but it was that cargo just didn’t run my fixed code at all.

so hats why I went ahead and added rustc_wrapper to the cache (there’s already logic in place to track rustc binary mtime)

While at it, I’ve noticed that there’s workspace wrapper as well, so I decided to treat it in a similar way. I am not sure what’s the right behavior for ws wrapper is, and I donor know if that process ~> workspace process change in particular is a good idea.

Though, without that change, the test in this PR fails. The test does demonstrate a surprising behavior where the code doesn’t get rebuild when workspace wrapper is updated.

ehuss · 2021-04-18T17:13:16Z

OK, makes sense. There is some risk with changing this, but I think it should be fine. For example, every time one switches between clippy and a regular build will rebuild the rustc cache. I don't think that will be noticeable performance-wise, though.

@bors r+

bors · 2021-04-18T17:13:18Z

📌 Commit f6070b3 has been approved by ehuss

bors · 2021-04-18T17:24:27Z

⌛ Testing commit f6070b3 with merge 1243c37...

bors · 2021-04-18T17:51:27Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 1243c37 to master...

matklad · 2021-04-18T19:28:33Z

Agree about the risk! If you want to, I can flip process back and test *just* the RUSTC_WRAPPER. The process change is there solely to make the test pass, it’s not something required to fix my immediate problem

…

On Sunday, 18 April 2021, bors ***@***.***> wrote: Merged #9348 <#9348> into master. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9348 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M2IWGOEFSUWISNMZZDTJML23ANCNFSM42Y67HJA> .

ehuss · 2021-04-18T19:43:55Z

Nah. We can always change it later if it does seem like an issue. We could also change the cache to keep multiple entries (keyed off the fingerprint maybe) so that switching through different rustc versions will keep them all cached. But I don't see an immediate need for that.

Update cargo, rls ## cargo 18 commits in 65d57e6f384c2317f76626eac116f683e2b63665..0ed318d182e465cd66071b91ac3d265af63ef8a1 2021-04-04 15:07:52 +0000 to 2021-04-23 20:54:54 +0000 - Restore crates.io's `SourceId` hash value to before (rust-lang/cargo#9397) - Fix loading `branch=master` patches in the v3 lock transition (rust-lang/cargo#9392) - Update changelog for 1.52 beta changes. (rust-lang/cargo#9396) - Fix build-std updating the index on every build. (rust-lang/cargo#9393) - Fix typo in profile docs (rust-lang/cargo#9386) - Fix disagreement about lockfile ordering on stable/nightly (rust-lang/cargo#9384) - Don't give a hard error when the end-user specifies RUSTC_BOOTSTRAP=crate_name (rust-lang/cargo#9365) - Fix rust-lang/cargo#9350 (cargo build -Z help is missing options) (rust-lang/cargo#9369) - an struct -> a struct (rust-lang/cargo#9379) - Handle man pages better on Windows. (rust-lang/cargo#9378) - fix: better error message when dependency/workspace member missing (rust-lang/cargo#9368) - Fix typo in book (rust-lang/cargo#9376) - Don't re-use rustc cache when RUSTC_WRAPPER changes (rust-lang/cargo#9348) - doc: add split-debuginfo doc in config chapter (rust-lang/cargo#9372) - refactor: remove `CargoResultExt` (rust-lang/cargo#9367) - Track "CARGO" in environment fingerprint. (rust-lang/cargo#9363) - Update clippy lint allow set. (rust-lang/cargo#9356) - Fix 'suport' typo in documentation (rust-lang/cargo#9338) ## rls 3 commits in 32c0fe006dcdc13e1ca0ca31de543e4436c1299e..74d1800c25498689c5b5120a1e8495fce0cd0d0d 2021-04-12 11:21:12 +0000 to 2021-04-22 21:29:51 +0000 - Bump default integration test message timeout to 30s (rust-lang/rls#1731) - itertools = 0.9, fst = 0.4 (rust-lang/rls#1729) - Update cargo (rust-lang/rls#1728)

pickfire · 2021-04-27T11:38:47Z

src/cargo/util/rustc.rs

+    }
+    if let Some(workspace_wrapper) = workspace_wrapper {
+        hash_exe(&mut hasher, workspace_wrapper)?;
+    }


Could there be an else case?

It maybe could, but that would be worse, imo. The current approach of always hashing both is robust to future changes in the surrounding code.

rust-highfive assigned ehuss Apr 12, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 12, 2021

matklad commented Apr 12, 2021

View reviewed changes

matklad mentioned this pull request Apr 12, 2021

feat: avoid checking the whole project during initial loading rust-lang/rust-analyzer#8476

Merged

matklad force-pushed the wrapper-fingerprint branch from 3ddd781 to f6070b3 Compare April 16, 2021 17:08

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 Apr 18, 2021

bors merged commit 1243c37 into rust-lang:master Apr 18, 2021

ehuss mentioned this pull request Apr 21, 2021

Update cargo, rls rust-lang/rust#84412

Merged

pickfire reviewed Apr 27, 2021

View reviewed changes

ehuss added this to the 1.53.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't re-use rustc cache when RUSTC_WRAPPER changes #9348

Don't re-use rustc cache when RUSTC_WRAPPER changes #9348

matklad commented Apr 12, 2021

rust-highfive commented Apr 12, 2021

matklad Apr 12, 2021

ehuss Apr 13, 2021

matklad Apr 16, 2021

ehuss commented Apr 17, 2021

matklad commented Apr 17, 2021

ehuss commented Apr 18, 2021

bors commented Apr 18, 2021

bors commented Apr 18, 2021

bors commented Apr 18, 2021

matklad commented Apr 18, 2021 via email

ehuss commented Apr 18, 2021

pickfire Apr 27, 2021

matklad Apr 27, 2021

Don't re-use rustc cache when RUSTC_WRAPPER changes #9348

Don't re-use rustc cache when RUSTC_WRAPPER changes #9348

Conversation

matklad commented Apr 12, 2021

rust-highfive commented Apr 12, 2021

matklad Apr 12, 2021

Choose a reason for hiding this comment

ehuss Apr 13, 2021

Choose a reason for hiding this comment

matklad Apr 16, 2021

Choose a reason for hiding this comment

ehuss commented Apr 17, 2021

matklad commented Apr 17, 2021

ehuss commented Apr 18, 2021

bors commented Apr 18, 2021

bors commented Apr 18, 2021

bors commented Apr 18, 2021

matklad commented Apr 18, 2021 via email

ehuss commented Apr 18, 2021

pickfire Apr 27, 2021

Choose a reason for hiding this comment

matklad Apr 27, 2021

Choose a reason for hiding this comment