Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: fix positional args in task runner #52810

Merged
merged 7 commits into from May 8, 2024

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented May 2, 2024

The proposed change correctly splits the positional arguments.

Fixes: #52740

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 2, 2024
@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig force-pushed the fix-positional-args branch 2 times, most recently from cf0ccb9 to cf8fef9 Compare May 2, 2024 22:56
@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 2, 2024
@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented May 3, 2024

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

@anonrig
Copy link
Member Author

anonrig commented May 3, 2024

@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?

@anonrig
Copy link
Member Author

anonrig commented May 3, 2024

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.

@lemire
Copy link
Member

lemire commented May 3, 2024

@anonrig Windows programming is a very ancient art passed on from generation to generation.

Looking...

@anonrig
Copy link
Member Author

anonrig commented May 3, 2024

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

@anonrig
Copy link
Member Author

anonrig commented May 3, 2024

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

@richardlau
Copy link
Member

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 #52682 (comment) / npm/cli#7375.

@anonrig
Copy link
Member Author

anonrig commented May 3, 2024

@richardlau It seems ditching -- for positional arguments might reduce the friction. What do you think?

@lemire
Copy link
Member

lemire commented May 3, 2024

@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?

@anonrig
Copy link
Member Author

anonrig commented May 3, 2024

@lemire EscapeShell function adds quotes around any argument that has space. But somehow that quotation marks are removed on Windows.

@lemire
Copy link
Member

lemire commented May 3, 2024

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?

@lemire
Copy link
Member

lemire commented May 3, 2024

So... if you do

job.bat 'a b'

The there are two parameters...

'a

and

b'

Are we agreed?

@lemire
Copy link
Member

lemire commented May 3, 2024

You get back...

'--help "hello world test"' A B C

which is...

'--help 
"hello world test"' 
A 
B 
C

@anonrig
Copy link
Member Author

anonrig commented May 3, 2024

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.

@lemire
Copy link
Member

lemire commented May 3, 2024

@anonrig I'll issue a PR against your PR.

@lemire
Copy link
Member

lemire commented May 3, 2024

@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

richardlau commented May 3, 2024

@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 #52810 (comment) is a npm bug specific to using Powershell and the Powershell npm script to run npm.

@anonrig
Copy link
Member Author

anonrig commented May 4, 2024

@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'
  }

@lemire
Copy link
Member

lemire commented May 4, 2024

@anonrig It says that there are six arguments and prints --help "hello world test"" A B C.

src/node_task_runner.cc Show resolved Hide resolved
src/node_task_runner.cc Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented May 6, 2024

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\"\""

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2024
@nodejs-github-bot nodejs-github-bot merged commit fe4e569 into nodejs:main May 8, 2024
53 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in fe4e569

targos pushed a commit that referenced this pull request May 11, 2024
PR-URL: #52810
Fixes: #52740
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node --run doesn't split additional params
7 participants