Skip to content

fix(probe): pin LC_ALL=C on compiler probe spawns (#741) - #775

Merged
jleni merged 1 commit into
kunobi-ninja:mainfrom
Ysh204:fix/probe-lc-all-c
Aug 19, 2026
Merged

fix(probe): pin LC_ALL=C on compiler probe spawns (#741)#775
jleni merged 1 commit into
kunobi-ninja:mainfrom
Ysh204:fix/probe-lc-all-c

Conversation

@Ysh204

@Ysh204 Ysh204 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #741

Pins LC_ALL=C on compiler probe subprocess spawns so that localized output (such as non-ASCII banner lines under non-English locales or localized --version strings) cannot reach probe parsers.

Summary of Changes

  • CcProber::probe: Added .env("LC_ALL", "C") to Command::new(req.compiler).arg("--version") so version_line (which is cache key material) is locale-independent.
  • run_family_probe: Added .env("LC_ALL", "C") to child_cmd (-E -P -x c).
  • resolve_invocation: Added .env("LC_ALL", "C") to Command::new(compiler).arg("-###").
  • live_probe_diagnostic_for: Added .env("LC_ALL", "C") to --version probe and unresolved stderr head re-run.
  • Test diagnostic fallback: Added .env("LC_ALL", "C") to cc -### test re-run.
  • Test: Added probe_spawns_pin_lc_all_c unit test verifying that compiler child processes receive LC_ALL=C.

Verification

  • just fmt-check passed cleanly.
  • cargo test probe_spawns_pin_lc_all_c passed.
  • cargo test probe:: passed (64/64 tests).
  • just test passed across all workspace crates.

@jleni jleni left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. The src/probe/mod.rs change looks good: pinning LC_ALL=C on all five probe spawns is the right fix, version_line is cache key material so locale independence matters there, and the unix test that asserts every spawn saw LC_ALL=C is a nice touch. GNU gettext also ignores LANGUAGE when the locale is C, so there is no escape hatch left open.

The Cargo.lock change should come out though. It is two separate things and neither belongs in this PR.

The h2 bump is already on main. h2 0.4.16 landed in 977ad4d (#772). It shows up in your diff only because this branch is based on 13899ad, from before that commit. RUSTSEC-2026-0258 was a real advisory and the intent was fine, it is just redundant now.

The windows-sys changes are a regression. The lock moves 12 dependency edges down from windows-sys 0.61.2:

crate main this PR
rustix, tempfile, seize, rustls-platform-verifier, winapi-util, errno 0.61.2 0.52.0
anstyle-query, anstyle-wincon, dirs-sys, nu-ansi-term, socket2, termina 0.61.2 0.60.2

Those crates declare wide ranges (rustix, tempfile and errno use >=0.52, <0.62, socket2 uses >=0.60, <0.62), and the workspace depends on windows-sys = "0.61" directly in Cargo.toml, so cargo unifies them onto 0.61.2. A fresh cargo generate-lockfile on current main moves one more edge up to 0.61.2 and moves nothing down, so this is not a resolution cargo would produce today. It is most likely a stale registry index or a hand resolved lock conflict. The practical cost is three copies of windows-sys compiled where main needs only the unified one. CI will not flag it because --locked accepts any valid lock.

Could you rebase on main and restore the lock so the PR touches src/probe/mod.rs only?

git fetch upstream main
git rebase upstream/main
git checkout upstream/main -- Cargo.lock

If the rebase leaves Cargo.lock untouched relative to main, the diff should be the probe change on its own.

@jleni

jleni commented Aug 19, 2026

Copy link
Copy Markdown
Member

To add some context on the ask above: we have been bitten by lock file churn in PRs before, so as a general rule I would rather keep dependency updates in their own change. For this one please limit the fix to the LC_ALL change only for now. Security bumps like the h2 one are worth doing, just as a separate PR where the lock diff is the whole point and easy to review on its own.

@Ysh204
Ysh204 force-pushed the fix/probe-lc-all-c branch from 13899ad to 18a21c0 Compare August 19, 2026 16:21
@Ysh204

Ysh204 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on upstream/main and restored Cargo.lock so this PR touches src/probe/mod.rs only.

@jleni

jleni commented Aug 19, 2026

Copy link
Copy Markdown
Member

Thanks!

@jleni
jleni merged commit 48c5a4e into kunobi-ninja:main Aug 19, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

probe: pin LC_ALL=C on compiler probe spawns so localized output cannot reach the parsers

2 participants