Repository navigation
The no-op always-success RUSTC_WRAPPER breaks build scripts #12973
Description
It is not an uncommon pattern for build scripts to invoke rustc to detect whether some features are available. Popular crates like anyhow and eyre do that, and the autocfg crate (14 million "recent" downloads, >100 million all-time downloads) even provides a convenient interface to write such build scripts.
RA entirely breaks that by having a RUSTC_WRAPPER that always returns success. I think that is a bug in RA.
Activity
[-]The no-op RUSTC_WRAPPER breaks build scripts[/-][+]The no-op always-success RUSTC_WRAPPER breaks build scripts[/+]lnicola commentedon Aug 8, 2022
This was never reported before. Skimming
autocfg, it doesn't seem to useRUSTC_WRAPPER, which might explain the difference. You can use disablerust-analyzer.cargo.buildScripts.useRustcWrapperas a workaround.Specifically about
anyhow, I don't think it should be testing for a feature that no longer exist, but we don't control that.It would be nice for
cargoto have a "run the build scripts, but don't check the code" mode.RalfJung commentedon Aug 8, 2022
This wrapper might also be responsible for rust-lang/rust#99538; at least I have not seen that problem since I disabled proc macros and build scripts.
Oh, good point.
anyhow was specifically asked to use RUSTC_WRAPPER in dtolnay/anyhow#157. And I tend to agree with the author that using RUSTC_WRAPPER is more correct. But neither eyre nor autocfg are using RUSTC_WRAPPER so they happen to not hit this problem. I think that is an accident though, and arguments could be made that they should use RUSTC_WRAPPER.
Well, the probe needs updating to latest nightly, yeah. And soon the feature will be stable and this will only be relevant for old compilers. But that doesn't really help with the general problem.
lnicola commentedon Aug 8, 2022
One solution might be to detect nested invocations of our wrapper: if we're not the outer one, we could pass through the call to
rustc(or even to aRA_ORIGINAL_RUSTC_WRAPPERthat we've saved).bjorn3 commentedon Aug 8, 2022
Build scripts don't cause nested invocations of the wrapper.
lnicola commentedon Aug 8, 2022
Doesn't the
anyhowbuild script do just that?RalfJung commentedon Aug 8, 2022
Yes, it does.
bjorn3 commentedon Aug 8, 2022
Anyhow's build script invokes the wrapper, but the build script doesn't run underneath the wrapper, but directly under cargo. As such the wrapper invocation is not nested within the wrapper.
lnicola commentedon Aug 8, 2022
Am I missing something here?
RUSTC_WRAPPER=rust-analyzercargo checkcargo checkcompiles theanyhowbuild scriptcargo checkruns theanyhowbuild script, presumably without clearingRUSTC_WRAPPERRUSTC_WRAPPERand uses it to compile some codebjorn3 commentedon Aug 8, 2022
That is correct. But how do we detect that anyhow's build script invoked the wrapper rather than cargo itself? Both use pretty much the same env vars and because at step 4 the wrapper isn't used to run the build script, we can't set an env var either to signal that a build script ran rustc with the wrapper as opposed to cargo.
lnicola commentedon Aug 8, 2022
Can't we set
RA_RUSTC_WRAPPPER=$RUSTC_WRAPPERin step 1, then we check for it (in step 5) and run that instead?bjorn3 commentedon Aug 8, 2022
How do you make sure that it doesn't run for rustc invoked by cargo then?
bjorn3 commentedon Aug 8, 2022
I think we can check env vars like
NUM_JOBSorPROFILEwhich only get set for build scripts.RalfJung commentedon Aug 8, 2022
Yeah, fundamentally this is a hack to work-around not being able to tell cargo what you really want to do. Is that being tracked on the cargo side somewhere, are the cargo devs aware of this?
lnicola commentedon Aug 8, 2022
Hmm...
I suppose not, but a request for
cargo build --dependencies-only(which isn't that different) has been open since 2016 without any end in sight.4 remaining items
vlad20012 commentedon Aug 9, 2022
Wow, this seems like exactly what we need!
Auto merge of #12984 - Veykril:keep-going, r=Veykril
matklad commentedon Aug 11, 2022
🤔 I think
—-keep-going(which we maybe should rename to be consistent with analogous flag oflibtest) changes the calculus here significantly. It should allow us to remove the RUSTC_WRAPPER hack, as the main problem why we need that is not performance, but reliability.Veykril commentedon Aug 11, 2022
A quick test seems to not really change the times for me when running with and without the wrapper (though that might be because
cargo checkgets skipped due to caching here), I'd be in favor of just removing the wrapper, as it also makes cargo be confused about diagnostics (in rare cases it seems to succeedcargo checks on crates that have actual errors in them, see #12808)lnicola commentedon Aug 11, 2022
I think it's going to matter a lot for people who disable
rust-analyzer.checkOnSave.enable. But we can probably remove or disable it until we get some proper support in Cargo.Veykril commentedon Aug 12, 2022
Opened a zulip topic in the t-cargo stream to see if we can work on this from the cargo side of things as our own options here are rather limited and to me it sounds like it would make more sense to just push rust-lang/cargo#7178 forward
https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/cargo.20build.20--compile-time-deps.20cargo.237178/near/293136753
dtolnay commentedon Aug 13, 2022
#12973 (comment) sounds the most promising to me. Here are Cargo's build script environment variables: https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts. The criteria for selecting one for this purpose are:
CARGO_CFG_UNIXwhich is set on unix onlyHOSTThese are the ones that I see fit the bill:
CARGO_CFG_TARGET_ARCHCARGO_CFG_TARGET_ENDIANCARGO_CFG_TARGET_FAMILYCARGO_CFG_TARGET_OSCARGO_CFG_TARGET_POINTER_WIDTHCARGO_CFG_TARGET_VENDORNUM_JOBSOPT_LEVELPROFILESo the plan would be for rustc_wrapper.rs to do its early return thing only if
std::env::var_os("CARGO_CFG_TARGET_ARCH").is_none().Veykril commentedon Aug 13, 2022
Picking one of the
CARGO_CFG_TARGET_*ones seems most reasonable, I could see someone setting one of the later 3 on their system for something unrelated given their short names (though rather unlikely still of course).Veykril commentedon Aug 13, 2022
PR #13010 is up, though I can't test whether it works as I am actually not running into the issue with
anyhowfor some reason.sehz commentedon Aug 13, 2022
dtolnay commentedon Aug 13, 2022
This has been fixed by #13010.
Merge #9235