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

fix(run): add cwd/node_modules/.bin to run command search path #7151

Merged
merged 5 commits into from
Oct 30, 2019

Conversation

rally25rs
Copy link
Contributor

Summary

This fixes a bug that was introduced in #6850 where the bin path was being built only from
config.lockfileFolder. However in workspaces, bins may not be hoisted to the workspace root,
causing bins to not be found. This change adds config.cwd to the bin search path, so the yarn run command will look in a workspace package's node_modules, as well as the workspace root.

fixes #7126

Test plan

Changed as existing test that placed a bin at both the workspace root and in a package. Split into 2 tests, one with the bin only at the workspace root, the other with the bin only in the package. This way we are testing that we can detect the bin in either location.

…nds. Fixes run in workspaces.

This fixes a bug that was introduced in yarnpkg#6850 where the bin path was being built only from
`config.lockfileFolder`. However in workspaces, bins may not be hoisted to the workspace root,
causing bins to not be found. This change adds `config.cwd` to the bin search path, so the `yarn
run` command will look in a workspace package's node_modules, as well as the workspace root.

fixes yarnpkg#7126
@buildsize
Copy link

buildsize bot commented Mar 27, 2019

File name Previous Size New Size Change
yarn-[version].noarch.rpm 1.11 MB 1.11 MB -1.62 KB (0%)
yarn-[version].js 4.48 MB 4.47 MB -1.76 KB (0%)
yarn-legacy-[version].js 4.67 MB 4.67 MB -1.79 KB (0%)
yarn-v[version].tar.gz 1.12 MB 1.12 MB -563 bytes (0%)
yarn_[version]all.deb 816.66 KB 816.49 KB -176 bytes (0%)

@GraceAredel GraceAredel mentioned this pull request Apr 5, 2019
@farooqu
Copy link

farooqu commented Aug 6, 2019

Can this be prioritized please? It is totally broken right now for workspaces mono repos.

@Amy-Lynn
Copy link

Amy-Lynn commented Oct 4, 2019

Can this please get prioritized/merged? This is a huge issue in mono-repos

@jasonmobley
Copy link

@rally25rs can you update this PR to resolve the conflicts?
@arcanis can you review? This fixes a regression introduced by #6850 which you reviewed

In my project, I'm currently pinned on Yarn 1.14 because of the regression caused by #6850. I'd love to get back to current if we could get this PR through. The change seems ready to go if we got the conflicts fixed and could get review on it. Thanks!

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks legit, thanks @rally25rs! Merge at will.

@rally25rs
Copy link
Contributor Author

I'll resolve it this evening and get this merged. Thanks @arcanis and @jasonmobley

@rally25rs
Copy link
Contributor Author

@arcanis the "yarn acceptance tests" had 1 failing test, only on node 8, however those tests pass locally on node 8 for me. Is there a way to re-run that test and see if it was just a hiccup in the CI test? I didn't see a "rerun" button anywhere for that test suite.

@arcanis
Copy link
Member

arcanis commented Oct 30, 2019

For some reason I cannot log into Azure, it makes an infinite loop of signin in and immediately signing out ... 🤷‍♀️ The timeout doesn't look critical to me, let's merge. I'll make a patch release later this week.

@arcanis arcanis merged commit 79a96e3 into yarnpkg:master Oct 30, 2019
@simonbuchan
Copy link

simonbuchan commented Oct 30, 2019

How does this work with cd /a/packages/b/c && yarn --cwd d some-command? I would expect it to try /a/packages/b/node_modules/.bin/some-command then /a/node_modules/.bin/some-command in /a/packages/b/c/d, assuming /a/packages/b is a package in a workspace /a.

@rally25rs rally25rs deleted the fix-workspace-bin-path-7126 branch October 31, 2019 12:35
@rally25rs
Copy link
Contributor Author

cwd is added to the PATH before the lockfile directory (the workspace root) so I believe it will use a bin from the cwd over one from the workspace.

@simonbuchan
Copy link

Not my question. To me it looks like the current code will look in /a/packages/b/c/d/node_modules/.bin or /a/packages/b/c/node_modules/.bin first (depending on when --cwd is applied), since cwd is often not the package directory.

arcanis pushed a commit that referenced this pull request Nov 22, 2019
* fix(run): change run command to check cwd/node_modules/.bin for commands. Fixes run in workspaces.

This fixes a bug that was introduced in #6850 where the bin path was being built only from
`config.lockfileFolder`. However in workspaces, bins may not be hoisted to the workspace root,
causing bins to not be found. This change adds `config.cwd` to the bin search path, so the `yarn
run` command will look in a workspace package's node_modules, as well as the workspace root.

fixes #7126

* modify chagelog
VincentBailly pushed a commit to VincentBailly/yarn that referenced this pull request Jun 10, 2020
…kg#7151)

* fix(run): change run command to check cwd/node_modules/.bin for commands. Fixes run in workspaces.

This fixes a bug that was introduced in yarnpkg#6850 where the bin path was being built only from
`config.lockfileFolder`. However in workspaces, bins may not be hoisted to the workspace root,
causing bins to not be found. This change adds `config.cwd` to the bin search path, so the `yarn
run` command will look in a workspace package's node_modules, as well as the workspace root.

fixes yarnpkg#7126

* modify chagelog
VincentBailly pushed a commit to VincentBailly/yarn that referenced this pull request Jun 10, 2020
…kg#7151)

* fix(run): change run command to check cwd/node_modules/.bin for commands. Fixes run in workspaces.

This fixes a bug that was introduced in yarnpkg#6850 where the bin path was being built only from
`config.lockfileFolder`. However in workspaces, bins may not be hoisted to the workspace root,
causing bins to not be found. This change adds `config.cwd` to the bin search path, so the `yarn
run` command will look in a workspace package's node_modules, as well as the workspace root.

fixes yarnpkg#7126

* modify chagelog
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

Successfully merging this pull request may close these issues.

yarn <executable> with workspaces doesn't work anymore
7 participants