fix: honor Build::env overrides in flag support probes by MohammedAlkindi · Pull Request #1866 · rust-lang/cc-rs

MohammedAlkindi · 2026-08-20T11:22:23Z

Implements the design settled in #1859.

is_flag_supported_inner builds its probe Build without copying self.env, so Build::env overrides are ignored by flag probes: the probe resolves a bare cc through the ambient PATH and answers about a different compiler than the one being built with. Since #1845 made -mno-omit-leaf-frame-pointer conditional on is_flag_supported, that silently changes flags. Family detection in src/tool.rs had the same gap at four more spawns, which applied no environment at all.

Copying self.env alone shifts the shim's out{N} indices, so this adds CC_SHIM_OUT_FILES_FOR_FAMILY_DETECTION and CC_SHIM_OUT_FILES_FOR_FLAG_SUPPORT_CHECK. cc renames the class-appropriate one to CC_SHIM_OUT_FILES before spawning that probe; when it is absent the probe records nothing, per your call in #1859. cached_compiler_family is now keyed on the probe environment too, so two builds differing only in Build::env cannot share a family answer.

Baseline caf7609 vs this branch, cargo nextest run --no-fail-fast --workspace, x86_64-pc-windows-msvc:

	baseline	branch
total	115 run, 112 passed, 3 failed, 2 skipped	116 run, 115 passed, 1 failed, 2 skipped
`gnu_debug_fp`	FAIL	PASS
`gnu_debug_fp_auto`	FAIL	PASS
`clang_android`	FAIL	FAIL (unchanged)
`probe_env_overrides`	n/a	PASS (new)

Doc tests 28 to 28, no regressions. clippy --no-deps --workspace --all-targets -- -D warnings clean, fmt --check clean, and cargo check -p cc passes on 1.65.0.

Three things worth your attention:

Once detection runs under Build::env it reaches the shim, which emitted no family macros but exited 0 on -?, so every Test::gnu() compiler classified as MSVC. Previously detection failed only because the shim panicked on a missing CC_SHIM_OUT_DIR. The shim now declines -E explicitly rather than learning to preprocess. Family answers match baseline for every shim name in the suite.

clang_android is the same root cause at a third site — src/lib.rs:3723 probes llvm-ar with a bare Command::new, bypassing Build::env's PATH. I left it out: fixing it needs a third recording class, which expands a design you settled at two, and it needs its own test since your CI runners ship a real llvm-ar. Happy to send it separately.

Everything above is Windows-only. I audited the cfg(not(windows)) tests for dependence on a real compiler and converted the one that needed a probe rejection, but your CI settles Unix.

#851 and #1632 touch the same region of src/lib.rs; neither implements this design and this does not subsume them.

LLM usage, per the Rust policy: this change was developed with AI assistance (Claude). Adding this retroactively, since it should have been disclosed when the PR was opened.

`is_flag_supported_inner` built its probe from a fresh `Build`, dropping `Build::env` along with the out dir and the caches. The probe therefore resolved a bare compiler name such as `cc` through the ambient `PATH` rather than the one the compile commands run in, and answered a question about a different compiler than the one being built with. Compiler family detection had the same gap at four more spawns: they applied no environment at all, and `Tool::with_features` was never handed one. Both now run in the environment `Build::env` sets up, and the family lookup cache is keyed on that environment as well as the path and arguments, so two builds that differ only in it cannot share an answer. Reaching the right compiler means the probes now reach the test shim, where they would have taken `out{i}` slots and shifted the numbering every assertion depends on. Two variables opt one class of probe in by name instead: `CC_SHIM_OUT_FILES_FOR_FAMILY_DETECTION` and `CC_SHIM_OUT_FILES_FOR_FLAG_SUPPORT_CHECK`. cc renames the one matching the probe it is about to spawn to `CC_SHIM_OUT_FILES`, rather than copying `Build::env` over blindly, so a probe no test asked about records nothing at all. That replaces the `-c` sniffing the shim did to guess whether it was looking at a flag probe, and the panic on a missing `CC_SHIM_OUT_DIR` that family detection relied on to keep failing. `gnu_flag_if_supported` no longer skips Windows. It needed the probe to reject a flag and got that from whatever compiler the machine happened to have installed; it now says so with `CC_SHIM_FAIL_IF_ARG`. Refs rust-lang#1859

NobodyXu

Thank you so much!

Just some feedback on code but should be quite to settle

NobodyXu · 2026-08-20T13:29:46Z

clang_android is the same root cause at a third site — src/lib.rs:3723 probes llvm-ar with a bare Command::new, bypassing Build::env's PATH. I left it out: fixing it needs a third recording class, which expands a design you settled at two, and it needs its own test since your CI runners ship a real llvm-ar. Happy to send it separately.

Yeah that indeed worth a separate PR, thank you for this PR!

Four things from the review, no behaviour change. The shim collected argv twice. `split_first` gives the program and the rest in one step, and `record` now takes `&[String]` rather than `&[&String]`. The probe `Build` copies the env with `clone_from` instead of replaying it key by key. `ProbeKind` and `set_probe_env` are private again, behind a `CommandExt` trait with `set_family_detection_env` and `set_flag_supported_env`, so a caller names the probe class instead of passing an enum. The test harness gained `collect_family_detection_probes` and `collect_flag_supported_probes` with matching getters, so a test opts a class in by name and `CC_SHIM_OUT_FILES_FOR_*` stays inside the harness. The getters take an index because `clang_cl_scope_all` records two flag support probes in one build, so a single file per class was not enough.

MohammedAlkindi · 2026-08-20T13:39:29Z

All four done in 94eca3a. Thanks, the CommandExt one in particular made the tool module read better.

The extra collect. Gone. split_first gives the program and the rest in one step, so record now takes &[String] and there is only the one collect of env::args().

clone_from. Taken as suggested.

CommandExt. ProbeKind and set_probe_env are private again. The trait is on Command with set_family_detection_env and set_flag_supported_env, so tool.rs and lib.rs name the class rather than passing an enum.

The harness helper. collect_family_detection_probes and collect_flag_supported_probes, with get_family_detection_probes and get_flag_supported_probes to read them back. CC_SHIM_OUT_FILES_FOR_* and the file names now live entirely inside Test, so a test opts a class in by name.

One deviation from your sketch, and it is the only place I did not follow it exactly. The getters take an index:

pub fn get_flag_supported_probes(&self, i: usize) -> Execution

clang_cl_scope_all records two flag support probes in one build, one for -fmacro-prefix-map and one for -fdebug-prefix-map, so a single file per class could not hold both. Each class now gets four slots, filled in the order the probes run, and the test asks for 0 and 1. If you would rather have the singular signature with a separate accessor for the multi-probe case, say so and I will split it.

Verified on Windows 11, x86_64-pc-windows-msvc, from this branch:

every test binary green, and doc-tests 28 passed
tests/test.rs run one test per process: 58 of 59 pass, the exception being clang_android, which is the pre-existing third probe site we agreed belongs in its own PR
cargo clippy --workspace --all-targets -- -D warnings clean, cargo fmt --all --check clean
MSRV check -p cc on 1.65.0 clean

One honest note on the test run. cargo test --test archiver failed on this machine with An Application Control policy has blocked this file (os error 4551), which is a Windows policy blocking cargo's spawn of the freshly built binary, not anything in the code. Running that binary directly gives 3 passed, and it passes on the unmodified branch too, so it is my box rather than this change.

And agreed on clang_android, I will send that separately.

NobodyXu

Thanks, just two final nits, and it'd be ready for merge

NobodyXu · 2026-08-20T14:10:22Z

+        fn probe<'cmd>(cmd: &'cmd mut Command, env: &BuildEnv) -> &'cmd mut Command {
+            cmd.set_family_detection_env(env);


Do we still need this function? I think we can remove it?

NobodyXu · 2026-08-20T14:12:52Z

+    /// them by name with [`Test::probe_out_files`], so this numbering covers
+    /// the compile and archive commands only.
    pub fn cmd(&self, i: u32) -> Execution {
+        self.execution(self.td.path().join(format!("out{}", i)))


Can we use

Suggested change

self.execution(self.td.path().join(format!("out{}", i)))

self.execution(self.probe_slot("out", i))

MohammedAlkindi · 2026-08-20T14:23:22Z

Both done in f028de4.

The probe wrapper is gone. set_family_detection_env already returns &mut Command, so the four call sites now chain it directly and read better for it, for example:

Command::new(path).arg("--version").set_family_detection_env(env)

Test::cmd reuses probe_slot, as suggested. It takes usize now rather than u32 so the two share a signature.

Verified on Windows 11, x86_64-pc-windows-msvc: every other test binary green, doc-tests 28 passed, tests/test.rs 58 of 59 one test per process, clippy -D warnings clean, fmt clean, MSRV check -p cc on 1.65.0 clean. The one exception is clang_android, which is the third probe site and is fixed in #1867 rather than here, so it is expected to abort on this branch.

One piece of sequencing, since you mentioned merging this one first. #1867 still points at the pre-nit commit, because it was already approved and I did not want to force-push a rebase under the approval. If this lands as a squash, #1867's branch will carry the old versions of tool.rs and support/mod.rs and would undo f028de4 on merge. I will rebase it onto main as soon as this merges, so it arrives as just the one llvm-ar commit. Worth not merging #1867 until I have done that.

NobodyXu

Thank you, LGTM! Will wait for rebase on the follow-up PR

NobodyXu requested changes Aug 20, 2026

View reviewed changes

Comment thread src/bin/cc-shim.rs Outdated

Comment thread src/lib.rs Outdated

Comment thread src/command_helpers.rs Outdated

Comment thread tests/test.rs Outdated

MohammedAlkindi mentioned this pull request Aug 20, 2026

fix: run the Android llvm-ar probe in the caller's environment #1867

Closed

NobodyXu requested changes Aug 20, 2026

View reviewed changes

refactor: drop the probe wrapper and reuse probe_slot in Test::cmd

f028de4

NobodyXu approved these changes Aug 20, 2026

View reviewed changes

NobodyXu merged commit c5cc913 into rust-lang:main Aug 20, 2026
82 checks passed

This was referenced Aug 20, 2026

fix: run the Android llvm-ar probe in the caller's environment #1868

Merged

is_flag_supported ignores Build::env overrides; gnu_debug_fp/gnu_debug_fp_auto fail on Windows hosts without a system cc (since #1845) #1859

Closed

github-actions Bot mentioned this pull request Aug 21, 2026

chore(cc): release v1.4.4 #1869

Merged

Navigation Menu

Uh oh!

fix: honor Build::env overrides in flag support probes - #1866

fix: honor Build::env overrides in flag support probes#1866
NobodyXu merged 3 commits into
rust-lang:mainfrom
MohammedAlkindi:fix/probe-env-overrides

MohammedAlkindi commented Aug 20, 2026 •

edited

Loading

NobodyXu left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NobodyXu commented Aug 20, 2026

MohammedAlkindi commented Aug 20, 2026

NobodyXu left a comment

NobodyXu Aug 20, 2026

NobodyXu Aug 20, 2026

MohammedAlkindi commented Aug 20, 2026

NobodyXu left a comment

Uh oh!

Labels

2 participants

		fn probe<'cmd>(cmd: &'cmd mut Command, env: &BuildEnv) -> &'cmd mut Command {
		cmd.set_family_detection_env(env);

	self.execution(self.td.path().join(format!("out{}", i)))
	self.execution(self.probe_slot("out", i))

Uh oh!

Conversation

MohammedAlkindi commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

NobodyXu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NobodyXu commented Aug 20, 2026

MohammedAlkindi commented Aug 20, 2026

NobodyXu left a comment

Choose a reason for hiding this comment

NobodyXu Aug 20, 2026

Choose a reason for hiding this comment

NobodyXu Aug 20, 2026

Choose a reason for hiding this comment

MohammedAlkindi commented Aug 20, 2026

NobodyXu left a comment

Choose a reason for hiding this comment

Uh oh!

Labels

2 participants

MohammedAlkindi commented Aug 20, 2026 •

edited

Loading