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

[BUG] npm on windows can't run scripts with ".", relative paths "../" #2880

Closed
DavidHenri008 opened this issue Mar 16, 2021 · 12 comments
Closed
Labels
Bug thing that needs fixing Release 7.x work is associated with a specific npm 7 release

Comments

@DavidHenri008
Copy link

Current Behavior:

On windows npm cannot run scripts like

"build": "../node_modules/.bin/tsc",
Error:
'..' is not recognized as an internal or external command,
operable program or batch file.

Replacing with ..\ makes it work.

Expected Behavior:

The string should be converted to make it cross-platform.
As a note, it does work with Yarn.

Steps To Reproduce:

Place in script task something with "../" or "./" on WINDOWS.

Environment:

npm: '7.6.2',
node: '14.4.0',

@DavidHenri008 DavidHenri008 added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Mar 16, 2021
@LukasWillin
Copy link

Can confirm: Replacing with backslash fixes the issue:

Steps To Reproduce:

A command like this run in windows powershell.
"browserify": "./node_modules/.bin/browserify app/js/app.js -o build/dist/js/bundle.min.js --fast true --detect-globals false",

Gives error

'.' is not recognized as an internal or external command,
operable program or batch file.
npm ERR! code 1

Working-Example:

"browserify": ".\\node_modules\\.bin\\browserify app/js/app.js -o build/dist/js/bundle.min.js --fast true --detect-globals false",

Environment:

npm: 7.6.3
node: v11.0.0
windows: 10.0.19042 Build 19042

@ljharb
Copy link
Contributor

ljharb commented Mar 17, 2021

In that example, you don't need to hardcode node_modules at all - you can do "browserify": "browserify … directly.

Hardcoding node_modules should be rare.

@ehoogeveen-medweb
Copy link

IIUC npm uses cmd.exe as its default shell to execute scripts on Windows, which has this limitation. Another workaround is to surround the path in quotes, e.g. "build": "\"../node_modules/.bin/tsc\"". Is that enough to solve your problem? As mentioned e.g. in #2699 npm is unlikely to switch to a different default shell in v7.

@DavidHenri008
Copy link
Author

Surrounding with quotes didn't work for me when I tried.

The fact that this is supported by yarn may be due to a different shell being used?

@ehoogeveen-medweb
Copy link

Hmm, it works in plain cmd.exe but I didn't try through npm, sorry. I guess npm does some processing on it which might be getting in the way, like with #2731 (the fix for that issue was merged into run-script but npm v7.6.3 doesn't have it yet).

@LukasWillin
Copy link

LukasWillin commented Mar 18, 2021

@ljharb

In that example, you don't need to hardcode node_modules at all - you can do "browserify": "browserify … directly.

Hardcoding node_modules should be rare.

You are right about the use case but that is not the point. I can also reproduce the error when trying to run a cmd script from npm. And they did work on npm version 6.4.1

@nlf
Copy link
Contributor

nlf commented Apr 7, 2021

this is a difficult one because your expected behavior statement "the string should be converted to make it cross platform" means that we would have to parse bash scripts and port them to cmd scripts and vice versa.

this is an absolutely massive undertaking filled with a huge number of edge cases and gotchas. some versions of npm 7 attempted a form of this, but caused a lot of failures so we removed that work in favor of just running the string as-is. for most use cases this will do the right thing and is far more intuitive to write and debug.

as @ljharb mentioned, you should be able to simply run browserify without the leading path since we add the .bin directory to your PATH environment variable anyway. for running other scripts, unfortunately it's pretty much up to you to write them in a cross platform way.

it is, however, strange that running it in a command prompt directly works while running it through an npm script does not. i'll take a look and see if i can reproduce that and what might be happening.

@ehoogeveen-medweb
Copy link

ehoogeveen-medweb commented Apr 7, 2021

Interestingly, it works if I wrap the entire command in yet another set of quotes.

If the following works from a cmd terminal: "../test/test.cmd" "Hello world!" (where test.cmd just does echo %~1)
Then the following script does not work: "test": "\"../test/test.cmd\" \"Hello world!\""
But this does work: "test": "\" \"../test/test.cmd\" \"Hello world!\" \"" (spaces added for readability, it works either way)

@nlf
Copy link
Contributor

nlf commented Apr 7, 2021

is that on the latest npm? 7.8.0?

@nlf
Copy link
Contributor

nlf commented Apr 7, 2021

ok, so to break this down. if you have a script defined like:

{ "scripts": { "test": "./node_modules/.bin/foo" } }

what we run is cmd.exe /d /s /c ./node_modules/.bin/foo

if you copy that to a running shell and run it, it fails with the '.' is not recognized as an internal or external command error, which is why you're seeing that with npm.

if you wrap it in quotes once, like

{ "scripts": { "test": "\"./node_modules/.bin/foo\"" } }

then we now run cmd.exe /d /s /c "./node_modules/.bin/foo" which fails with the same error, this is because cmd.exe deliberately removes them on your behalf. you can see this documentation if you run cmd.exe /?

1.  If all of the following conditions are met, then quote characters
        on the command line are preserved:

        - no /S switch
        - exactly two quote characters
        - no special characters between the two quote characters,
          where special is one of: &<>()@^|
        - there are one or more whitespace characters between the
          two quote characters
        - the string between the two quote characters is the name
          of an executable file.

    2.  Otherwise, old behavior is to see if the first character is
        a quote character and if so, strip the leading character and
        remove the last quote character on the command line, preserving
        any text after the last quote character.

since we do pass /s we use the old behavior, which removes the first and last quote characters on the line, effectively making this command the same as without quotes at all.

putting a second pair of quotes gets around this because the inner quotes will be left alone while the outer quotes are removed. the problem lies in the handling of arguments. we can't just wrap the whole string in two pairs of quotes, because then the shell will think that the entire string is a command, which will give you more not recognized as a command errors.

theoretically, we would have to parse the string and look for the first space and wrap everything up to that in a pair of quotes, then append the arguments to that and wrap the whole thing in another pair of quotes. the tricky part of that is the numerous ways that a space could exist and still be part of the first parameter (i.e. the "command"), it could be wrapped in quotes, it could be a literal space, it could be a \ escaped space, or it could be a windows style ^ escaped space. in addition to that, multiple commands can exist in the same script, and can be chained in a number of different ways. ;, &, && and || just to name a few. and each of those can be escaped.

the challenges and edge cases here are why we opted to simply run the string we're given and not attempt conversion. as an added gotcha, shell builtins don't work if you quote them. i.e. "cd" foo will give the same '"cd"' is not recognized error.

@darcyclarke darcyclarke removed the Needs Triage needs review for next steps label May 14, 2021
@darcyclarke
Copy link
Contributor

Closing.

@SubliemeSiem
Copy link

Why is this closed? This worked in older npm versions and is still broken now in 8.1.1. I can't imagine anyone is happy with the double quoting "solution" as suggested, since it makes npm scripts more unreadable and is a lot more error-prone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

No branches or pull requests

7 participants