Pass rustc shim flags using environment variable

[Kobzol]

This PR implements a generalized way of passing of host flags to the rustc shim in bootstrap, as proposed here.

I tried to implement the bootstrap side using OsString, but then I realized that the shim code was using env::var before anyway, instead of env::var_os, so I just settled on a String. The shim side is still general and uses env::vars_os now.

I'm not sure if we actually need to do something with the rustdoc shim. It seems to me that the env. vars passed to it (RUSTDOC_LINKER) and (RUSTDOC_LLD_NO_THREADS) could just be passed to cargo directly (or rather, the commands that they invoke in the shim could be passed directly). I'm not sure why are they set by the shim.

r? @onur-ozkan
CC @petrochenkov

[petrochenkov]

could just be passed to cargo directly

If something can be passed directly rather than through the shims, it certainly should be.
The shims only exist due to cargo deficiencies, the logic in them should be minimized when possible, and ideally they wouldn't exist at all.

[Kobzol]

@petrochenkov The rustdoc linker flag was first added in #45191 😅 I don't understand the full context from the PR though, notably what is "...the issue with doctests...".

[petrochenkov]

It was many years ago, I don't remember what was the issue exactly 🤦
But rustdoc needs linker because it compiles and runs code snippets in doc comments (doctests).

[Kobzol]

But rustdoc needs linker because it compiles and runs code snippets in doc comments (doctests).

I'll send a PR where I'll try to pass the linker directly, without the shim. Seems to not crash locally.

[petrochenkov]

But rustdoc needs linker because it compiles and runs code snippets in doc comments (doctests).

I'll send a PR where I'll try to pass the linker directly, without the shim. Seems to not crash locally.

It needs to be tested separately, the linker option in config.toml is not tested by CI, AFAIK.
I tested #45191 by setting [target.x86_64-unknown-linux-gnu] linker to my custom nonsensically named symlink and making sure there are no other linkers with standard names are in PATH.

[onur-ozkan]

thanks!

[onur-ozkan]

@bors r+ rollup=iffy

[bors]

📌 Commit eddbd7c has been approved by onur-ozkan

It is now in the queue for this repository.

[petrochenkov]

I think rustdoc shim should also be migrated before merge, #116449 is unlikely to work.

[Kobzol]

I would rather do these things in two separate PRs, if we needed to revert them in the future. I think that they are orthogonal (or not?).

[onur-ozkan]

Feel free to dequeue the PR if you want to handle rustdoc too in this PR.

[petrochenkov]

@bors r-
Even for the rustc shim this PR seems to only scratch the surface.
The shim adds many compiler options, for both host and target, that can be passed in a pre-cooked way through the new env var, while in this PR even host options are not fully migrated.

I think, the only options that cannot be passed like this are those that inspect which specific crate we are building right now (e.g. inspect crate_name).

[Kobzol]

else {
        // FIXME(rust-lang/cargo#5754) we shouldn't be using special env vars
        // here, but rather Cargo should know what flags to pass rustc itself.

        // Find any host flags that were passed by bootstrap.
        // The flags are stored in a RUSTC_HOST_FLAGS variable, separated by spaces.
        if let Ok(flags) = std::env::var("RUSTC_HOST_FLAGS") {
            for flag in flags.split(' ') {
                cmd.arg(flag);
            }
        }

        // Cargo doesn't pass RUSTFLAGS to proc_macros:
        // https://github.com/rust-lang/cargo/issues/4423
        // Thus, if we are on stage 0, we explicitly set `--cfg=bootstrap`.
        // We also declare that the flag is expected, which we need to do to not
        // get warnings about it being unexpected.
        if stage == "0" {
            cmd.arg("--cfg=bootstrap");
        }
        cmd.arg("-Zunstable-options");
        cmd.arg("--check-cfg=values(bootstrap)");
    }

This is (AFAIK) the only block that deals with host-only options. Do you want me to also pass these three options via the env. variable? Since they are either unconditional or dependent on stage, which is readily available, I thought that it would be easier to keep this in the shim. If we pass --cfg=bootstrap, we could remove passing the stage in theory, though.

[petrochenkov]

Do you want me to also pass these three options via the env. variable?

Yes, if possible.
It fall under the general idea that ideally the shim shouldn't exist at all, that I mentioned above.

[Kobzol]

Looks like it might be spurious? I would just retry.

[onur-ozkan]

@bors retry

[Kobzol]

@bors r=onur-ozkan,petrochenkov

[bors]

💡 This pull request was already approved, no need to approve it again.

• There's another pull request that is currently being tested, blocking this pull request: Rollup of 5 pull requests #116640

[bors]

📌 Commit 3f9ab7a has been approved by onur-ozkan,petrochenkov

It is now in the queue for this repository.

[bors]

⌛ Testing commit 3f9ab7a with merge 9d1e4b7...

[bors]

☀️ Test successful - checks-actions
Approved by: onur-ozkan,petrochenkov
Pushing 9d1e4b7 to master...

[rust-timer]

Finished benchmarking commit (9d1e4b7): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-1.4%, -1.4%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.9%	[2.2%, 5.5%]	2
Regressions ❌ (secondary)	1.8%	[1.8%, 1.8%]	1
Improvements ✅ (primary)	-3.9%	[-5.6%, -2.5%]	3
Improvements ✅ (secondary)	-2.2%	[-2.7%, -1.6%]	2
All ❌✅ (primary)	-0.8%	[-5.6%, 5.5%]	5

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	2.1%	[2.1%, 2.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.8%, 0.8%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 626.264s -> 629.226s (0.47%)
Artifact size: 271.33 MiB -> 271.32 MiB (-0.01%)