Open Alexendoo opened 1 month ago
Note that cargo
has no knowledge of CLIPPY_ARGS
, one way or the other, to be able to fingerprint it.
Would not caching failed invocations in .rustc_info.json
be an option?
The command being invoked is
$ /home/epage/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/clippy-driver /home/epage/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/rustc -vV
So CLIPPY_ARGS is being passed through to this which is causing it to fail.
Trying 1.75, this wasn't a problem. Looks like this might have been broken by #13659
CC @RalfJung
@RalfJung , I'm not seeing a problem statement in #10885. Any further details on that?
Would not caching failed invocations in .rustc_info.json be an option?
Caching of failures was intentionally added in #9112 (cc5e9df64a7ed6b3657e9dd790e2e34387d33df5)
This commit updates the rustc info cache to cache failures to execute rustc as well as successes. This fixes a weird issue where if you're probing for flags the
rustc_info_cache
test fails on channels which don't have the flag since previously a failure to execute rustc resulted in never caching the result.
The problem in that case isn't too clear to me and would need further investigation to go down that route.
@RalfJung , I'm not seeing a problem statement in #10885. Any further details on that?
That issue explains the motivation, not sure what to add. Also, what good is a "wrapper" that doesn't actually get invoked for all rustc invocation? If the goal of the wrapper is to e.g. swap out which rustc is being called, it is crucial that all invocations are made through the wrapper.
Seems like the clippy wrapper is just blindly adding extra arguments to all invocations. Note that rustc -vV
is not the only such "query invocation" cargo does, it also does a rustc invocation to print all available targets, and that has always gone through the wrapper. So something must be treating that query different -- either that doesn't get cached for some reason (seems unlikely) or clippy is already aware of it and avoids adding potentially faulty arguments, in which case that logic needs to be extended to recognize other "queries" as well.
That issue explains the motivation, not sure what to add.
From my looking it came down to
This is causing us a bunch of pain right now since we need to figure out how to best intercept this -vV call.
without an explanation of why that call needs to be intercepted
Seems like the clippy wrapper is just blindly adding extra arguments to all invocations. Note that rustc -vV is not the only such "query invocation" cargo does, it also does a rustc invocation to print all available targets, and that has always gone through the wrapper. So something must be treating that query different -- either that doesn't get cached for some reason (seems unlikely) or clippy is already aware of it and avoids adding potentially faulty arguments, in which case that logic needs to be extended to recognize other "queries" as well.
This would be a good next step to investigate when considering all possible directions for resolving this.
Please also read the rest of the issue. :)
The context is that Miri and bootstrap use RUSTC_WRAPPER to completely replace the rustc being invoked; there is no a priori relationship between $RUSTC_WRAPPER rustc
and the rustc
in the path. Why they need to do that is not super relevant, it's exactly the kind of usecase that RUSTC_WRAPPER is designed for and they have done this for many years. Because $RUSTC_WRAPPER rustc
invokes a completely different binary, the result cargo gets by invoking rustc -vV
without the wrapper is obviously bogus since it is talking to the wrong compiler.
It was an issue in 1.75 also
export RUSTUP_TOOLCHAIN=1.75
cargo new --lib demo && cd demo
cargo check
cargo clippy -- -x
cargo clippy
error: failed to run `rustc` to learn about target-specific information
Caused by:
process didn't exit successfully: `...\clippy-driver.exe '...\rustc.exe'
- --crate-name ___ --print=file-names --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=split-debuginfo --print=crate-name --print=cfg` (exit code: 1)
--- stderr
error: Unrecognized option: 'x'
error: failed to run `rustc` to learn about target-specific information
Caused by:
process didn't exit successfully: `...\clippy-driver.exe '...\rustc.exe'
- --crate-name ___ --print=file-names --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=split-debuginfo --print=crate-name --print=cfg` (exit code: 1)
--- stderr
error: Unrecognized option: 'x'
Skipping some steps if we encounter --print
/etc is something I'll try on the clippy driver side
Yeah that is exactly the other "query invocation" I mentioned above. https://github.com/rust-lang/cargo/pull/13659 just made it so that both query invocations are fed through the wrapper consistently, but clippy's wrapper behavior in combination with caching was a problem even when just one of the queries is fed through the wrapper.
Skipping some steps if we encounter --print/etc is something I'll try on the clippy driver side
FWIW, this is Miri's logic for determining whether the invocation is an "info query" and hence needs to be treated differently:
@RalfJung please assume people have made a good faith effort. In this case, I did look over the issue and another one it links to, multiple times in case I missed something.
Whats still not clear is the end-user side effect of what you are wanting, the end-user either being the RUSTC_WRAPPER
author or their end user. So far this is coming across as a purity argument ("its not that tool so its not that version").
Looking over the code, it looks like we primarily use it for MSRV checks and fingerprinting of rustc. The first requires the result to be representative of the compiler / stdlib version being performed. Unsure if there are other factors helping with fingerprinting or if that is ultimately what you are looking for here.
@epage sorry for that. I was slightly frustrated since your questions sounded somewhat dismissive. I think there are nicer ways to ask "would you mind explaining which RUSTC_WRAPPER usecase is affected by this"; the undertone of your question that I received was "this looks useless, what are you doing". My reply was still unnecessary.
As described here (but admittedly without going into much detail), the end users I am concerned about are rustc bootstrap and cargo-miri
. They both set RUSTC_WRAPPER. rustc bootstrap uses the wrapper to decide which stage's compiler to invoke. Things would obviously go quite wrong if cargo got the wrong rustc version here. To achieve that, bootstrap currently sets both the RUSTC and RUSTC_WRAPPER env vars, but that then requires awkward hacks in the rustc
binary that it uses for both of those env vars because the binary has to auto-detect whether it got invoked as $RUSTC
or as $RUSTC_WRAPPER $RUSTC
. That hack also got linked in the issue I referenced above. cargo-miri
has similar hacks. (Sadly Miri will likely have to keep these hacks forever as there are too many build scripts out there that invoke rustc without accounting for the wrapper. But that is no excuse for cargo itself to do this incorrectly, and rustc bootstrap may get away with dropping the hack since it more tightly controls which crates it is run on.)
Anyway I think the discussion is rather moot now since the Clippy issue predates my PR.
Yes, with the verification that that change didn't break rustc, it is no longer a blocker for this issue. I'll copy the relevant notes to that issue.
This commit updates the rustc info cache to cache failures to execute rustc as well as successes. This fixes a weird issue where if you're probing for flags the rustc_info_cache test fails on channels which don't have the flag since previously a failure to execute rustc resulted in never caching the result.
Alex was referring to calls like
let supports_split_debuginfo = rustc
.cached_output(process.clone().arg("-Csplit-debuginfo=packed"))
.is_ok();
which we do when we don't wait two releases before calling something.
If we reverted cc5e9df64a7ed6b3657e9dd790e2e34387d33df5, then the next time we add a probe, it will break the tests for our caching and the mixture of messages from the failure (not cached) and success (cached) cases would reduce the tests insight into what is working. This test change would then need to be reverted if/when we remove the probe.
This wasn't called out but cc5e9df64a7ed6b3657e9dd790e2e34387d33df5 should have also reduced the overhead from the flag probing.
While miri and clippy can workaround this, it is a foot gun for anyone writing a RUSTC_WRAPPER
and I'd prefer that we not have tests be negatively impacting the user experience like that. I'm also not sure what we should change the tests to. The most important aspect is that we don't have behavior regressions. Personally, I tend to put a lower priority on testing "is an implicit cache giving us the right performance" because I don't think its usually worth the invasive changes to verify.
FWIW I don't think Miri is concerned with working around this. It anyway has to detect these "info queries" as they otherwise throw off our heuristics for whether the current rustc invocation is a "host" or a "target" build (which matters a lot for Miri because all "target" builds are just interpreted, not compiled to machine code).
Problem
From https://github.com/rust-lang/rust-clippy/issues/12623#issuecomment-2034588171:
Output:
cargo clippy -- -x
is expected to fail but the followingcargo clippy
is notIn normal operation
clippy-driver
gets far enough along to registerCLIPPY_ARGS
into the env depinfo but that doesn't happen hereSteps
No response
Possible Solution(s)
CLIPPY_ARGS
could be added to the fingerprint as a special case, but the issue could also exist for other wrappersNotes
No response
Version
No response