Skip to content

Conversation

@dongfengweixiao
Copy link

For the runInShell parameter, I have preserved the original logic as much as possible, only setting runInShell to false when the executable file path contains spaces. I believe this change will better align with the expected behavior.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.

@google-cla
Copy link

google-cla bot commented Dec 18, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@dongfengweixiao dongfengweixiao changed the title Header path with space [native_toolchain_c] Header path and cl.exe path with space Dec 18, 2025
Copy link
Collaborator

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

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

Is there a way to trigger this in a test on the GitHub actions?

...?environment?.entries.map((entry) => '${entry.key}=${entry.value}'),
executable.toFilePath(),
...arguments.map((a) => a.contains(' ') ? "'$a'" : a),
...arguments,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this break other things?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you’re right. After testing, I found that my changes cause other issues in some scenarios.

workingDirectory: workingDirectory?.toFilePath(),
environment: environment,
runInShell: Platform.isWindows && workingDirectory != null,
runInShell: executable.toFilePath().contains(' ')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer a runInShell override in the method parameter, because otherwise it becomes too much magic why it choses what.

@dongfengweixiao dongfengweixiao changed the title [native_toolchain_c] Header path and cl.exe path with space [native_toolchain_c] cl.exe path with space Dec 19, 2025
When the Visual Studio C++ compiler (cl.exe) is installed in a path
containing spaces (e.g., the default "C:\Program Files\Microsoft Visual
Studio\2022\..."), the build system failed to execute it correctly.

The command was invoked without proper quoting, leading to an error:
  'C:\Program' is not recognized as an internal or external command

Signed-off-by: Dee HY <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants