Closed alexeagle closed 5 years ago
Interestingly if I spill to a params file, it contains
''
hello
Which seems backwards - a blank line would have been more useful to parse args from a file, while the empty quotes would have worked in the argv
Yes, this is definitely a bug.
C:\tmp>type print-argv.cc
#include <iostream>
int main(int argc, char** argv) {
std::cout << "argc=" << argc << std::endl;
for (int i = 0; i < argc; ++i) {
std::cout << "argv[" << i << "]=(" << argv[i] << ")" << std::endl;
}
return 0;
}
C:\tmp>print-argv.exe '' foo "" bar
argc=5
argv[0]=(print-argv.exe)
argv[1]=('')
argv[2]=(foo)
argv[3]=()
argv[4]=(bar)
...I should've added that the code is a demonstration of how empty args work on Windows.
Apparently ''
is taken literally but ""
means an empty string.
FYI here's an essay about cmd.exe quoting rules: https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
/cc @meteorcloudy
I'm not sure about the right priority, so I'm not assigning any.
I'm taking a look at this.
Thank you Yun!
@alexeagle I tried to use @laszlocsomor 's print-argv.cc
in a skylark rule, it seems the parameters are passed correctly.
See the following files:
execute.bzl:
def _impl(ctx):
# The list of arguments we pass to the script.
args = ctx.actions.args()
args.add([], join_with=",")
args.add("hello")
# Action to call the script.
ctx.actions.run(
inputs=[],
outputs=[ctx.outputs.out],
arguments=[args],
executable=ctx.executable._print_bin)
print_param = rule(
implementation=_impl,
attrs={
"out": attr.output(mandatory=False),
"_print_bin": attr.label(executable=True, cfg="host", allow_files=True,
default=Label("//actions_run:print"))
}
)
and BUILD:
load(":execute.bzl", "print_param")
print_param(
name = "run",
out = "output.txt",
)
cc_binary(
name = "print",
srcs = ["print.cc"],
)
The output:
SUBCOMMAND: # //actions_run:run [action 'SkylarkAction actions_run/output.txt']
cd C:/src/tmp/_bazel_pcloudy/8oryhygq/execroot/__main__
bazel-out/host/bin/actions_run/print.exe hello
INFO: From SkylarkAction actions_run/output.txt:
argc=3
argv[0]=(C:\src\tmp\_bazel_pcloudy\8oryhygq\execroot\__main__\bazel-out\host\bin\actions_run\print.exe)
argv[1]=()
argv[2]=(hello)
Bazel shows the command is "bazel-out/host/bin/actions_run/print.exe hello", but actually the empty argument is passed correctly because we invoke the binary through system API instead of running in terminal.
As I explained, this doesn't seem to be a bug in Bazel, so closing. Reopen if it's still a problem.
I can still reproduce this:
BUILD.bazel
load("@build_bazel_rules_nodejs//:defs.bzl", "nodejs_binary")
nodejs_binary(
name = "bin",
entry_point = "__main__/repro.js",
data = ["repro.js"],
)
load(":index.bzl", "my_rule")
my_rule(name = "my_rule", out="params.txt")
filegroup(name = "node_modules")
WORKSPACE
git_repository(
name = "build_bazel_rules_nodejs",
remote = "https://github.com/bazelbuild/rules_nodejs.git",
tag = "0.5.3")
load("@build_bazel_rules_nodejs//:defs.bzl", "node_repositories")
node_repositories(package_json = [])
index.bzl
def _my_rule(ctx):
args = ctx.actions.args()
args.add([], join_with=",")
args.add("hello")
ctx.actions.run(
inputs=[],
executable = ctx.executable._bin,
outputs = [ctx.outputs.out],
arguments = [args],
)
my_rule = rule(_my_rule,
attrs = {
"out": attr.output(mandatory=False),
"_bin": attr.label(default=Label("//:bin"),
executable=True, cfg="host"),
},
)
repro.js
console.error(process.argv);
bazel build :my_rule
DEBUG: C:/users/alexeagle/appdata/local/temp/_bazel_alexeagle/znpsit4n/external/bazel_tools/tools/cpp/lib_cc_configure.bzl:37:3:
Auto-Configuration Warning: 'BAZEL_VC' is not set, start looking for the latest Visual C++ installed.
DEBUG: C:/users/alexeagle/appdata/local/temp/_bazel_alexeagle/znpsit4n/external/bazel_tools/tools/cpp/lib_cc_configure.bzl:37:3:
Auto-Configuration Warning: Looking for VS%VERSION%COMNTOOLS environment variables,eg. VS140COMNTOOLS
DEBUG: C:/users/alexeagle/appdata/local/temp/_bazel_alexeagle/znpsit4n/external/bazel_tools/tools/cpp/lib_cc_configure.bzl:37:3:
Auto-Configuration Warning: Visual C++ build tools found at C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\
INFO: Analysed target //:my_rule (0 packages loaded).
INFO: Found 1 target...
INFO: From SkylarkAction params.txt:
WARNING: source-map-support module not installed.
Stack traces from languages like TypeScript will point to generated .js files.
[ 'C:\\users\\alexeagle\\appdata\\local\\temp\\_bazel_alexeagle\\znpsit4n\\external\\nodejs\\node.exe',
'__main__/repro.js',
'hello' ]
Maybe it's somehow related to nodejs specifically, but if I just run the program like this, I don't see the problem:
C:\users\alexeagle\appdata\local\temp\_bazel_alexeagle\znpsit4n\external\nodejs\node.exe repro.js '' 'hello'
[ 'C:\\users\\alexeagle\\appdata\\local\\temp\\_bazel_alexeagle\\znpsit4n\\external\\nodejs\\node.exe',
'C:\\Users\\alexeagle\\Projects\\repro_4778\\repro.js',
'\'\'',
'\'hello\'' ]
I debugged this issue again. And found, if I modify https://github.com/bazelbuild/bazel/blob/1f684e1b87cd8881a0a4b33e86ba66743e32d674/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsProcesses.java#L229-L232 to
if (arg.isEmpty()) {
result.append("\'\'");
continue;
}
The nodejs example will have the expected output:
[ 'C:\\users\\pcloudy\\_bazel_pcloudy\\4isq3p4t\\external\\nodejs\\node.exe',
'__main__/repro.js',
'',
'hello' ]
But the C++ example will output:
argc=3
argv[0]=(C:\users\pcloudy\_bazel_pcloudy\4isq3p4t\execroot\__main__\bazel-out\host\bin\print.exe)
argv[1]=('')
argv[2]=(hello)
The reason is different binaries have different command line parser. While the C++ binary treats ""
as an empty argument, the nodejs binary just ignores it.
There is a very interesting article about this: http://daviddeley.com/autohotkey/parameters/parameters.htm
Saddly, I don't think there is any good solution from Bazel side, because Bazel doesn't know how the command line will be parsed. :( @laszlocsomor @alexeagle
@meteorcloudy : thanks for looking into this issue!
What exact command line did Bazel pass to CreateProcessW
in these two cases? Is there a way to pass an empty argument to a C++ binary?
Before the change:
<binary> "" hello
after the change:
<binary> '' hello
The first one is able to pass an empty argument to a C++ binary, but not to a nodejs binary.
The second one works for nodejs binary, but the C++ binary will receive a ''
instead of an empty string.
Interesting, I also read https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/ which doesn't mention empty arguments but does agree that parsing the command line is up to the process.
I tried one more thing, same repro.js
as above but this time calling node directly:
C:\Users\alexeagle\Projects\repro_4778
λ node repro.js 1 2 3 ""
[ 'C:\\cmder\\bin\\node.exe',
'C:\\Users\\alexeagle\\Projects\\repro_4778\\repro.js',
'1',
'2',
'3',
'' ]
C:\Users\alexeagle\Projects\repro_4778
λ node repro.js 1 2 3 "" ''
[ 'C:\\cmder\\bin\\node.exe',
'C:\\Users\\alexeagle\\Projects\\repro_4778\\repro.js',
'1',
'2',
'3',
'',
'\'\'' ]
C:\Users\alexeagle\Projects\repro_4778
λ node -v
v8.11.1
As you can see, the double quotes do create an empty argument. So I guess the problem is related to how we spawn the nodejs process under Bazel.
@alexeagle You are absolutely correct! I finally found the bug. Because we launch the nodejs binary through a bash script, and the bash script is again launched by the native binary launcher created by Bazel. We didn't parse empty argument correctly in the native launcher, see: https://github.com/bazelbuild/bazel/blob/256c75adbaea966364077f6f342fa9351174e8b2/src/tools/launcher/util/launcher_util.cc#L131
After adding
if (argument.empty()) {
return L"\"\"";
}
The output is now correct:
$ bazel build //:my_rule
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
C:\Users\pcloudy/.bazelrc
DEBUG: C:/users/pcloudy/_bazel_pcloudy/4isq3p4t/external/build_bazel_rules_nodejs/internal/common/check_bazel_version.bzl:46:5:
Current Bazel is not a release version, cannot check for compatibility.
DEBUG: C:/users/pcloudy/_bazel_pcloudy/4isq3p4t/external/build_bazel_rules_nodejs/internal/common/check_bazel_version.bzl:48:5: Make sure that you are running at least Bazel 0.5.4.
INFO: Analysed target //:my_rule (0 packages loaded).
INFO: Found 1 target...
INFO: From SkylarkAction params.txt:
WARNING: source-map-support module not installed.
Stack traces from languages like TypeScript will point to generated .js files.
[ 'C:\\users\\pcloudy\\_bazel_pcloudy\\4isq3p4t\\external\\nodejs\\node.exe',
'__main__/repro.js',
'',
'hello' ]
I'll send a fix for this issue.
Awesome work again @meteorcloudy thank you!!
I add some empty args with an action, like this
On linux/mac, the program is run with
my_action '' hello
so I parse two arguments.On windows it's run with
my_action hello
I can make a minimal repro if that's useful