Handle xwin cache paths containing spaces - #229
Conversation
messense
left a comment
There was a problem hiding this comment.
Thanks for the PR! The --config target.<triple>.rustflags=[…] approach is the right way around cargo-config2's separator error, and the unit tests / fmt / clippy all pass locally. A few issues before this can land, though:
Blocking
1. Quoted include paths break everyone on cc < 1.1.11, even without spaces.
CC_SHELL_ESCAPED_FLAGS was only added in cc-rs 1.1.11 (Aug 2024, rust-lang/cc-rs#1181). Older cc ignores the env var and whitespace-splits CFLAGS_*, so /imsvc "/home/u/.cache/cargo-xwin/crt/include" becomes a literal argument with the quote characters in it and clang-cl can't find the headers. Lockfiles pinned to cc 1.0.x are still very common, so this would regress the default (no-spaces) case for a lot of users. Could we only quote / set CC_SHELL_ESCAPED_FLAGS when the cache path actually contains whitespace? That keeps today's behaviour for everyone else, and we can document the cc-rs requirement for the spaces case.
2. cargo xwin env regression.
src/env.rs only prints cmd.get_envs(). With the rustflags now passed as a --config argument, the -Lnative=… paths disappear from cargo xwin env output entirely, so eval "$(cargo xwin env)" && cargo build --target x86_64-pc-windows-msvc no longer links. env needs to keep emitting something equivalent (e.g. fall back to CARGO_TARGET_<T>_RUSTFLAGS there), or the --config path should be limited to the spawned command.
Should address
3. CC_SHELL_ESCAPED_FLAGS=1 is global, not target-scoped.
It changes how cc-rs parses all *FLAGS for the whole cargo run, including host builds (build scripts, proc-macros) and the user's own CFLAGS/CXXFLAGS/CL_FLAGS, which are appended verbatim. Anything with backslashes (/IC:\foo on a Windows host) or quotes now gets reinterpreted. Another reason to only enable it when needed, plus a README note.
4. RCFLAGS quoting is unverified.
RCFLAGS is consumed by embed-resource, not cc-rs, and I don't think the cargo check in the validation section exercised any .rc files. Please confirm embed-resource handles quoted args before changing the format. src/compiler/clang.rs still emits unquoted RCFLAGS, so the two backends would also disagree.
5. Rebuilding the Command in insert_cargo_config.
This silently drops anything get_* can't read back (env_clear, stdio, pre_exec). Nothing sets those today so it works, but it's fragile. Simpler: add --config where the command is built (build_command in src/macros.rs, run.rs, test.rs, bench.rs), e.g. by having apply_command_env return the extra args.
Minor
- Please keep the note that
RUSTFLAGS/CARGO_ENCODED_RUSTFLAGSare already folded intorustflagsvia cargo-config2; otherwise the newenv_removecalls look like they drop user flags. - The new tests cover the string formats well, but nothing exercises the real case end-to-end. A CI step with a cache path containing a space (
XWIN_CACHE_DIR="… /xwin cache") would be the actual regression guard.
|
Thanks for the detailed review. I revised the branch locally to address each point:
Local validation on the revised tree:
The serial test setting avoids the existing process-wide |
Summary
--configarray so each path remains one rustc argument when the xwin cache directory contains spaces.Problem
cargo xwincurrently serializes generated-Lnativearguments through the space-separated target rustflags variable. A cache path containing a space is rejected bycargo-config2before compilation withflag in rustflags must not contain its separator (' ').Using a TOML array resolves that failure without restoring global encoded rustflags, which would leak Windows linker flags into cross-target artifact dependencies. The generated clang-cl include flags also need quoting. Without shell-escaped parsing, cc-rs splits an SDK include path at the space and clang-cl cannot find the Windows headers.
Approach
The spawned Cargo command receives the already-resolved
target.<triple>.rustflagsas a--configTOML array. This preserves cargo-xwin's existing flag-resolution behavior while keeping each complete SDK path intact and retaining the target isolation introduced for cross-target artifact dependencies. The option is inserted before any trailing--arguments used bycargo test,cargo run, orcargo bench. The include-path helper emits quoted arguments forCL_FLAGS, targetCFLAGSandCXXFLAGS, bindgen, andRCFLAGS;CC_SHELL_ESCAPED_FLAGS=1tells cc-rs to parse those quoted arguments as shell-escaped flags.Validation
cargo xwin check --locked -p zeroclaw-providers --target x86_64-pc-windows-msvc --testscompleted successfully with xwin cache and Cargo target paths containing spaces.git diff --checkpasses.cargo test --locked compiler::clang_cl::testspasses (3 passed, 0 failed).