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.

Sign up for GitHub

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

Jump to bottom

[Bug] .bin resolving order is wrong when using yarn workspace #8590

Open
sglim opened this issue Mar 3, 2021 · 5 comments · May be fixed by #9079
Open

[Bug] .bin resolving order is wrong when using yarn workspace #8590

sglim opened this issue Mar 3, 2021 · 5 comments · May be fixed by #9079

Comments

@sglim
Copy link

sglim commented Mar 3, 2021

Expected

In yarn workspace, yarn run command use package dependent binary instead of root one.

Actual

yarn run uses root binary when there are different binary versions

How to reproduce?

See https://github.com/sglim/yarn-workspace-bin-bug
@sglim sglim mentioned this issue Mar 3, 2021
Open
@rally25rs
Copy link
Contributor

I believe this is as-designed. I ordered the .bin resolution specifically in that order a few years ago #4543 (comment) because if you are running a command for a specific package, it should use that package's explicit dependency before falling back to the workspace root.

When you run yarn commands from within a workspace package, yarn considers you to be operating on that package. In your example, cd packages/workspace-1 ; yarn add leftpad would add leftpad as a dependency to workspace-1 's package.json only, not to workspace-2 or to the workspace root.

Likewise, running yarn run from within the packages/workspace-1 directory considers you to be operating on workspace-1 and will use it's .bin folder first.

@devurandom
Copy link

I appear to be running the same problem. In a project with two workspaces that use different versions of Cypress, 11.2.0 and 13.4.0, yarn workspaces run cypress version executes the root module's node_modules/.bin/cypress (which happens to be 11.2.0) instead of a workspace-dependent version, even though myworkspace/node_modules/.bin/cypress exists and points to myworkspace/node_modules/cypress/bin/cypress in both cases.

@benzittlau
Copy link

@rally25rs It appears you might have misread the report. The OP was saying that the expected behaviour is that yarn run would utilize the package's explicit dependency before falling back to the root (as you state the behaviour should be), but what they were actually observing is that it was utilizing the root version of the package instead.

I am observing this same behaviour with yarn 1.22.22 and attempting to utilize a different eslint version via yarn run within the package.

Here's a repo with reproduction steps:
https://github.com/benzittlau/yarn-workspace-package-version-bug

@rally25rs
Copy link
Contributor

rally25rs commented Jul 6, 2024 •
edited
Loading

ohhh I see! Sorry @benzittlau , my comment was partially correct in that yarn will run the correct script and also sets the PATH correctly sometimes. It looks like there is a case where it does not. I appreciate the sample project. That helps a lot.

OK so after some testing:

  1. yarn sets the PATH correctly to have the workspace package's .bin first, before the root .bin.

Can see this by adding a script to package.json in a sub package:

"scripts": {
  "path": "echo $PATH"

then run

yarn workspace workspace-a run path

outputs (replaced ':' with newline to make its readable)

/var/folders/2k/gl0z9w3s0wv01mnwn5mv59zm0000gn/T/yarn--1720231136138-0.14349062450304784
/Users/jeff/Projects/yarn-workspace-package-version-bug/src/apps/workspace-a/node_modules/.bin
/Users/jeff/.config/yarn/link/node_modules/.bin
/Users/jeff/Projects/yarn-workspace-package-version-bug/node_modules/.bin
/Users/jeff/.yarn/bin
/Users/jeff/.config/yarn/global/node_modules/.bin
/usr/local/bin
/usr/bin
/bin
/usr/sbin
/sbin

which would handle any other child processes that get executed, since it would resolve src/apps/workspace-a/node_modules/.bin before node_modules/.bin

  1. if a script is defined in package.json then the correct bin is executed.

for example if you edit both workspace-a/package.json and workspace-b/package.json and add the script

"scripts": {
  "lint": "eslint --version"

then execute them, they now work as expected:

$ yarn workspace workspace-a run lint

v8.57.0

$ yarn workspace workspace-b run lint

v7.32.0
  1. what does not seem to work is simply running abin that is installed by a transitive dep, that isn't a package.json script.
$ yarn workspace workspace-a run eslint --version

v7.32.0

$ yarn workspace workspace-b run eslint --version

v7.32.0

This appears to be related to this getBinEntries method: https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/run.js#L28

I think the problem is that the return value is a Map<string, string> that maps script names to paths.
It checks the workspace first and adds

eslint = yarn-workspace-package-version-bug/src/apps/workspace-a/node_modules/.bin/eslint

then runs through the workspace root next, and ends up replacing the workspace-specific eslint with

eslint = yarn-workspace-package-version-bug/node_modules/.bin/eslint

It seems like the order would need to either be reversed, or check first to see if a script is already in the map, and don't replace it if it is.

Reopening this issue, however please note that the timeline for fixing things in yarn classic/v1 that aren't critical (yarn no longer works at all) or a security vuln, aren't added that often, so not sure if/when this would get fixed. Upgrading to yarn modern is always a solid suggestion (we are up to v4 now, so bug fixing v1 hasn't been a priority for ~3 years).

In the meantime, you can also work around this by adding the command as a script in package.json as shown in 2) above.

@rally25rs rally25rs reopened this Jul 6, 2024
rally25rs added a commit to rally25rs/yarn that referenced this issue Jul 6, 2024
@rally25rs
fix(run): Use bin scripts defined in a workspace package before the w…
49d1841 
…orkspace root.

When resolving bin scripts, ones in the workspace root were being added to a Map last,
overwriting those from the workspace package where the script was being run. This change will now
favor workspace-package specific bins instead.

fix yarnpkg#8590
rally25rs added a commit to rally25rs/yarn that referenced this issue Jul 6, 2024
@rally25rs
fix(run): Use bin scripts defined in a workspace package before the w…
968a440 
…orkspace root.

When resolving bin scripts, ones in the workspace root were being added to a Map last,
overwriting those from the workspace package where the script was being run. This change will now
favor workspace-package specific bins instead.

fix yarnpkg#8590
@rally25rs rally25rs linked a pull request Jul 6, 2024 that will close this issue
Open
@benzittlau
Copy link

@rally25rs Awesome, thanks so much for digging into that. We played around with it a bit further here and similarly discovered that the $PATH appeared to have the correct sequencing, but beyond that we were running into the limits of our knowledge to investigate further. Knowing about the workaround via a package.json script helps a lot.

neuracr added a commit to google/safety-web that referenced this issue Jul 25, 2024
@neuracr
Add commands to update the violation golden files
a79b037 
Also explicitely use the node_module path for running eslint8 as there's a bug in yarn that makes it run the wrong binary yarnpkg/yarn#8590

Change-Id: I0ffb28f7831b8c016d1f6f00b9af9909d78647ed
neuracr added a commit to google/safety-web that referenced this issue Jul 25, 2024
@neuracr
Pull in the latest safety-web development (#4)
bfa0219 
* Add a base skeleton for the plugin

Change-Id: I7e98fdf3aa147ca8c1e14eb0f2bbf508aa610f60

* Set up Mocha with a placeholder test

Change-Id: I2f31bcf5f868c237417f17a75dc58d0e62438fba

* Remove tests from tsconfig and add yarn clean script

Change-Id: I1455d874fd158a5e915e48c4d1a57de40b37cbf2

* Add Copyright header to js files

Change-Id: Ib3f5a77d4b36e21b40413092dc42ddf135b30378

* Add a notice that safety-web is under development

Change-Id: Idd3d9458d2682560d9b9534a87203aa34ac3ba68

* Sets the rule tester up

Change-Id: I14e9dfd64aea1b8262a762c3689aa07caac34d18

* Add a script to update a vendored version of tsetse and add the current version of tsetse

Change-Id: I542eafad8b6570612592af12f43041c7767fc921

* Track the last run of the tsetse_update.sh script and the latest commit pulled for tsetse

Change-Id: Ice0292972ac65701676e34289688dd03d110cb37

* Add readme instructions for updating tsetse

Change-Id: I541078c4ff8cb51f177f6e73242c0637db47aa2b

* Add dependencies for tsetse

Change-Id: If68828cde13c7a70276115493e6d311cf489aad8

* Wire the tsetse checks, update the tests and test fixtures

This implementation is taken from the tsec eslint plugin. It only works with projects that have a tsconfig setup.

Change-Id: Ia8c000f96e10ec0a91e9c232af47fee72e590b53

* Add a test project as an integration test

Change-Id: I8b39d9537ca903b26f2a2758ac9c50d3054364df

* Add a test helper to check for expected violations

WIP: this helper will be refined to focus on certain fields and make the test less rigid.
Change-Id: I5b0f64c6b52f7b89b7e94bd5c5fbf62af709097b

* Add a JavaScript test project as an integration test

Change-Id: I4031da14bad7d9d4b8999727e81bd0178b33067c

* Set ESLint up for safety-web sources

Change-Id: Ieff5925cfd6ecfbc314dd69ad3d077cab0320ae1

* Fix the lint issues in safety-web

Change-Id: Ic649c4eadd8c036727e7f71af8a1368878d0a4bc

* Fix the main entry point path

Since test/ was added to the tsconfig, lib.index.js moved to lib/src/index.js. This was caught in the integration tests, but I haven't set the CI yet and I forgot to run them before the tsconfig change.

Change-Id: Id9bacc21976090270a008abcfb93c46e3ac01bdb

* Only add compiled JS to the NPM release

Change-Id: Ic96d5a7e791bc0e80ea6b7ba5973a03f3a314659

* Improve the violation expectation helper

- Canonicalize the finding reports to compare them
- Also add the license header
- Also add the javascript test project to the integration tests

Change-Id: I6eded1df866bdb0fe3a364e2834d361c67a36df1

* Add a typescript integration test set up with ESLint8

There are a few breaking changes between ESLint 8 and 9 so it's worth testing that safety-web works with both versions.

Change-Id: Ic97ab5d5d7a958631cb2ef527e1748752ed2528b

* Fix the basic-typescript-eslint9 tests

Change-Id: Icf62c861dc3f95377e10a4474da188214dd01ce9

* Define common script for all packages to be able to use `yarn workspaces run x`

Change-Id: I4aced5d5fbe69aec917dbf52cf14bb81e2822fe4

* Add a test:watch

Change-Id: Iba160f4b696ce2bfc2bdbd5d3b375c46cc5b865d

* Wire tsetse violations to ESLint message ids.

Change-Id: Ic594a1ed16102e987bb2e47b6d30d0f524134d52

* Add commands to update the violation golden files

Also explicitely use the node_module path for running eslint8 as there's a bug in yarn that makes it run the wrong binary yarnpkg/yarn#8590

Change-Id: I0ffb28f7831b8c016d1f6f00b9af9909d78647ed

* Clean the javascript eslint 9 test config up

Change-Id: I17239dc278005c054d83b7e45599bb72205f05d9

* Mark the test helper binary as executable once built

Change-Id: I4b111226d5920c0de3ac9ff5236c68277fa5e184

* Add a integration for javascript running on ESLint8

Change-Id: Ide643796658c061627191d0e020750dd721b3cc0

* Add a build:watch script to auto-rebuild safety-web on changes

Change-Id: I9a8758a69f39569181864851a7f6dff19fde9543

* Check that we're actually using eslint8 in integration test

yarn run eslint doesn't resolve the correct version by default. This was
already working, but was not checked.

Change-Id: I74fd24254d7ce0f17ad984668c78cf3ab869d127

* Specify the yarn version to use with corepack

Change-Id: I55c8548c6ee85d97f7d5d71e08642178d978d704

* Upgrade yarn to the latest version (Berry 4.3.1)

Also upgrade the version of some dependencies, commands and
documentation to match the new Yarn syntax.

Change-Id: I21aaa6ba30185fc7c49b31efbcce31660fff7717

* Fix the project after Yarn was upgraded.

Change-Id: Ibcb3320de668abccb68fece5242c74913d4b36e2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(run): Use bin scripts defined in a workspace package before the workspace root. rally25rs/yarn
4 participants
@rally25rs @devurandom @benzittlau @sglim