fix(xtask): launch non-exe commands through cmd.exe on Windows#79
fix(xtask): launch non-exe commands through cmd.exe on Windows#79
Conversation
On Windows, CreateProcessW only resolves .exe files, so non-.exe commands (e.g. .cmd/.bat scripts like pnpm) fail to spawn. Wrap command invocations through cmd /c on Windows to resolve these correctly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Attempt to fix #78 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a Windows-specific process-spawning failure in the xtask dev tooling. On Windows, CreateProcessW cannot resolve non-.exe commands such as .cmd/.bat scripts (e.g. pnpm). The fix wraps the spawned command in cmd.exe /c when cfg!(windows) is true, while leaving Unix behavior unchanged.
Changes:
xtask/src/process.rs: Restructuresspawn_childso that on Windows the command is launched viacmd.exe /c <cmd> <args...>, and on non-Windows it continues to useCommand::new(cmd)directly.
| let mut command = if cfg!(windows) { | ||
| let mut c = Command::new("cmd"); | ||
| c.arg("/c").arg(cmd).args(args); | ||
| c | ||
| } else { | ||
| let mut c = Command::new(cmd); | ||
| c.args(args); | ||
| c | ||
| }; |
There was a problem hiding this comment.
When cmd.exe /c is used to wrap the command on Windows, the Child handle returned by command.spawn() refers to the cmd.exe process, not the actual target process (e.g. pnpm). As a result, send_graceful_stop sends CTRL_BREAK_EVENT to cmd.exe's process group ID (the cmd.exe PID), not to the real subprocess's group. Because cmd.exe /c spawns the real process in its own context and does not forward CTRL_BREAK_EVENT, the graceful shutdown mechanism (lines 229–238) silently fails: only cmd.exe is signalled, and the real long-running process (e.g. a pnpm dev server) is left running or must wait for the 3-second timeout to be force-killed.
A more robust approach for Windows is to use where.exe or std::env::var("PATHEXT") to locate the actual .cmd/.bat file and pass its full path to Command::new, or to use the which crate to resolve the executable. Alternatively, if cmd.exe /c must be kept, the process group setup and graceful-stop logic need to account for the extra cmd.exe wrapper layer.
| // On Windows, non-.exe commands (e.g. .cmd/.bat scripts like pnpm) must be | ||
| // launched through cmd.exe because CreateProcessW only resolves .exe files. | ||
| let mut command = if cfg!(windows) { | ||
| let mut c = Command::new("cmd"); | ||
| c.arg("/c").arg(cmd).args(args); | ||
| c | ||
| } else { |
There was a problem hiding this comment.
When cmd.exe /c is used, individual arguments in the args slice are passed as separate arg() calls, but cmd.exe reassembles them into a single command line string using its own quoting rules. If any argument or the cmd value contains spaces, special characters (&, |, >, <, ^, %, !), or quotes, cmd.exe may misinterpret them, leading to incorrect behavior or potential argument injection. The current callers only pass simple strings like "start:server", so this is not an immediate problem, but it is a latent hazard as the function is public and could be called with arbitrary arguments in the future. Consider documenting this limitation or using std::os::windows::process::CommandExt::raw_arg to bypass cmd.exe's re-quoting.
| // On Windows, non-.exe commands (e.g. .cmd/.bat scripts like pnpm) must be | |
| // launched through cmd.exe because CreateProcessW only resolves .exe files. | |
| let mut command = if cfg!(windows) { | |
| let mut c = Command::new("cmd"); | |
| c.arg("/c").arg(cmd).args(args); | |
| c | |
| } else { | |
| // Spawn the requested command directly; on Windows we rely on PATH/PATHEXT | |
| // resolution so that shell wrapper scripts (e.g. .cmd/.bat) are located | |
| // without going through `cmd.exe`. | |
| let mut command = { |
|
@mohamedmansour I've opened a new pull request, #80, to work on those changes. Once the pull request is ready, I'll request review from you. |
On Windows, CreateProcessW cannot launch .cmd/.bat scripts directly. The previous approach wrapped all commands in cmd.exe /c, which broke graceful shutdown (CTRL_BREAK_EVENT targeted cmd.exe, not the real process) and introduced argument-injection risks. Changes: - Add build_command() helper in util.rs that uses the which crate to resolve executables on Windows: .cmd/.bat are wrapped in cmd.exe /c, while .exe files are launched directly. - Introduce ManagedChild wrapper around std::process::Child that holds a Windows Job Object with KILL_ON_JOB_CLOSE, ensuring the entire process tree (including cmd.exe children) is terminated on drop. - Apply build_command() to both spawn_child (process.rs) and run_command (util.rs) so all xtask command invocations resolve correctly on Windows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On Windows,
CreateProcessWonly resolves.exefiles, so non-exe commands like.cmd/.batscripts (e.g.pnpm) fail when spawned directly viaCommand::new.This change updates
spawn_childinxtask/src/process.rsto launch commands throughcmd.exe /con Windows, ensuring batch-script-based tools work correctly.Changes
xtask/src/process.rs: Wrap command invocation incmd /con Windows (cfg!(windows)), while keeping direct execution on other platforms.