std::Command::new fails for some CMD scripts on Windows 10 msvc nightly 1.19/1.20

[budziq]

While working on mdbook build.rs issue I've discovered that on nightly the std::Command::new() for a windows 10 batch script "npm.cmd" results in underlying node-js process to not find required resource paths.

I tried this code:

use std::process::Command;

fn main() {
    let status = Command::new("npm.cmd")
        .arg("-v")
        .output();

    println!("Command exited with '{:?}'", status);
    let status = status.expect("Process spawn should succeed!");
    if !status.status.success() {
        panic!("Command failed!");
    }
}

I expected to see this happen:
This works on all channels for my linux and windows 7 boxes (did not test other than msvc here) as well as stable and beta for windows 10 box (and appveyor)

Command exited with 'Ok(Output { status: ExitStatus(ExitStatus(0)), stdout: "3.10.10\n", stderr: "" })'

Instead, this happened only on windows 10 (and appveyor) msvc nightly:

Command exited with 'Ok(Output { status: ExitStatus(ExitStatus(1)), stdout: "", stderr: "module.js:471\r\n    throw err;\r\n    ^\r\n\r\nError: Cannot find module \'C:\\projects\\nightly-cmd-fail\\node_modules\\npm\\bin\\npm-cli.js\'\r\n    at Function.Module._resolveFilename (module.js:469:15)\r\n    at Function.Module._load (module.js:417:25)\r\n    at Module.runMain (module.js:604:10)\r\n    at run (bootstrap_node.js:389:7)\r\n    at startup (bootstrap_node.js:149:9)\r\n    at bootstrap_node.js:504:3\r\nmodule.js:471\r\n    throw err;\r\n    ^\r\n\r\nError: Cannot find module \'C:\\projects\\nightly-cmd-fail\\node_modules\\npm\\bin\\npm-cli.js\'\r\n    at Function.Module._resolveFilename (module.js:469:15)\r\n    at Function.Module._load (module.js:417:25)\r\n    at Module.runMain (module.js:604:10)\r\n    at run (bootstrap_node.js:389:7)\r\n    at startup (bootstrap_node.js:149:9)\r\n    at bootstrap_node.js:504:3\r\n" })'
thread 'main' panicked at 'Command failed!', main.rs:11
note: Run with `RUST_BACKTRACE=1` for a backtrace.

Please note that the panic is explicitly triggered in main.rs to make appveyor report the error.

The npm.cmd sets paths for node-js via environmental variables and these seem not to be forwarded properly.

Sorry for mostly useless description as I have no experience with npm/node and no access to the actual win10 box on which the problem was reproduced (I was only able to locate this problem via appveor logs).

Meta

rustc --version --verbose
rustc 1.20.0-nightly (445077963 2017-06-20)
binary: rustc
commit-hash: 445077963c55297ef1e196a3525723090fe80b22
commit-date: 2017-06-20
host: x86_64-pc-windows-msvc
release: 1.20.0-nightly
LLVM version: 4.0

But the problem exists at least since (sorry no verbose output here. I have no access to that win10 box anymore)
rustc 1.19.0-nightly (04145943a 2017-06-19)

Link to appveyor showing the problem:
https://ci.appveyor.com/project/budziq/nightly-cmd-fail/build/1.0.10

[retep998]

So you're saying the combination of Rust nightly and either appveyor or Windows 10 is causing the npm stuff to fail?

Also, the actual error that is occurring here:

module.js:471
    throw err;
    ^

Error: Cannot find module \'C:\\projects\\nightly-cmd-fail\\node_modules\\npm\\bin\\npm-cli.js\'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:389:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:504:3
module.js:471
    throw err;
    ^

Error: Cannot find module \'C:\\projects\\nightly-cmd-fail\\node_modules\\npm\\bin\\npm-cli.js\'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:389:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:504:3

[budziq]

So you're saying the combination of Rust nightly and either appveyor or Windows 10 is causing the npm stuff to fail?

What I was able to gather sofar is that env variables in the cmd script might not be forwarded correctly to the nodejs subprocess.

[alexcrichton]

@budziq what makes you think this is a rustc bug? It's looking like the process was spawned correctly and output was captured, which makes me think that this may be a local configuration issue?

[budziq]

@alexcrichton I am not positive that it is a rustc bug. But identical setup works on windows7 (stable/beta/nightly) as well as the exact same setup works for windows 10 stable/beta and fails on nightly suggests that this might be a regression in stdlib.

I am assuming that while process start and output capture works, there might be problem with how the env variables are passed to the child process. I'm no expert on windows nor nodejs (I just happened to help with triage on mdBook issue ) I am assuming that very specific contents of npm.cmd trigger this behavior. I might try to get a hold of win10 box again and try to get the reproduction path down to a manageable minimum.

[ollie27]

After some investigating it turns out this is an interesting Windows feature/quirk/bug in how %~dp0 is handled. We're seeing this now in Rust because of #42436 (so my bad). %~dp0 is supposed to return the drive and path to the running batch file but that can fail if it was called with a quoted name.

This isn't a new issue: https://stackoverflow.com/a/26851883.

Note that this can still be triggered on stable if the name of the batch file contains a space such as Command::new("foo bar.bat") so reverting #42436 won't really help.

What I suggest doing in this case is using Command::new("cmd").arg("/c").arg("npm.cmd") instead which seems to work around the issue. It's also what you should be doing according to the CreateProcess docs.

[Mark-Simulacrum]

Marking as a regression from stable to beta, which I think is now true? Uncertain. Either way, I suspect we should consider this a compatibility note and close, since based on @ollie27's comment directly above people using this functionality should be doing so differently according to the Windows documentation.

[azerupi]

This is an annoying regression though. Because instead of having to cfg only the name of the binary you now have to cfg all the places where you create / run the command because you need different args etc.

We used the method below in our build.rs, but in the actual code the Command part is repeated a couple of times in different places. So this regression would result in more cfg boilerplate.

#[cfg(windows)]
mod execs {
    pub const NPM: &'static str = "npm.cmd";
    pub const STYLUS: &'static str = "stylus.cmd";
}
#[cfg(not(windows))]
mod execs {
    pub const NPM: &'static str = "npm";
    pub const STYLUS: &'static str = "stylus";
}

...

if !Command::new(execs::STYLUS)
        .arg(stylus_dir)
        .arg("--out")
        .arg(theme_dir)
        .arg("--use")
        .arg("nib")
        .status()?
        .success() {
    bail!("Stylus encoutered an error");
}

[budziq]

Either way, I suspect we should consider this a compatibility note and close, since

I would opt for a fix instead of closing with compatibility note based on the fact that problem is focused on windows 10 and nightly (not sure if it has already crawled into beta) with the original (stable) behaviour being desirable and this regression possibly affecting large number of crates but being hard to catch with cargo bomb (i suppose that not many crates run windows cmd scripts in their regression tests)

based on @ollie27's comment directly above people using this functionality should be doing so differently according to the Windows documentation.

You are essentially suggesting that crates / binaries implement a workaround. IMHO this is what stdlib is for (providing a portable abstraction) and client code should not be interested (or even aware) of the fine details of underlying implementation.

[ollie27]

@azerupi something like the following should work and as I mentioned should probably be done anyway.

#[cfg(windows)]
mod execs {
    use std::process::Command;
    pub fn npm() -> Command {
        let mut cmd = Command::new("cmd");
        cmd.arg("/c").arg("npm.cmd");
        cmd
    }
    pub fn stylus() -> Command {
        let mut cmd = Command::new("cmd");
        cmd.arg("/c").arg("stylus.cmd");
        cmd
    }
}
#[cfg(not(windows))]
mod execs {
    use std::process::Command;
    pub fn npm() -> Command {
        Command::new("npm")
    }
    pub fn stylus() -> Command {
        Command::new("stylus")
    }
}

the fact that problem is focused on windows 10

Is it? I haven't been able to reproduce any differences between Window 10 64 bit and Windows 7 32 bit in a VM. The Stack Overflow answer I linked talks about this in Windows XP so if there really is a difference between 7 and 10 then there might be a different bug.

You are essentially suggesting that crates / binaries implement a workaround.

I believe the best place for the workaround is in the batch files themselves. Using a subroutine (like shown here) for %~dp0 seems to be the only reliable thing to do.

IMHO this is what stdlib is for (providing a portable abstraction) and client code should not be interested (or even aware) of the fine details of underlying implementation.

std doesn't provide a portable way to run batch files. If you use Command::new("foo.bat") then you have to specify the file extension anyway so using cmd /c isn't really any less portable. Client code should not rely on the fine details of the underlying implementation either, in this case whether the executable name is quoted.

Command is a bit of a mess on Windows. In this case it's definitely the fault of Windows. #37519 is Rust's fault though.

It wouldn't be a disaster to revert #42436 and return to the previous behaviour which just has different bugs.