Can't pass args containing backslashes or spaces to nodejs_binary on Windows

[gmishkin]

🐞 bug report

Affected Rule

The issue is caused by the rule: nodejs_binary

Is this a regression?

No

Description

You can't pass arguments containing backslashes to a nodejs_binary on Windows because of the way the batch file wrapper calls the bash wrapper: https://github.com/bazelbuild/rules_nodejs/blob/c82b43dd9bfd0dcf1c708821dc48898961221a12/internal/common/windows_utils.bzl#L99

This appears to pass the expanded arguments literally with no necessary escaping for bash should some arguments contain embedded backslashes (or spaces).

🔬 Minimal Reproduction

https://github.com/gmishkin/rules_nodejs_batch_args just prints the first argument

should be abc\def

PS C:\Users\geoff\code\rules_nodejs_batch_args> bazel run :main -- abc\def                                                                                           
abcdef

should be abc def

PS C:\Users\geoff\code\rules_nodejs_batch_args> bazel run :main -- "abc def"                                                                                         
abc

🌍 Your Environment

Operating System:

  
Windows 10 version 1909
  

Output of bazel version:

  
Build label: 2.0.0
Build target: bazel-out/x64_windows-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Thu Dec 19 12:31:16 2019 (1576758676)
Build timestamp: 1576758676
Build timestamp as int: 1576758676
  

Rules_nodejs version:

(Please check that you have matching versions between WORKSPACE file and @bazel/* npm packages.)

  
1.3.0
  

Anything else relevant?

Here's my ugly hack that doesn't quite work right.

set quotedargs=
:top
if "%1"=="" goto end
set quotedargs=%quotedargs% '%1'
shift
goto top
:end
"C:/msys64/usr/bin/bash.exe" -c "!run_script! %quotedargs%"

[alexeagle]

Yeah I'm not surprised. Handling argv on Windows is a constant source of bugs :(
/cc @meteorcloudy

[meteorcloudy]

set args=%*
set args=%args:\=\\\\%
set args=%args:"=\"%
"{bash_bin}" -c "!run_script! %args%"

Escaping \ and " could make the following work:

bazel run :main -- "abc def" abc
=> abc def

bazel run :main -- abc\\def abc
=> abc\def

bazel run :main -- "abc\\def" abc
=> abc\def

bazel run :main -- 'abc\def' abc
=> abc\def

Is this a acceptable solution?

[gmishkin]

@meteorcloudy for me or @alexeagle ?

[alexeagle]

I think @meteorcloudy is asking if you could change the way you invoke bazel as a workaround

[gmishkin]

In my case it's a genrule passing $@ so I don't think so.

I guess I could have an intermediate script do OS detection, wrap it in a printf "%q" and hope for the best?

[meteorcloudy]

Sorry for not making this clear.
I mean we can fix windows_utils.bzl by escaping \ and " for %* before putting it into a double quote here:
https://github.com/bazelbuild/rules_nodejs/blob/c82b43dd9bfd0dcf1c708821dc48898961221a12/internal/common/windows_utils.bzl#L99

This will make the result "more" correct and consistent with Linux:
On Linux, you also have:

$ bazel run //:main -- abc\def hij
abcdef
$ bazel run //:main -- abc\\def hij
abc\def

I cannot assure all command line args are passed as people expect, this article answers why it's so hard to get it right:
http://daviddeley.com/autohotkey/parameters/parameters.htm

[meteorcloudy]

Here is the PR #1685

[meteorcloudy]

I guess the use case is to make file paths get passed correctly?
With #1685, we can achieve this;
On Windows:

pcloudy@pcloudy1-w MSYS /c/src/projects/rules_nodejs_batch_args
$ bazel run //:main -- C:\foo\bar C:/foo/bar "C:/foo bar" C:\\foo\\bar  "C:\foo\bar" "C:\\foo\\bar" 'C:\foo\bar' 'C:\\foo\\bar' 'C:\foo bar'
WARNING: Option 'ui' is deprecated
INFO: Analyzed target //:main (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //:main up-to-date:
  bazel-bin/main.sh
  bazel-bin/main_loader.js
  bazel-bin/main_require_patch.js
  bazel-bin/main.bat
INFO: Elapsed time: 0.434s, Critical Path: 0.01s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
INFO: Build completed successfully, 1 total action
[
  'C:\\src\\tmp\\kktrcjfq\\external\\nodejs_windows_amd64\\bin\\nodejs\\node.exe',
  'info_amsa_rules_nodejs_batch_args/index.js',
  'C:foobar',
  'C:/foo/bar',
  'C:/foo bar',
  'C:\\foo\\bar',
  'C:\\foo\\bar',
  'C:\\foo\\bar',
  'C:\\foo\\bar',
  'C:\\\\foo\\\\bar',
  'C:\\foo bar'
]

Compared to linux result, they are the same:

pcloudy@pcloudy:~/workspace/rules_nodejs_batch_args
$ bazel run //:main -- C:\foo\bar C:/foo/bar "C:/foo bar" C:\\foo\\bar  "C:\foo\bar" "C:\\foo\\bar" 'C:\foo\bar' 'C:\\foo\\bar' 'C:\foo bar'
INFO: Analyzed target //:main (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //:main up-to-date:
  bazel-bin/main.sh
  bazel-bin/main_loader.js
  bazel-bin/main_require_patch.js
INFO: Elapsed time: 0.122s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
INFO: Build completed successfully, 1 total action
[
  '/usr/local/google/home/pcloudy/.cache/bazel/_bazel_pcloudy/3b856f93ab24767e9c6eddccebe41015/external/nodejs_linux_amd64/bin/nodejs/bin/node',
  'info_amsa_rules_nodejs_batch_args/index.js',
  'C:foobar',
  'C:/foo/bar',
  'C:/foo bar',
  'C:\\foo\\bar',
  'C:\\foo\\bar',
  'C:\\foo\\bar',
  'C:\\foo\\bar',
  'C:\\\\foo\\\\bar',
  'C:\\foo bar'
]

[gmishkin]

I can confirm this is fixed in 1.4.1. Thank you!