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

Yarn workspace incorrectly resolves node_modules path during script execution of workspace dependencies #4564

Open
labriola opened this issue Sep 27, 2017 · 23 comments
Assignees

Comments

@labriola
Copy link

labriola commented Sep 27, 2017

What is the current behavior?
When you install the main project, it correctly begins installing the workspace dependencies. However, if one of the scripts in a dependency attempts to execute a command, the location of the node_modules folder is incorrectly resolved.

As an example of the error:
\workspaceModuleBug*node_modules\node_modules*\shx\lib\cli.js

If the current behavior is a bug, please provide the steps to reproduce.
With yarn workspaces enabled, clone the following repository and do a yarn install.
https://github.com/labriola/workspaceModuleBug

Yarn will throw an error when attempting to do an shx echo.

What is the expected behavior?
The node_module path should recurse upward correctly instead of making (what I believe to be) an invalid assumption.

Please mention your node.js, yarn and operating system version.
node v.6.11.3
yarn 1.1.0
windows 10

Happy to attempt to resolve if I can get a hint on where to poke around.

@labriola
Copy link
Author

labriola commented Sep 28, 2017

Apparently this is windows only.

Long and short of it is that, when creating the output shim, we compute the path relative to the root. So in my example, we compute the relative path from 'test2' as a folder directly under the workspace to the node_module folder of the workspace.

from - ..\workspaceModuleBug\test2\node_modules\.bin
to - ..\workspaceModuleBug\node_modules\[package]\dist\index.js

..However, before executing the spawned process, we add the nested path (underneath the node_module\test2\node_module):

...\workspaceModuleBug\node_modules\test2\node_modules\.bin

So note, we wrote a CMD that understands the relative path between test2\node_module\.bin and the workspace's node_modules, but we add the installed module, node_modules\test2\node_modules to the path. So, when we execute the spawned child process, it fails because the path incorrectly resolves to:

...\workspaceModuleBug\node_modules\node_modules\[package]\dist\index.js

Note, the duplicate node_module. If I can get advice on the proper place to patch this, I will be happy to try but I am unsure if altering the path we create is a good idea. (or how to tell when I should) in the current code.

Also, since the same physical package installation will now be available via two different physical paths, I don't know we can fulfill both requirements with the current template of the CMD file as generated by cmd-shim. So it would either need to be more intelligent or would only work in one of the two circumstances.

@labriola
Copy link
Author

This line is where the potentially errant path is added:

pathParts.unshift(path.join(cwd, binFolder));

I am not saying this is necessarily where the fix should go but I am hoping it provides a quick way for someone to review and let me know a valid way to proceed.

@BYK BYK self-assigned this Sep 29, 2017
@labriola
Copy link
Author

Wondering if anyone had further thoughts on this?

@darkalor
Copy link

Still an issue in yarn 1.7.0. Have not found any workaround yet.

@BYK is this still on your list? It seems like a rather big blocker for using workspaces.

@pronebird
Copy link

pronebird commented Aug 20, 2018

We have the same issue with yarn 1.9.4, node_modules twice and scripts in node_modules/.bin cannot be executed.

@pronebird
Copy link

pronebird commented Aug 21, 2018

This seems to be Windows only problem, so I wonder if junctions are somehow not properly handled which causes node_modules being added twice to the bin path?

error C:\Users\pronebird\mullvad\mullvadvpn-app\gui\node_modules\@mullvad\components: Command failed.
Exit code: 1
Command: yarn run build
Arguments:
Directory: C:\Users\pronebird\mullvad\mullvadvpn-app\gui\node_modules\@mullvad\components
Output:
yarn run v1.9.4
$ run-s private:build:clean private:build:compile
module.js:549
    throw err;
    ^

Error: Cannot find module 'C:\Users\pronebird\mullvad\mullvadvpn-app\gui\node_modules\node_modules\npm-run-all\bin\run-s\index.js'
    at Function.Module._resolveFilename (module.js:547:15)
    at Function.Module._load (module.js:474:25)
    at Function.Module.runMain (module.js:693:10)
    at startup (bootstrap_node.js:188:16)

Update

I was able to fix this by adding a preinstall script to one of workspace packages that linked node_modules\node_modules to node_modules. Ugly but it works. I wonder why this is not being addressed for almost a year. Seems like a critical bug.

@pronebird
Copy link

I was able to fix this by adding a preinstall script that linked node_modules\node_modules to node_modules to one of packages in workspace. Ugly but it works. I wonder why this is not being addressed for almost a year. Seems like a critical bug.

@marcelolx
Copy link

I'm facing same issue.

vince-fugnitto added a commit to eclipse-theia/theia-cpp-extensions that referenced this issue Sep 20, 2019
- updated the `postinstall` scripts for both `cortext-debug` and `cpp-debug`.
- updated the scripts as a workaround to a known yarn issue on windows
(yarnpkg/yarn#4564)

Signed-off-by: vince-fugnitto <[email protected]>
vince-fugnitto added a commit to eclipse-theia/theia-cpp-extensions that referenced this issue Sep 20, 2019
- updated the `postinstall` scripts for both `cortext-debug` and `cpp-debug`.
- updated the scripts as a workaround to a known yarn issue on windows
(yarnpkg/yarn#4564)

Signed-off-by: vince-fugnitto <[email protected]>
vince-fugnitto added a commit to eclipse-theia/theia-cpp-extensions that referenced this issue Sep 20, 2019
- updated the `postinstall` scripts for both `cortext-debug` and `cpp-debug`.
- updated the scripts as a workaround to a known yarn issue on windows
(yarnpkg/yarn#4564)

Signed-off-by: vince-fugnitto <[email protected]>
vince-fugnitto added a commit to eclipse-theia/theia-cpp-extensions that referenced this issue Sep 20, 2019
- updated the `postinstall` scripts for both `cortext-debug` and `cpp-debug`.
- updated the scripts as a workaround to a known yarn issue on windows
(yarnpkg/yarn#4564)

Signed-off-by: vince-fugnitto <[email protected]>
vince-fugnitto added a commit to eclipse-theia/theia-cpp-extensions that referenced this issue Sep 23, 2019
- updated the `postinstall` scripts for both `cortext-debug` and `cpp-debug`.
- updated the scripts as a workaround to a known yarn issue on windows
(yarnpkg/yarn#4564)

Signed-off-by: vince-fugnitto <[email protected]>
@Heliks
Copy link

Heliks commented Mar 3, 2020

Issue still exists in 1.22

@FMGordillo
Copy link

Can confirm that still happens on version 2.4.0 :c

@nwittwer
Copy link

nwittwer commented Apr 10, 2021

Seeing this as well inside a Yarn Workspace.
OS: Windows, Yarn v1.22.5, Node v14.15.3.

error C:\Users\hi\Documents\...\node_modules\@myapp\app: Command failed.
Exit code: 1
Command: electron-builder install-app-deps
Arguments:
Directory: C:\Users\hi\Documents\...\node_modules\@myapp\app
Output:
internal/modules/cjs/loader.js:883
  throw err;
  ^

Error: Cannot find module 'C:\Users\hi\Documents\...\node_modules\node_modules\electron-builder\out\cli\cli.js'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:880:15)
    at Function.Module._load (internal/modules/cjs/loader.js:725:27)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47 {

@CGQAQ
Copy link

CGQAQ commented Aug 21, 2021

why this critical issue is not been solved, and even not a single contributor commented on this???

@drktfl
Copy link

drktfl commented Aug 24, 2022

Not a solution, but in some cases another workaround could be to prevent yarn from hoisting the required dependencies:

I.e., in the failing packages (not in the root package.json!):

"workspaces": {
  "nohoist": [
    "rimraf"
  ]
}

This will make sure that rimraf is resolved in the package-local node_modules.

But I would appreciate a fix in yarn, too :-)

@rbuckton
Copy link

rbuckton commented Sep 5, 2022

I'm running into this as well and tried the "nohoist" workaround. Unfortunately, "nohoist" is not allowed in public packages in yarn, so it would need to be added to the root package.json if the failing package is a public package.

@cruhl
Copy link

cruhl commented Nov 22, 2022

Just ran into this with a co-worker on windows.

@KAJdev
Copy link

KAJdev commented Nov 25, 2022

Also running into this. 1.22.5

@lsarrazi
Copy link

lsarrazi commented Apr 6, 2023

Still a problem in 2023 ...

@slyduda
Copy link

slyduda commented Oct 22, 2023

just ran into this issue on windows as well

@m0rtalis
Copy link

Is there a fix for this? Having the same issue

@ehsankhfr
Copy link

Still the same problem in 2024...

@nonara
Copy link

nonara commented Jul 6, 2024

Quick update on what's happening here.

The failure is happening when running the generated cmd files.

Detail

Here's an example:

Say we have

- /package.json
- /packages/a
- /packages/b

Now assume our packages are scoped as: @pkg/a @pkg/b

/packages/b/node_modules/.bin/rimraf.cmd

@IF EXIST "%~dp0\node.exe" (
  "%~dp0\node.exe"  "%~dp0\..\..\..\..\node_modules\prisma\build\index.js" %*
  
  ; Added this line
  echo "%~dp0\..\..\..\..\node_modules\prisma\build\index.js"
) ELSE (
  @SETLOCAL
  @SET PATHEXT=%PATHEXT:;.JS;=;%

  ; Added this line
  echo "%~dp0\..\..\..\..\node_modules\prisma\build\index.js"

  node  "%~dp0\..\..\..\..\node_modules\prisma\build\index.js" %*
)

When it gets run from the symlink path (eg. C:\project\node_modules\@pkg\a\node_modules\.bin) instead of the resolved (real) path (eg. C:\project\packages\a\node_modules\.bin), it will resolve incorrectly, because the files are written assuming we're relative to the repo root.

For example, this is what the paths would look like:

bin file path:

C:\project\node_modules\pkg\a\node_modules\.bin\\..\..\..\..\node_modules\prisma\build\index.js

resolved:

C:\project\node_modules\\node_modules\prisma\build\index.js

Why it Happens

The issue is due to the fact that yarn is adding the symlink path instead of the real path to the PATH env.

@labriola was correct in it originating here:

pathParts.unshift(path.join(cwd, binFolder));

This gets added as the top-most search location. Using the symlink path instead of the fully resolved real path will cause the relative paths to be broken.

Solutions

Solution 1 : Modify execute-lifecycle-script.js

This would be the most ideal IMO, assuming there are no side-effects.

A simple change is to edit makeEnv to the following:

  // Add node_modules .bin folders to the PATH
  for (const registryFolder of config.registryFolders) {
    const binFolder = path.join(registryFolder, '.bin');
    if (config.workspacesEnabled && config.workspaceRootFolder) {
      pathParts.unshift(path.join(config.workspaceRootFolder, binFolder));
    }
    pathParts.unshift(path.join(config.linkFolder, binFolder));

    // We resolve the path here before adding to PATH
    const realBinFolder = await fs.realpath(path.join(cwd, binFolder));
    pathParts.unshift(realBinFolder);
  }

Solution 2 : Modify cmd files

We can resolve symlinks in the generated command files.

Here is an example:

@echo off
SETLOCAL
FOR /f "tokens=*" %%i IN ('fsutil reparsepoint query "%~dp0" ^| find "Print Name"') DO SET RealPath=%%i
SET RealPath=%RealPath:*Print Name: =%

@IF EXIST "%~dp0\node.exe" (
  "%~dp0\node.exe"  "%RealPath%\..\..\..\node_modules\rimraf\dist\esm\bin.mjs" %*
  echo "%RealPath%\..\..\..\node_modules\rimraf\dist\esm\bin.mjs"
) ELSE (
  node  "%RealPath%\..\..\..\node_modules\rimraf\dist\esm\bin.mjs" %*
  echo "%RealPath%\..\..\..\node_modules\rimraf\dist\esm\bin.mjs"
)
ENDLOCAL

We'd want to ensure that this works with Win XP and up, but I believe it will.

If not, we can also do something more simple, like using cd to the path.

Questions for the Yarn Team

Questions I'd have for the Yarn team are:

  1. Solution 1
    a. Would it be better to just use realpath at a higher level?
    b. Is there any potential downside to modifying it here?

Also: I see that some patches are being accepted for Yarn 1 which are not strictly security related. Notably, I tried this on Yarn 3 and had the same issue.

I'd love to get this merged, and if you require, I'll do an update Yarn 3 as well on whatever route we determine.

PR

@nonara
Copy link

nonara commented Jul 6, 2024

PR is submitted. Hopefully I can get in touch with a team member so we can get it through.

Here's a release you all can use for now:

@nonara/[email protected]

I'll keep the latest tag pointing to the most recent

Looks like it's working here. Would love to hear feedback if you try it out.

nonara added a commit to nonara/yarn that referenced this issue Jul 6, 2024
@nonara
Copy link

nonara commented Jul 11, 2024

Workaround & Update

I'm holding on completing the PR for now to see if I get a response. My published version works in the case of lifecycle scripts, but there are certain other issues where it plays a factor.

It may make more sense to hit this in the generated .cmd files. In any case, I'll wait to see if the team responds.

In the mean time, I made an npx tool we can use that will correct the .cmd files by making them resolve and use the realpath, where possible / necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests