Inform build scripts of rustc compiler context

[jonhoo]

Fixes #9600.

[rust-highfive]

r? @alexcrichton

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

[jonhoo]

Tests and documentation coming shortly.

[dtolnay]

How feasible would it be to fold all of this into the existing RUSTC env var? Cargo can emit a wrapper script that functions like rustc but passes the right additional rustc flags to the right rustc wrapper. Basically I would prefer if all existing uses of Command::new(var_os("RUSTC").unwrap()) could just work correctly, without needing additional logic to handle two more env vars.

Pseudocode when Cargo's caller invokes Cargo with CARGO_BUILD_RUSTC_WRAPPER=/path/to/wrapper and CARGO_BUILD_RUSTFLAGS=--foo:

#!/bin/bash

/path/to/wrapper /path/to/rustc --foo "$@"

Then Cargo would make the env::var_os("RUSTC") seen by the build script refer to this wrapper script.

[jonhoo]

Yeah, I've wanted this too. My guess is that it's a bit of a pain since we'd need such a wrapper for every target. And furthermore, it likely shouldn't take any new dependencies (like bash). Maybe we could build a Rust wrapper for the target platform and invoke that, but that feels somewhat heavy-handed. I think @alexcrichton is the right person to chime in on this.

[alexcrichton]

I'm not sure which is the best way to go on this (generating a wrapper script vs just passing a bunch of env vars). I've got some experience with the cc crate on this in that people like to set CC='ccache cc' or something like that and expect things to work. The cc crate, however, has to implement logic to unpack the user's request into component parts because it does some degree of compiler detection based on the compiler's executable name. The analogy isn't perfect here in any case.

Basically I think there's two possible routes for this:

• One is to set the RUSTC env var with a wrapper script, and forcibly unset variables like RUSTC_WRAPPER and RUSTFLAGS. This would get RUSTC-based invocations and recursive cargo-based invocations working by default.
• Another options is to pass down a bunch of variables, e.g. RUSTC as today as well as RUSTC_WRAPPER and RUSTFLAGS recursively. This would get nested cargo-based invocations working but anything wanting to use rustc would need to manually interpret the flags/wrapper.

I could see this going either way, personally, although naturally it's much easier from an implementation perspective to just pass down the components and let the build script to figure it out. I don't get the feeling that it's super common to want to invoke rustc with flags/wrappers that it's worth going out of our way to inject a wrapper script.

You're right in that we naturally can't pick up bash as a requirement to run Cargo, so the way that I would imagine this working is that we'd do something like hard-link the cargo binary itself under a different name, say cargo-thinks-its-rustc. Then when Cargo starts up it sees if its name is cargo-thinks-its-rustc and if so it reads the component env vars and execs into rustc itself. This has the overhead of managing a hard-link (or a copy if that fails) of the cargo binary into probably the target directory).

[jonhoo]

I think my personal preference is the envvars, since it gives the recipient the choice of whether to use the flags or not. For example, I could maybe imagine an intermediate that wants to use RUSTC_WRAPPER, but not RUSTFLAGS.

[alexcrichton]

@dtolnay do you have reason to believe that reading RUSTC and wanting to take all these env vars into account is common throughout the ecosystem? I'd ideally prefer to not have to make a shim script, but if this is a common pattern I could be persuaded otherwise.

[dtolnay]

Well certainly anyhow needs them taken into account. See dtolnay/anyhow#156 and https://github.com/dtolnay/anyhow/blob/1.0.41/build.rs. As far as I know the rest of my own uses of env::var_os("RUSTC") are along the lines of https://github.com/serde-rs/serde/blob/v1.0.126/serde/build.rs where it's unimportant whether or not they are taken into account. I expect most uses in the ecosystem are like that. But I can't currently think of any that specifically want them not taken into account (irrespective of Jon's imagination in #9601 (comment)).

[jonhoo]

That serde build script makes me wonder how common it is to have a rustc wrapper that changes the Rust version that is available 🤔

[alexcrichton]

Presumably though the logic that you're implementing for anyhow is already centralized in other crates like autocfg, though, right? I understand the desire to not have a dependency, but if this is the case I wouldn't see this as much motivation for having a wrapper script. Requiring a small handful of crates to handle these env vars I think is pretty reasonable, and then if crates need to duplicate the logic because they don't want the dependency I think that's also reasonable but it doesn't really count toward the set of crates that need to be updated in my mind.

So, personally, I remain convinced that this should be communicated through separate env vars. I still feel, though, that the env vars should not have the CARGO_ prefix.

[dtolnay]

Makes sense, I am on board with that.

[cuviper]

My hesitation with reusing unprefixed RUSTFLAGS is that I'd have to know the cargo version to know whether that is a "trustworthy" value, or whether I still need my old imperfect heuristics. I could trust a new CARGO_RUSTFLAGS blindly when that exists.

But if I must check the version, would you consider setting CARGO_VERSION so I don't need to run another command? For that matter, RUSTC_VERSION would also be helpful, since I think cargo already knows that.

[alexcrichton]

Oh I tried to address the trustworthiness part above, but I think it'd be fine to export version env vars too. We could even break it down into major/minor/patch most likely since we're already parsing with semver.

[jonhoo]

Okay, I pushed two new commits that remove the CARGO_ prefix and add CARGO_VERSION, RUSTC_VERSION (and _MAJOR, _MINOR, and _PATCH variants).

[ehuss]

@jonhoo or @alexcrichton, a problem came up with this in #10111 where this broke previous behavior, where RUSTFLAGS is no longer set at all. I didn't catch that, and was wondering what the reasoning was behind that change?

[jonhoo]

Ah, so, Cargo never explicitly set RUSTFLAGS for build scripts, but it did propagate RUSTFLAGS if it was set in the environment. But with this PR, it now removes RUSTFLAGS if it detects that it was set. I think the rationale for that was that whatever RUSTFLAGS is in the environment doesn't necessarily represent the flags that end up being passed to rustc. The obvious example of this is if RUSTFLAGS is empty, then Cargo will use the rust flags from the config (the previous code would leave RUSTFLAGS empty to build scripts in this case, leading them to do "the wrong thing").

So the short of it is that relying on RUSTFLAGS in build scripts only sometimes gave correct behavior in the past. With the new code, there isn't the temptation to use the incorrect RUSTFLAGS by accident, and thus consumers are encouraged to use the correctly-populated CARGO_ENCODED_RUSTFLAGS instead.