Closed edmorley closed 3 years ago
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
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: https://github.com/yarnpkg/yarn/blob/7f419103307da7856627c4ca078460234701eb75/src/util/execute-lifecycle-script.js#L213-L221
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?
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 [...]"
}
}
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?
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...
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 thePATH
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.
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...
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?
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.
Ah - I get it now. Makes sense.
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.
Closing as fixed in V2
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.
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 (?!).If the current behavior is a bug, please provide the steps to reproduce.
mkdir test && cd test
curl https://nightly.yarnpkg.com/yarn-1.13.0-20181114.2216.js -O
package.json
with the following contents:./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.
cc @arcanis