autocfg still calls rustc without the wrapper

[RalfJung]

I'm playing around with a patched version of cargo that lets me set

RUSTC_WRAPPER=<my binary here>
RUSTC=/path/that/does/not/exist

This makes it immediately obvious when something bypasses RUSTC_WRAPPER.

And it turns out this makes autocfg panic. I think it's this call that is not wrapped:

autocfg/src/lib.rs

Line 160 in c10bc17

let rustc_version = try!(Version::from_rustc(&rustc));

[cuviper]

Hmm... I'm not sure that we should, given rust-lang/cargo#10885, but I suppose if cargo is willing to change then we can follow suit. We can't make that behavior conditional on version, since that's what we're probing there, but maybe if the wrapper fails we can try again without it.

[dtolnay]

This might alternatively be a feature request for Cargo. Do RUSTC_WRAPPER and RUSTC_WORKSPACE_WRAPPER even need to be exposed to build scripts, or should Cargo unset those when running the build script and instead provide a RUSTC (or PATH containing a rustc?) which is a shim executable that already has baked in the right wrappers?

Exposing the original *_WRAPPER variables to build scripts is only useful if we want build scripts to be able to run those rustc wrappers with the build script's own choice of rustc different from the current one, which seems dubious. The downside of exposing the original variables to build scripts is... we're counting on every build script to incorporate relatively verbose code to apply both these wrappers correctly, as in this issue.

Possible implementations for fixing this on the Cargo side include:

• Generate a shell script that embeds the values of the wrapper environment variables, and set the script as RUSTC.

  #!/bin/sh
  "/path/to/rustc-wrapper" "/path/to/rustc-workspace-wrapper" "/path/to/rustc" "$@"

• The same thing but without shell: run rustc to compile a Rust program that embeds the shim's compile-time value of env!("RUSTC_WRAPPER") which is then no longer set during the shim's run-time.

• The same thing but without Rust compilation penalty: ship a precompiled shim that reads wrapper paths out of a sibling file. Pseudocode:

  fn main() {
      let current_exe = env::current_exe().unwrap();
      let rustc_wrapper = match fs::read_to_string(current_exe.with_file_name("rustc-wrapper")) {
          Ok(content) => Some(content),
          Err(err) if err.kind() == io::ErrorKind::NotFound => None,
          Err(err) => /* fail */,
      };
      let rustc_workspace_wrapper = /* similar */;
      let rustc = /* similar */;  // (or all from the same file)
  
      let mut wrapped_rustc = rustc_wrapper
          .into_iter()
          .chain(rustc_workspace_wrapper)
          .chain(iter::once(rustc));
      let mut cmd = Command::new(wrapped_rustc.next().unwrap());
      cmd.args(wrapped_rustc);
      cmd.args(env::args_os().skip(1));
      cmd.exec();
  }

Any of these, especially the last one, seems better than boiling the ocean to update every real-world build script.

[RalfJung]

This might alternatively be a feature request for Cargo.

Sounds like a new approach to resolve this feature request: rust-lang/cargo#11244

[cuviper]

better than boiling the ocean to update every real-world build script.

I would expect it's more of a pond than an ocean -- do you really think manual rustc probing is so common?

[dtolnay]

This kind of thing is common: https://github.com/dtolnay/semver/blob/1.0.22/build.rs#L67-L68

In my own crates, the build scripts are written so that if $RUSTC --version fails then the fallback configuration corresponds to a default relatively recent stable rustc. But in the rest of the ecosystem, my impression is it's much more common that build scripts default to assuming a super old rustc, or fail altogether, so this interacts poorly with Ralf's RUSTC=/path/that/does/not/exist patch.

Some examples in high-profile crates:

• https://github.com/seanmonstar/httparse/blob/b2625f38cc3a200e840861f2a3371eb8f5afdd73/build.rs#L50-L52
• https://github.com/google/zerocopy/blob/b56c01b91ca0745ad9d0036638aac8bfabfd3315/build.rs#L69-L71
• https://github.com/PyO3/pyo3/blob/cf74624de9c88e51914c7d2a2dbae5c4d4a6e63d/pyo3-build-config/src/lib.rs#L148-L156

And here's hundreds more uses of $RUSTC: https://github.com/search?q=%2F%5Cbvar(_os)%3F%5C(%22RUSTC%22%5C)%2F%20path%3Abuild.rs&type=code. Who knows what they're all doing but practically none of them pay attention to *_WRAPPER.

[cuviper]

Ouch, that's a lot more than I expected...

[RalfJung]

Yeah... that makes setting RUSTC to a non-existent path not very appealing. :/

So maybe the farthest we can go is to say that if you invoke RUSTC directly without the wrapper all you should do is query the version. You definitely can't expect the sysroot to be the same as with the wrapper though, and you can't expect direct RUSTC invocations to support the target indicated by TARGET.

[RalfJung]

That said, rustc bootstrap may still want to require the wrapper to be called even for --version. We can be picky about dependencies there, and making such a requirement could simplify some of the logic and make it less likely that bootstrap's sysroot management is accidentally circumvented.

[taiki-e]

FYI, clippy-deriver is one of the wrappers that currently does not work properly in the use case of getting the rustc version.

$ clippy-driver rustc --version                                              
clippy 0.1.79 (f9b16149 2024-04-19)

$ clippy-driver rustc --version --verbose
clippy 0.1.79 (f9b16149 2024-04-19)

$ clippy-driver rustc --rustc --version  
rustc 1.79.0-nightly (f9b161492 2024-04-19)

$ clippy-driver rustc --rustc --version --verbose
rustc 1.79.0-nightly (f9b161492 2024-04-19)
binary: rustc
commit-hash: f9b16149208c8a8a349c32813312716f6603eb6f
commit-date: 2024-04-19
host: aarch64-apple-darwin
release: 1.79.0-nightly
LLVM version: 18.1.4

$ clippy-driver rustc -V
clippy 0.1.79 (f9b16149 2024-04-19)

$ clippy-driver rustc -Vv
rustc 1.79.0-nightly (f9b161492 2024-04-19)
binary: rustc
commit-hash: f9b16149208c8a8a349c32813312716f6603eb6f
commit-date: 2024-04-19
host: aarch64-apple-darwin
release: 1.79.0-nightly
LLVM version: 18.1.4

$ clippy-driver rustc -vV
rustc 1.79.0-nightly (f9b161492 2024-04-19)
binary: rustc
commit-hash: f9b16149208c8a8a349c32813312716f6603eb6f
commit-date: 2024-04-19
host: aarch64-apple-darwin
release: 1.79.0-nightly
LLVM version: 18.1.4

AFAIK clippy-driver is usually only used as RUSTC_WORKSPACE_WRAPPER, so I think we could work around the problem by applying only RUSTC_WRAPPER when getting the rustc version. (The bug should be fixed on the clippy side, but a workaround is needed anyway, since rustc version detection needs to work with older toolchains.)

Since RUSTC_WORKSPACE_WRAPPER applies only for workspace members, unlike RUSTC_WRAPPER which applies for all crates, it should not change the toolchain (since it will result in linking crates compiled with different rustc versions). So ignoring RUSTC_WORKSPACE_WRAPPER when getting the rustc version should not affect the correctness of the results.

[RalfJung]

Why is clippy-driver even a wrapper? The equivalent miri binary is not.

I would argue this is a bug in clippy-driver. A wrapper is supposed to wrap the rustc invocation, not change its --version output. The fact that -Vv and --version --verbose behave differently also shows that clippy-driver is behaving sketchy here.

[RalfJung]

Filed as a clippy bug: rust-lang/rust-clippy#12697

[cuviper]

It also seems weird to me that the clippy wrapper would be set while executing a build script, but so it is...

[RalfJung]

That is expected, it is set for the entire "cargo clippy" invocation and cargo does not unset anything for build scripts. After all build scripts should also honor which RUSTC cargo invokes (and with which wrapper).