[native_toolchain_c] Fix MSVC builds when paths contain spaces#3453
Conversation
|
@dcharkes do you mind checking this out? |
Package publishingIf you have publishing permissions, you can use the links below to publish the changes after merging this PR.
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
| arguments, | ||
| workingDirectory: workingDirectory?.toFilePath(), | ||
| environment: environment, | ||
| runInShell: Platform.isWindows && workingDirectory != null, |
There was a problem hiding this comment.
Hm, I don't remember why I did this. I remember it was to work around some other bug. But let's try removing it and see if anything breaks.
There was a problem hiding this comment.
runInShell was modified in #1759.
Check out the diff https://github.com/dart-lang/native/pull/1759/changes#diff-a9ce5e451f2dda3e3d822fa8660b26845b97441b58e55575af6673bb9e176cbeL59
Also the run_process.dart in the hooks_runner:
runInShell:
Platform.isWindows &&
(!includeParentEnvironment || workingDirectory != null),It seems to be a blend of both?
Description
Building with MSVC failed with
'C:\Program' is not recognized as an internal or external commandwhenever a source or include path contained a space —e.g. the default pub cache under a Windows user name with a space
(
C:\Users\First Last\AppData\Local\Pub\Cache\...).The cause:
runProcessran every command throughcmd.exe /con Windows(when a working directory was set), and cmd strips the outer quotes once the
command line contains more than two quote characters — so a quoted
cl.exepath plus one quoted argument was enough to mangle the invocation.
Changes:
runProcessno longer runs through a shell;cl.exe/lib.exeare invokeddirectly. (No caller needs shell features: on Windows programs expand
wildcards themselves, and the vcvars environment setup uses its own
Process.run.) As a consequence,cmd.exe%VAR%expansion no longerapplies to compiler flags/defines — they now reach the tools verbatim.
sources(matching
cl.exe /c's naming) instead of*.obj. This keeps object filesleft in the output directory by other builds out of the archive, and keeps
archive member names short and machine-independent instead of embedding
the local build path (verified with
lib.exe /list).it can be copy-pasted to reproduce an invocation.
spaces in the source, include, and output paths.
Related Issues
Fixes #3321.
Fixes #2848.
PR Checklist
dart tool/ci.dart --alllocally and resolved all issues identified. This ensures the PR is formatted, has no lint errors, and ran all code generators. This applies to the packages part of the toplevelpubspec.yamlworkspace.CHANGELOG.mdfor the relevant packages. (Not needed for small changes such as doc typos).