Yarn run doesn't find bin scripts when using Plug and Play on Windows

[edmorley]

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

What is the current behavior?
yarn run fails to find packages' bin scripts when using plug and play mode on Windows, instead giving:

C:\Users\Ed\src\testcase-yarn-pnp-windows>yarn lint
yarn run v1.12.0
warning package.json: No license field
$ eslint --version
'eslint' is not recognized as an internal or external command,
operable program or batch file.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

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

1. Create an empty directory containing this package.json:

{
  "installConfig": {
    "pnp": true
  },
  "dependencies": {
    "eslint": "^5.6.1"
  },
  "scripts": {
    "lint": "eslint --version"
  }
}

2. yarn --pnp
3. yarn lint

What is the expected behavior?
ESLint is found and outputs its version.

Please mention your node.js, yarn and operating system version.
Yarn 1.12.0, Node 10.11.0, Windows 10 x64

Other:

• This reproduced for me inside both an MSYS2 bash shell and when using native cmd.exe
• The same error occurred using something other than eslint (eg jest)
• If instead of running yarn lint, I call the package directly (ie not a scripts command), I get a more informative error message:

yarn eslint --version
yarn run v1.12.0
warning package.json: No license field
$ C:\Users\Ed\AppData\Local\Yarn\Cache\v3\npm-eslint-5.6.1-348134e32ccc09abb2df1bf282b3f6eed8c7b480\node_modules\eslint\.bin\eslint --version
'C:\Users\Ed\AppData\Local\Yarn\Cache\v3\npm-eslint-5.6.1-348134e32ccc09abb2df1bf282b3f6eed8c7b480\node_modules\eslint\.bin\eslint'
is not recognized as an internal or external command, operable program or batch file.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

• Inspecting the contents of the .bin directory in that error message, shows eslint to be a junction (so related to Yarn on Windows creates invalid junctions to files (junctions are not allowed to files, only directories) #6450? Though that issue doesn't appear to be about PnP):

01/10/2018  17:11    <JUNCTION>     eslint [C:\Users\Ed\AppData\Local\Yarn\Cache\v3\npm-eslint-5.6.1-348134e32ccc09abb2df1bf282b3f6eed8c7b480\node_modules\eslint\bin\eslint.js]

• Trying to open the junction in Windows Explorer gives:

C:\Users\Ed\AppData\Local\Yarn\Cache\v3\npm-eslint-5.6.1-348134e32ccc09abb2df...\eslint
is not accessible.

The directory name is invalid.

• MSYS2 reports it as a symlink to a non-existent directory rather than a file (note the trailing slash):

lrwxrwxrwx 1 Ed None 132 Oct  1 17:11 eslint -> /c/Users/Ed/AppData/Local/Yarn/Cache/v3/npm-eslint-5.6.1-348134e32ccc09abb2df1bf282b3f6eed8c7b480/node_modules/eslint/bin/eslint.js/

[jdalton]

I can also reproduce the bin file fail.

Note: Just to narrow the scope I confirmed basic functionality of yarn add lodash then yarn node -p "require('lodash').VERSION" does work with pnp on Windows.

Update:

The map path in the .pnp.js file seems OKAY too (though not really cross-platform portable)

"../../Users/jdalton/AppData/Local/Yarn/Cache/v3/npm-eslint-5.6.1-348134e32ccc09abb2df1bf282b3f6eed8c7b480/node_modules/eslint/"

Update:

I tried running with

"installConfig": {
    "pnp": false
  },

and yarn lint works so it's something with pnp support.

Update:

I made it to getGlobalPrefix() with binFolders of

[ 'C:\\Users\\jdalton\\AppData\\Local\\Yarn\\bin',
  'C:\\Users\\jdalton\\.yarn\\bin' ]

[arcanis]

Yep, this seems definitely related to #6450. It's going to be annoying, we'll likely have to bump the cache version to fix it (only on Windows, fortunately).

The faulty logic is likely there:

yarn/src/util/fs.js

Line 705 in e905f74

await fsSymlink(src, dest, 'junction');

Which is called here:

yarn/src/fetchers/base-fetcher.js

Line 80 in e905f74

await fs.symlink(src, `${binDest}/${binName}`);

It's interesting: while this code has changed lately, it always used to create junction points on Windows. Was it already broken before, or is it a recent regression?

[edmorley]

I'm presuming the issue is that fs.symlink() is used in these places for a file:

yarn/src/util/generate-pnp-map.js

Line 290 in 1b78b1a

await fs.symlink(symlinkSource, symlinkFile);

yarn/src/fetchers/base-fetcher.js

Line 80 in 1b78b1a

await fs.symlink(src, `${binDest}/${binName}`);

When the implementation uses junction, which is only suitable for a directory:

yarn/src/util/fs.js

Lines 703 to 705 in e905f74

	if (process.platform === 'win32') {
	// use directory junctions if possible on win32, this requires absolute paths
	await fsSymlink(src, dest, 'junction');

Compare to this case, which on Windows uses cmdShim() instead:

yarn/src/package-linker.js

Lines 34 to 45 in 1b78b1a

	if (process.platform === 'win32') {
	const unlockMutex = await lockMutex(src);
	try {
	await cmdShim(src, dest);
	} finally {
	unlockMutex();
	}
	} else {
	await fs.mkdirp(path.dirname(dest));
	await fs.symlink(src, dest);
	await fs.chmod(dest, '755');
	}

[arcanis]

The symlink call in generate-pnp-map is valid (we generate symlinks to the cache in order to deambiguate peer dependencies while still keeping valid filesystem paths). The one in base-fetcher should use a similar logic to package-linker, though.

And now I remember: it's not a regression because we weren't creating the .bin folder in the cache previously. The code in base-fetcher was only added starting from PnP, to replace the linker that doesn't run when PnP is enabled 🙂

[edmorley]

I tried replacing the base-fetcher implementation linked above with a call to package-linker's linkBin() which uses cmd-shim, which meant the run command got as far as starting eslint, however there were then resolution errors, since it was using the native node resolver, due to the lack of -r .pnp.js in the added scripts:

.bin/eslint

#!/bin/sh
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")

case `uname` in
    *CYGWIN*) basedir=`cygpath -w "$basedir"`;;
esac

if [ -x "$basedir/node" ]; then
  "$basedir/node"  "$basedir/../bin/eslint.js" "$@"
  ret=$?
else 
  node  "$basedir/../bin/eslint.js" "$@"
  ret=$?
fi
exit $ret

.bin/eslint.cmd

@IF EXIST "%~dp0\node.exe" (
  "%~dp0\node.exe"  "%~dp0\..\bin\eslint.js" %*
) ELSE (
  @SETLOCAL
  @SET PATHEXT=%PATHEXT:;.JS;=;%
  node  "%~dp0\..\bin\eslint.js" %*
)

cmd-shim does support an (undocumented) args option, which would allow passing in the -r .pnp.js, however that would mean the global yarn cache would only work for pnp projects, which is a non-starter.

It seems like execute-lifecycle-script needs to instead invoke the node executable directly on the target of the package bin entry, so it can control the args? Either that, or the -r .pnp.js be set via NODE_OPTIONS (sadly only supported on Node 8+) when invoking the child process?

[arcanis]

however that would mean the global yarn cache would only work for pnp projects, which is a non-starter

I'm not sure that's true - we should check, but the .bin folder within the cache should be ignored for regular installs (because we create the symlinks in the node_modules/.bin folders).

[arcanis]

That said, the args from cmdshim likely wouldn't work, since the path to the PnP file cannot be deduced when we generate the cache entries (especially since there might be multiple such files).

Btw, we have a makePortableProxyScript function quite similar to cmdshim that allows adding such arguments (I added it when working on PnP because I was missing the same feature).

[arcanis]

Now what I don't understand is that the generated scripts should work even without the --require flag being injected. What happens when you run yarn run foo is this:

• Yarn creates a temporary directory
• Yarn writes in this temporary directory an indirection script for Node
  • This indirection script is meant to enforce all child processes to use the same Node than Yarn
  • We also use it to automatically inject the -r flag to any child process, if needed (cf here)
• Yarn injects the temporary directory into the PATH, then spawn the process
• It should then reach the <cache>/pkg/.bin/foo.cmd
• Which should then call the Yarn wrapper for node
• Which should then call the real Node with the -r injected

So the last step seems to be failing ... curious ...

[edmorley]

Adding set to the top of eslint.cmd to dump the environment variables, Path looks to have been updated as expected:

Path=C:\Users\Ed\AppData\Local\Temp\yarn--1538746708420-0.6016197465247681;
C:\Users\Ed\AppData\Local\Yarn\Cache\v4\npm-webpack-dev-server-3.1.9-8b32167624d2faff40dcedc2cbce17ed1f34d3e0\node_modules\webpack-dev-server/.bin;
...

Inspecting that directory (is it expected that the temporary directory isn't cleaned up after the lifecycle script ends?), shows:

yarn--NNNNN.../node

#!/bin/sh

"C:\Program Files\nodejs\node.exe" "-r" "C:\Users\Ed\src\test-react-neutrino9-beta0/.pnp.js" "$@"

yarn--NNNNN.../yarn

#!/bin/sh

"C:\Program Files\nodejs\node.exe" "C:\Program Files (x86)\Yarn\bin\yarn.js" "$@"

However the eslint.cmd shim (see example in previous comment) refers to node.exe rather than node, so presumably these don't shadow it? (And even if they did, cmd.exe wouldn't understand the shell scripts)

That said, I very rarely use cmd and instead use MSYS2 (checking out WSL is still on my "when I have time or get a laptop refresh" list), so perhaps I can avoid this for now locally by just overriding the shell using script-shell?

[edmorley]

However the eslint.cmd shim (see example in previous comment) refers to node.exe rather than node, so presumably these don't shadow it? (And even if they did, cmd.exe wouldn't understand the shell scripts)

Correction, it only refers to .exe in the case that's not relevant here.

Adding where node to eslint.cmd, I see:

C:\Users\Ed\AppData\Local\Temp\yarn--1538747958596-0.8625861460047743\node
C:\Program Files\nodejs\node.exe

But as expected, running the former gives:

'C:\Users\Ed\AppData\Local\Temp\yarn--1538747958596-0.8625861460047743\node'
is not recognized as an internal or external command, operable program or batch file.

(I guess the cmd.exe's Path traversal must silently ignore that error and move onto the global node binary? I don't use the Windows command line enough to be able to say)

[edmorley]

Repeating the STR in the issue description with Yarn 1.12.3 (ie with the fix in #6621), I now get:

$ yarn lint
yarn run v1.12.3
warning package.json: No license field
$ eslint --version
C:\Users\Ed\src\testcase-yarn-pnp\.pnp.js:2126
    throw firstError;
    ^

Error: You cannot require a package ("C:\Users\Ed\AppData\Local\Yarn\Cache\v4\npm-eslint-5.8.0-91fbf24f6e0471e8fdf681a4d9dd1b2c9f28309b\node_modules\eslint\bin\eslint.js") that is not declared in your dependencies (via "C:\Users\Ed\src\testcase-yarn-pnp/")
    at makeError (C:\Users\Ed\src\testcase-yarn-pnp\.pnp.js:55:17)
    at Object.resolveToUnqualified (C:\Users\Ed\src\testcase-yarn-pnp\.pnp.js:1859:17)
    at Object.resolveRequest (C:\Users\Ed\src\testcase-yarn-pnp\.pnp.js:1937:31)
    at Function.Module._resolveFilename (C:\Users\Ed\src\testcase-yarn-pnp\.pnp.js:2117:30)
    at Function.Module._load (C:\Users\Ed\src\testcase-yarn-pnp\.pnp.js:2035:31)
    at Function.Module.runMain (internal/modules/cjs/loader.js:760:12)
    at startup (internal/bootstrap/node.js:303:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:872:3)
error Command failed with exit code 1.

...even though eslint is in dependencies.

Edit: Though perhaps sounds like #6557?

[arcanis]

I've just merged #6495 which includes various fixes for Windows. I think it'll land with the 1.13 (~2-3 weeks), but in the meantime it will soon be available through our nightly builds.

[edmorley]

The STR in comment 0 now succeed :-)

$ ../yarn-1.13.0-20181114.2216.js --pnp
yarn install v1.13.0-20181114.2216
...
[1/5] Resolving packages...
[2/5] Fetching packages...
[3/5] Linking dependencies...
[5/5] Building fresh packages...
Done in 9.38s.

$ ../yarn-1.13.0-20181114.2216.js lint
yarn run v1.13.0-20181114.2216
...
$ eslint --version
v5.9.0
Done in 0.70s.

However trying against an existing project I've hit several more issues that I think mean it won't be possible for us to use it for the project I had in mind sadly (non platform portable paths in .pnp.js, symlinks being created in the .pnp directory due to externals, which breaks virtualbox shared folders with a Windows host ((we used to use `--no-bin-links to work around this))). For other projects it should work great though :-)

[edmorley]

(I'll open some new issues for the other items)