nodejs
src: fix positional args in task runner
The proposed change correctly splits the positional arguments.
Fixes: https://github.com/nodejs/node/issues/52740
Windows CI is failing:
not ok 5 - appends positional arguments
---
duration_ms: 70.9008
location: 'c:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\parallel\\test-node-run.js:57:3'
failureType: 'testCodeFailure'
error: |-
The input did not match the regular expression /Arguments: '--help "hello world test" A B C'/. Input:
`Arguments: ''--help "hello world test"' A B C'\r\n` +
'The total number of arguments are: 5\r\n'
code: 'ERR_ASSERTION'
name: 'AssertionError'
expected:
actual: |-
Arguments: ''--help "hello world test"' A B C'
The total number of arguments are: 5
operator: 'match'
stack: |-
TestContext.<anonymous> (c:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-node-run.js:63:12)
process.processTicksAndRejections (node:internal/process/task_queues:95:5)
async Test.run (node:internal/test_runner/test:748:9)
async Suite.processPendingSubtests (node:internal/test_runner/test:461:7)
...
1..5
@nodejs/platform-windows @lemire I couldn't find the reason for the failing test case. It seems the first argument receives an unnecessary quote before after it which causes the issue fail only on Windows. Any idea why it's happening and how to avoid it?
On macOS, running the following command produces the same exact thing:
test/fixtures/run-script on fix-positional-args via ⬢ v22.0.0
❯ npm run positional-args -- '--help "hello world test"' A B C
> positional-args
> positional-args --help "hello world test" A B C
Arguments: '--help "hello world test" A B C'
The total number of arguments are: 4
But on Windows, the ' characters for each argument is preserved.
@anonrig Windows programming is a very ancient art passed on from generation to generation.
Looking...
It produces something entirely different on Windows... I don't understand.
PS C:\Users\yagiz\Desktop\coding\node\test\fixtures\run-script> npm run positional-args -- '--help "hello world test"' A B C
> positional-args
> positional-args world test A B C
Arguments: 'world test A B C'
The total number of arguments are: 5
On Windows this branch produces the following output:
PS C:\Users\yagiz\Desktop\coding\node\test\fixtures\run-script> ..\..\..\out\Release\node --run positional-args -- '--help "hello world test"' A B C
ExperimentalWarning: Task runner is an experimental feature and might change at any time
Arguments: ''--help hello' world test A B C'
The total number of arguments are: 7
It produces something entirely different on Windows... I don't understand.
PS C:\Users\yagiz\Desktop\coding\node\test\fixtures\run-script> npm run positional-args -- '--help "hello world test"' A B C > positional-args > positional-args world test A B C Arguments: 'world test A B C' The total number of arguments are: 5
Probably related to https://github.com/nodejs/node/issues/52682#issuecomment-2087140628 / https://github.com/npm/cli/issues/7375.
@richardlau It seems ditching -- for positional arguments might reduce the friction. What do you think?
@anonrig I am trying to understand your test in the first place.
Normally, --help and "hello world text" would be two arguments.
If you are considering them as just one, then it is an argument with a space.
So the first argument contains a space, correct?
So maybe it is "C:\my document\file.txt". Right? It is the same idea. That is, a file with a space.
So you want to call the script with the first argument being "C:\my document\file.txt". Correct?
Now if you end up with
myscript.bat C:\my document\file.txt
Then that's clearly wrong.
You need quotes or something else around it.
Are we agreed?
@lemire EscapeShell function adds quotes around any argument that has space. But somehow that quotation marks are removed on Windows.
EscapeShell function adds quotes around any argument that has space. But somehow that quotation marks are removed on Windows.
Under Windows, are single quotes the correct approach? Do you have a reference?
So... if you do
job.bat 'a b'
The there are two parameters...
'a
and
b'
Are we agreed?
You get back...
'--help "hello world test"' A B C
which is...
'--help
"hello world test"'
A
B
C
Under Windows, are single quotes the correct approach? Do you have a reference?
@lemire I don't have any. Maybe it is wrong. I was following the original npm implementation, which is also flawed I guess.
The reference is here: https://daviddeley.com/autohotkey/parameters/parameters.htm
@richardlau It seems ditching
--for positional arguments might reduce the friction. What do you think?
No opinion. My point was that your quoted behaviour in https://github.com/nodejs/node/pull/52810#issuecomment-2093631567 is a npm bug specific to using Powershell and the Powershell npm script to run npm.
@lemire I fixed 2 issues, but there is still an unnecessary " inside the output which I didn't understand...
✖ failing tests:
test at test\parallel\test-node-run.js:57:3
✖ appends positional arguments (45.513ms)
AssertionError [ERR_ASSERTION]: The input did not match the regular expression /Arguments: '--help "hello world test" A B C'/. Input:
`Arguments: '--help "hello world test"" A B C'\r\n` +
'The total number of arguments are: 6\r\n'
at TestContext.<anonymous> (C:\Users\yagiz\Desktop\coding\node\test\parallel\test-node-run.js:63:12)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async Test.run (node:internal/test_runner/test:748:9)
at async Suite.processPendingSubtests (node:internal/test_runner/test:461:7) {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: `Arguments: '--help "hello world test"" A B C'\r\nThe total number of arguments are: 6\r\n`,
expected: /Arguments: '--help "hello world test" A B C'/,
operator: 'match'
}
@anonrig It says that there are six arguments and prints --help "hello world test"" A B C.
C:\workspace\node-compile-windows\node\test\cctest\test_node_task_runner.cc:41
Expected equality of these values:
node::task_runner::EscapeShell(input)
Which is: "\"\"\"$1\"\"\""
expected
Which is: "\"\"$1\"\""
