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

"'node' is not recognized as an internal or external command" when using Yarn PnP on Windows #6683

Closed
edmorley opened this issue Nov 15, 2018 · 13 comments
Assignees
Labels
fixed-in-modern This issue has been fixed / implemented in Yarn 2+. plugnplay triaged

Comments

@edmorley
Copy link
Contributor

edmorley commented Nov 15, 2018

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
When using Plug and Play on Windows, the postinstall command of the package fetch-mock fails to run, when certain other packages are listed in dependencies - however runs fine when those other unrelated dependencies are removed (?!).

$ ./yarn-1.13.0-20181114.2216.js
yarn install v1.13.0-20181114.2216
warning package.json: No license field
info No lockfile found.
warning No license field
[1/5] Resolving packages...
[2/5] Fetching packages...
info fsevents@1.2.4: The platform "win32" is incompatible with this module.
info "fsevents@1.2.4" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/5] Linking dependencies...
warning " > react2angular@4.0.4" has unmet peer dependency "@types/angular@>=1.5".
warning " > react2angular@4.0.4" has unmet peer dependency "@types/prop-types@>=15".
warning " > react2angular@4.0.4" has unmet peer dependency "@types/react@>=16".
warning " > react2angular@4.0.4" has unmet peer dependency "@types/react-dom@>=16".
[5/5] Building fresh packages...
error C:\Users\Ed\src\yarn-pnp-test\.pnp\unplugged\npm-fetch-mock-7.2.5-4682f51b9fa74d790e10a471066cb22f3ff84d48\node_modules\fetch-mock: Command failed.
Exit code: 1
Command: node scripts/support-fetch-mock.js
Arguments:
Directory: C:\Users\Ed\src\yarn-pnp-test\.pnp\unplugged\npm-fetch-mock-7.2.5-4682f51b9fa74d790e10a471066cb22f3ff84d48\node_modules\fetch-mock
Output:
'node' is not recognized as an internal or external command,
operable program or batch file.

If the current behavior is a bug, please provide the steps to reproduce.

  1. mkdir test && cd test
  2. curl https://nightly.yarnpkg.com/yarn-1.13.0-20181114.2216.js -O
  3. Create a package.json with the following contents:
{
  "dependencies": {
    "@uirouter/angularjs": "0.4.3",
    "ajv": "6.5.5",
    "angular": "1.7.5",
    "angular-clipboard": "1.6.2",
    "angular-local-storage": "0.7.1",
    "angular1-ui-bootstrap4": "2.4.22",
    "auth0-js": "9.8.2",
    "bootstrap": "4.1.3",
    "d3": "5.7.0",
    "font-awesome": "4.7.0",
    "history": "4.7.2",
    "jquery": "3.3.1",
    "jquery.flot": "0.8.3",
    "jquery.scrollto": "2.1.2",
    "js-cookie": "2.2.0",
    "js-yaml": "3.12.0",
    "json-e": "2.7.1",
    "json-schema-defaults": "0.4.0",
    "lodash": "4.17.11",
    "metrics-graphics": "2.15.6",
    "moment": "2.22.2",
    "mousetrap": "1.6.2",
    "ng-text-truncate-2": "1.0.1",
    "numeral": "2.0.6",
    "popper.js": "1.14.5",
    "prop-types": "15.6.2",
    "query-string": "6.2.0",
    "react": "16.6.3",
    "react-day-picker": "7.2.4",
    "react-dom": "16.6.3",
    "react-fontawesome": "1.6.1",
    "react-highlight-words": "0.14.0",
    "react-hot-loader": "4.3.12",
    "react-hotkeys": "1.1.4",
    "react-lazylog": "3.1.4",
    "react-linkify": "0.2.2",
    "react-redux": "5.1.1",
    "react-router-dom": "4.3.1",
    "react-select": "2.1.1",
    "react-split-pane": "0.1.84",
    "react-table": "6.8.6",
    "react-tabs": "2.3.0",
    "react2angular": "4.0.4",
    "reactstrap": "6.5.0",
    "redux": "4.0.1",
    "redux-debounce": "1.0.1",
    "redux-thunk": "2.3.0",
    "taskcluster-client-web": "8.1.0",
    "taskcluster-lib-scopes": "10.0.1",
    "webpack": "4.25.1",
    "webpack-cli": "3.1.2"
  },
  "devDependencies": {
    "fetch-mock": "7.2.5"
  },
  "installConfig": {
    "pnp": true
  }
}
  1. ./yarn-1.13.0-20181114.2216.js

What is the expected behavior?
That the yarn install succeed when using plug and play mode.

I've reduced the size of the testcase package.json a fair amount - however removing any more of the dependencies above resulted in the issue no longer occurring. I'm presuming there's a race condition or other oddity occurring?

Please mention your node.js, yarn and operating system version.

  • Windows 10
  • Node.js 11.0.0
  • Yarn Nightly v1.13.0-20181114.2216

cc @arcanis

@ghost ghost assigned rally25rs Nov 15, 2018
@ghost ghost added the triaged label Nov 15, 2018
@edmorley
Copy link
Contributor Author

edmorley commented Nov 15, 2018

The --verbose output gave the following stack:

'node' is not recognized as an internal or external command,
operable program or batch file.
    at ChildProcess.proc.on (C:\Users\Ed\src\yarn-1.13.0-20181114.2216.js:25288:15)
    at ChildProcess.emit (events.js:182:13)
    at maybeClose (internal/child_process.js:962:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:251:5)

Annotating Yarn's spawn wrapper showed that program, args and env were virtually identical between the working (ie some packages removed from dependencies) and not working cases, except in the broken case env.PATH contained additional entries.

The size of stringified env in a working case was 16530 characters, and 16689 in the broken case.
For env.PATH, the stringified lengths were 8740 and 8899 characters respectively.

According to this page, the maximum size of PATH on windows when used by the command prompt, is 8192 characters:
https://support.microsoft.com/en-gb/help/830473/command-prompt-cmd-exe-command-line-string-limitation

...which seems to line up with the findings above (once taking into account the additional characters added by JSON.stringify()).

There are also some other limits discussed here:
https://superuser.com/questions/1070272/why-does-windows-have-a-limit-on-environment-variables-at-all

ie: The additional directories on PATH push the original OS PATH entries past the point that gets truncated, such that node then cannot be found.

Do all of those package directories need to be on PATH? I see entries that don't have binaries, that perhaps shouldn't be there? eg:
C:\\Users\\Ed\\AppData\\Local\\Yarn\\Cache\\v4\\npm-react-16.6.3-25d77c91911d6bbdd23db41e70fb094cc1e0871c\\node_modules\\react/.bin

@edmorley
Copy link
Contributor Author

edmorley commented Nov 15, 2018

Do all of those package directories need to be on PATH? I see entries that don't have binaries, that perhaps shouldn't be there?

These appear to come from here:

for (const [name, reference] of packageInformation.packageDependencies.entries()) {
const dependencyInformation = pnpApi.getPackageInformation({name, reference});
if (!dependencyInformation || !dependencyInformation.packageLocation) {
continue;
}
pathParts.unshift(`${dependencyInformation.packageLocation}/.bin`);
}

One possible approach would be to check for the existence of a .bin directory within each package on the filesystem at lifecycle script runtime (to determine which should be added to PATH) - however that would presumably be slow. Perhaps the .pnp.js metadata could instead be updated to track which packages have declared binaries, that way allowing makeEnv() to filter packageInformationStores directly?

@arcanis
Copy link
Member

arcanis commented Nov 16, 2018

Oh wow, thanks for the debug! Completely forgot about the PATH size limits 😞

I think on the short term I'll do the existsSync call, but long term I'm considering deprecating the "binaries-exposed-through-the-PATH behavior" in favor of a single run utility that would have pretty much the same behavior than yarn run or npm run, but in a much more portable fashion:

{
  "scripts": {
    "foo": "run babel --preset=env [...]"
  }
}

@hWorblehat
Copy link
Contributor

Hi @arcanis,

Any movement on this? I'm hitting it. Would you be opposed to me submitting a PR for checking fot the existence of a .bin subdirectory, at least as a temporary fix?

I don't see an easy way around the resulting performnce hit (although it didn't seem that bad to me when I tested). Maybe we could only perform the checking if we're on Windows?

@hWorblehat
Copy link
Contributor

Incidentally, I like your idea of having a run utility that would bootstrap Node binaries. I do forsee a problem though:

Like you said, we would have to deprecate the old behaviour, rather than remove it, to avoid breaking existing projects. That means this bug would still exist - adding the run command doesn't fix anything, as the PATH will still get corrupted on Windows for being too long.

Not sure what the alternative is...

@arcanis
Copy link
Member

arcanis commented Nov 21, 2018

Any movement on this? I'm hitting it. Would you be opposed to me submitting a PR for checking fot the existence of a .bin subdirectory, at least as a temporary fix?

Absolutely not, please do! I think the perf impact should be negligible - this only affects scripts, which are supposed to be very few. It should be fine to do it on every platform for now.

Do you think you can open the PR today? Otherwise I can give it a look as well.

That means this bug would still exist - adding the run command doesn't fix anything, as the PATH will still get corrupted on Windows for being too long.

Yep, the run command would be a related improvement, not a fix in itself. The best fix right now is to check for the .bin existence, as you mentioned.

@hWorblehat
Copy link
Contributor

Do you think you can open the PR today? Otherwise I can give it a look as well.

Yep - working on it now 😄 Just testing the changes...

@hWorblehat
Copy link
Contributor

Yep, the run command would be a related improvement, not a fix in itself.

I must be missing something then - what benefit would the run command provide?

@arcanis
Copy link
Member

arcanis commented Nov 21, 2018

It would normalize how scripts and binaries are called across package managers (less so for binaries because for now the PATH injection works fine, but it might cause issues in the future when we implement zip loading - we won't be able to put those zip paths into the PATH).

One particular example is this:

{
  "scripts": {
    "build": "npm run foo && npm run bar",
    "build:foo": "do-something foo",
    "build:bar": "do-something bar"
  }
}

In the example above, Yarn will not be used to run build:foo and build:bar even if you use yarn build (because the npm binary is hardcoded into the script). That can potentially cause problems since it's possible that npm won't have all the knowledge required to correctly execute those scripts.

Similarly, if you replace yarn by npm in those scripts, then yarn will always be used even if you use npm run build, which is obviously not what you intended to.

Having a run indirection allows all package managers to implement it however they want, as long as they agree on the general CLI interface. Scripts would then become package-manager-agnostic.

@hWorblehat
Copy link
Contributor

Ah - I get it now. Makes sense.

@hWorblehat
Copy link
Contributor

I've raised PR #6711 to 'fix' this. As mentioned in the PR, it really just makes this issue less likely to occur, rather than avoiding it entirely.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jan 17, 2019
## 1.13.0

- Implements a new `package.json` field: `peerDependenciesMeta`

  [6671](yarnpkg/yarn#6671) - [**Maël Nison**](https://twitter.com/arcanis)

- Adds an `optional` settings to `peerDependenciesMeta` to silence missing peer dependency warnings

  [6671](yarnpkg/yarn#6671) - [**Maël Nison**](https://twitter.com/arcanis)

- Implements `yarn policies set-version [range]`. Check [the documentation]() for usage & tips.

  [6673](yarnpkg/yarn#6673) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes a resolution issue when a package had an invalid `main` entry

  [6682](yarnpkg/yarn#6682) - [**Maël Nison**](https://twitter.com/arcanis)

- Decreases the size of the generated `$PATH` environment variable for a better Windows support

  [6683](yarnpkg/yarn#6683) - [**Rowan Lonsdale**](https://github.com/hWorblehat)

- Fixes postinstall scripts for third-party packages when they were referencing a binary from their own dependencies

  [6712](yarnpkg/yarn#6712) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes yarn audit exit code overflow

  [6748](yarnpkg/yarn#6748) - [**Andrey Vetlugin**](https://github.com/antrew)

- Stops automatically unplugging packages with postinstall script when running under `--ignore-scripts`

  [6820](yarnpkg/yarn#6820) - [**Maël Nison**](https://twitter.com/arcanis)

- Adds transparent support for the [`resolve`](https://github.com/browserify/resolve) package when using Plug'n'Play

  [6816](yarnpkg/yarn#6816) - [**Maël Nison**](https://twitter.com/arcanis)

- Properly reports the error codes when the npm registry throws 500's

  [6817](yarnpkg/yarn#6817) - [**Maël Nison**](https://twitter.com/arcanis)
@merceyz
Copy link
Member

merceyz commented Jan 2, 2021

Closing as fixed in V2

https://yarnpkg.com/getting-started/migration

@merceyz merceyz closed this as completed Jan 2, 2021
@paul-soporan paul-soporan added the fixed-in-modern This issue has been fixed / implemented in Yarn 2+. label Jan 2, 2021
@sahar-Nosrati
Copy link

I have the same problem. Even I set the path of node.js. However, it did not work. what can I do? Please guide me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed-in-modern This issue has been fixed / implemented in Yarn 2+. plugnplay triaged
Projects
None yet
Development

No branches or pull requests

7 participants