link bins of transitive deps to top level #3310

rally25rs · 2017-05-03T14:38:54Z

Summary

Add transitive dependency bin symlinks to top-level node_modules/.bin to be more consistent with NPM behavior.

Fixes #2874
Replaces #3272

package-linker now iterates over the flat/hoisted tree and finds the newest version of each package. Each of those packages gets its bins linked at the top-level.

After that, during PackageLinker.save() each direct dependency has its bins linked (this was existing behavior, so unchanged) which will overwrite any existing bin links from transitive deps. This ensures that direct dependencies always take priority over the transitive ones.

Test plan

Stopped skipping a previously removed test, and added an additional test in __tests__/commands/install/integration.js

rally25rs · 2017-05-03T14:59:15Z

Getting this error on Appveyor:

FAIL  __tests__\commands\install\integration.js (161.599s)
  ● install should hoist nested bin scripts
    Error: UNKNOWN: unknown error, readlink 'C:\Users\appveyor\AppData\Local\Temp\1\yarn-install-nested-bin-1493822638231-0.43938980052931487\node_modules\.bin\standard'
        at Error (native)

These tests pass on my machine; OSX w/ Node 6.3.0.

If someone could help debug or even figure out how to reproduce, I would appreciate it!

I tried using the same node version and test command that appveyor uses:

~/Projects/yarn : node --version
v6.10.2
~/Projects/yarn : npm run test-only -- --maxWorkers 3
    .
    .
    .
Test Suites: 56 passed, 56 total
Tests:       5 skipped, 497 passed, 502 total
Snapshots:   53 passed, 53 total
Time:        56.126s, estimated 59s
Ran all test suites.

gonzofish · 2017-05-03T16:55:27Z

I'm getting errors on my pull request, too (#3308)...they're not related to anything I altered (as far as I can tell)

rally25rs · 2017-05-03T17:18:40Z

In this case it is failing in my test that I added, and I did use calls to fs.readlink() but can't figure out why it's throwing an error. That specific message doesn't come up from a Google search either.

The same test passes on the Travis build (that build fails due to an out-of-memory error though, which I doubt is my fault).

Must be some environment different between Travis and Appveyor?

Just realized Appveyor is running Windows builds. That's probably the difference. Nothing in the Node docs for fs.readlink indicate that it doesn't work on Windows though... ugh, I'm going to have to turn on my Windows laptop 😞

uhhhh if I run the same branch on my Surface Pro 3 i7 Win10, I get 60 failing tests and it takes an astonishing 647 seconds. 😢

sigh ; oh, right, they aren't actually symlinks when on Windows. It outputs a shell script with the same name instead. I'll probably have to have my unit test verify on Windows by looking for a string in the shell script instead of checking a link... working up a fix...

rally25rs · 2017-05-03T21:42:50Z

Alright, current status of this PR:

I fixed my unit tests to verify what directories the bin links point to so that they work on Windows and unix/mac.

CircleCI passes. 👍
Travis Unix tests pass, but the Apple test fails because "FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory" so that's not my fault. 👍
Appveyor builds both fail, but for different reasons, on tests that have passed in previous builds, and are unrelated to my changes. I think these are just some flakey tests, but my changes should be good. 👍

snagi · 2017-05-03T23:39:19Z

Did some analysis on various combanitions of dependency installs using npm.

Scenario: Transitive dependency having bin script

{
  "dependencies": {
    "sample-dep-eslint-3.10.0": "0.0.1"
  }
}

npm behavior: [email protected] is symlinked in node_modeules/.bin

$ ll node_modules/.bin/eslint
lrwxr-xr-x  node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/eslint/package.json
"3.10.0"

Scenario: Transitive dependency having version that is overridden by newer version as the direct dependency

{
  "dependencies": {
    "eslint": "3.19.0",
    "sample-dep-eslint-3.10.0": "0.0.1"
  }
}

npm behavior: [email protected] is symlinked in node_modeules/.bin and [email protected] is symlinked to node_modules/sample-dep-eslint-3.10.0/node_modules/.bin

$ ll node_modules/.bin/eslint
lrwxr-xr-x  node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/eslint/package.json 
"3.19.0"

$ ll node_modules/sample-dep-eslint-3.10.0/node_modules/.bin/eslint 
lrwxr-xr-x  node_modules/sample-dep-eslint-3.10.0/node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/sample-dep-eslint-3.10.0/node_modules/eslint/package.json 
"3.10.0"

Scenario: Transitive dependency having version that is overridden by older version as the direct dependency

{
  "dependencies": {
    "eslint": "3.7.0",
    "sample-dep-eslint-3.10.0": "0.0.1"
  }
}

npm behavior: [email protected] is symlinked in node_modeules/.bin and [email protected] is symlinked to node_modules/sample-dep-eslint-3.10.0/node_modules/.bin

$ ll node_modules/.bin/eslint
lrwxr-xr-x  node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/eslint/package.json 
"3.7.0"
$ ll node_modules/sample-dep-eslint-3.10.0/node_modules/.bin/eslint 
lrwxr-xr-x  node_modules/sample-dep-eslint-3.10.0/node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/sample-dep-eslint-3.10.0/node_modules/eslint/package.json 
"3.10.0"

Scenario: Transitive dependency having version that is overridden by newer version as the dev dependency

{
  "dependencies": {
    "sample-dep-eslint-3.10.0": "0.0.1"
  },
  "devDependencies": {
    "eslint": "3.19.0"
  }
}

npm behavior: [email protected] is symlinked in node_modeules/.bin and [email protected] dependency for sample-dep-eslint-3.10.0 module is ignored. This same behavior exists when dev dependency specifies an older version.

$ ll node_modules/.bin/eslint
lrwxr-xr-x  node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/eslint/package.json 
"3.19.0"

$ ll node_modules/sample-dep-eslint-3.10.0/node_modules/.bin/
ls: node_modules/sample-dep-eslint-3.10.0/node_modules/.bin/: No such file or directory

Scenario: Transitive dependency having version that is overridden by older version as the dev dependency

{
  "dependencies": {
    "sample-dep-eslint-3.10.0": "0.0.1"
  },
  "devDependencies": {
    "eslint": "3.7.0"
  }
}

npm behavior: [email protected] is symlinked in node_modeules/.bin and [email protected] dependency for sample-dep-eslint-3.10.0 module is ignored.

$ ll node_modules/.bin/eslint
lrwxr-xr-x  node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/eslint/package.json 
"3.7.0"
$ ll node_modules/sample-dep-eslint-3.10.0/node_modules/.bin/
ls: node_modules/sample-dep-eslint-3.10.0/node_modules/.bin/: No such file or directory

Scenario: Transitive dependency having version that is overridden by newer version as the optional dependency

{
  "dependencies": {
    "sample-dep-eslint-3.10.0": "0.0.1"
  },
  "optionalDependencies": {
    "eslint": "3.19.0"
  }
}

npm behavior: [email protected] is symlinked in node_modeules/.bin and [email protected] is symlinked to node_modules/sample-dep-eslint-3.10.0/node_modules/.bin. This behavior is same as direct dependency.

$ ll node_modules/.bin/eslint
lrwxr-xr-x  node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/eslint/package.json 
"3.19.0"

$ ll node_modules/sample-dep-eslint-3.10.0/node_modules/.bin/eslint 
lrwxr-xr-x  node_modules/sample-dep-eslint-3.10.0/node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/sample-dep-eslint-3.10.0/node_modules/eslint/package.json 
"3.10.0"

Scenario: Transitive dependency having version that is overridden by older version as the optional dependency

{
  "dependencies": {
    "sample-dep-eslint-3.10.0": "0.0.1"
  },
  "optionalDependencies": {
    "eslint": "3.7.0"
  }
}

npm behavior: [email protected] is symlinked in node_modeules/.bin and [email protected] is symlinked to node_modules/sample-dep-eslint-3.10.0/node_modules/.bin. This behavior is same as direct dependency.

$ ll node_modules/.bin/eslint
lrwxr-xr-x  node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/eslint/package.json 
"3.7.0"

$ ll node_modules/sample-dep-eslint-3.10.0/node_modules/.bin/eslint 
lrwxr-xr-x  node_modules/sample-dep-eslint-3.10.0/node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/sample-dep-eslint-3.10.0/node_modules/eslint/package.json 
"3.10.0"

Scenario: Transitive dependency having version that is conflicting with another transitive dependency version

{
  "dependencies": {
    "sample-dep-eslint-3.10.0": "0.0.1",
    "sample-dep-eslint-3.19.0": "0.0.1"
  }
}

npm behavior: [email protected] is symlinked in node_modeules/.bin and [email protected] is symlinked to node_modules/sample-dep-eslint-3.19.0/node_modules/.bin. Here it seems like npm add the modules in alphabatical order and transitive deps of first dependency is installed at top level.

$ ll node_modules/.bin/eslint
lrwxr-xr-x  node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/eslint/package.json 
"3.10.0"
$ ll node_modules/sample-dep-eslint-3.19.0/node_modules/.bin/eslint 
lrwxr-xr-x  node_modules/sample-dep-eslint-3.19.0/node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/sample-dep-eslint-3.19.0/node_modules/eslint/package.json 
"3.19.0"

Scenario: Transitive dependency having version that is conflicting with another dev transitive dependency version

{
  "dependencies": {
    "sample-dep-eslint-3.10.0": "0.0.1"
  },
  "devDependencies": {
    "sample-dep-eslint-3.19.0": "0.0.1"
  }
}

$ ll node_modules/.bin/eslint
lrwxr-xr-x  node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/eslint/package.json 
"3.10.0"
$ ll node_modules/sample-dep-eslint-3.19.0/node_modules/.bin/eslint 
lrwxr-xr-x  node_modules/sample-dep-eslint-3.19.0/node_modules/.bin/eslint -> ../eslint/bin/eslint.js
$ jq '.version' node_modules/sample-dep-eslint-3.19.0/node_modules/eslint/package.json 
"3.19.0"

rally25rs · 2017-05-03T23:49:11Z

@snagi this is awesome work! thank you! It's probably worth copying this information back to the issue #2874 too, since there are other details there.

If you are willing to tackle it, I think it would be really worth it to add some of these cases to the unit tests of this PR. I only added one relatively simple one.

The one that I know will be different is when 2 transitive deps both depend on different versions. NPM seemed to pick up whichever was "first", and my implementation here will pick the one with the greatest version (which IMO is "better" and I think we should leave it this way instead of the NPM way).

Thanks so much for the help looking into this issue!

snagi · 2017-05-04T00:38:06Z

There could be an issue in npm as well about how it handles duplicate transitive dependencies -

mypkg -> pkgA -> eslint@1
mypkg -> pkgA -> eslint@2

npm randomly (or in alphabatical order of direct deps) picks a version for transitive dep, while flattening the tree. This does not matter for the execution of js code as other transitive version is install correctly in the parent dependency. But for bin script, it might be an older version.

Logic in yarn is to install the transient dependency version, that is most dependent by other modules, at root/top level.

  /**
   * Perform a prepass and if there's multiple versions of the same package, hoist the one with
   * the most dependents to the top.
   */

  prepass(patterns: Array<string>) {

In this regard symlinking the latest version of transitive dependency might not be appropriate as it would be linking to a different version instead of one installed at top level. May be we should symlink only those modules/versions which are installed at root level in flattree.

determineTopLevelBinLinks(flatTree: HoistManifestTuples): Array<Manifest> {
  const linksToCreate = new Map();
  const modulesDirectory = path.resolve(this.config.cwd)
  flatTree.forEach(([dest, {pkg}]) => {
    // there would not be any duplicate modules at root level; flat tree would have already resolved the versions to install at root level. These same versions need to be symlinked for binscripts.
    if (modulesDirectory === path.dirname(dest)) {
      linksToCreate.set(pkg.name, pkg);
    }
  });
  return Array.from(linksToCreate.values());
}

Better approach would be to identify and flag the packages which are being hoisted at top level in package-hoister.js. I am still trying to get my head around how the tree is being built as part of package-hoister.js, so will probably comment later on where we could flag that bit.

snagi · 2017-05-04T00:45:07Z

There is some difference in how npm and yarn handles different versions of transitive dependencies. There are some pros with both yarn apporach (which ever is most dependent on goes at top level) and using latest version at top level.
Which ever approach we use we should have bin script and package install at root level for same version.

I am happy to contribute these tests to your PR, will work on this tomorrow.

rally25rs · 2017-05-04T01:00:01Z

I had the thought to make a bin-hoister to handle this, but yeah it might be able to shim into package-hoister somehow. Good luck figuring out how it all works... I stared at it for almost a week and made a PR that was nothing but unit tests around package-hoister because writing tests helped me understand it... I actually started completely rewriting it from scratch, but gave up. There are a lot of weird edge cases and conditionals in there...

However I'm pretty sure that somewhere it does get a count of how many other packages depend on each package, as part of another improvement that put the most-used packages closest tot he root level do avoid more duplication (and save HD space).

Part of me doesn't really mind if the root-level bin isn't exactly the same as NPM's, because if another package has install scripts that do care, they will be run in the context of the nested bin link anyway, so should be using the correct version.

But if we can sort it out to work the same as NPM then let's do it 👍

rally25rs · 2017-05-04T11:49:53Z

@snagi taking a look at the code again this morning;

In reply to your comment:

In this regard symlinking the latest version of transitive dependency might not be appropriate as it would be linking to a different version instead of one installed at top level. May be we should symlink only those modules/versions which are installed at root level in flattree.

Up in the PR notes I mention that it makes these transitive links first based on highest-version, but after that, they are overwritten for top-level direct dependencies. It just happens at a different spot in the code (because I left that part of the original implementation alone).

…mments. Changed bin linking to match this behavior

rally25rs · 2017-05-08T00:15:36Z

Finally got some time to come back to this...
Made the following changes:

Moved existing bin link tests to new test file: __tests__/commands/install/bin-links.js
Added tests to cover most of the NPM test cases mentioned by @snagi except for one (see below)
Changed the bin linking behavior to pass these new tests.

The one test scenario I ignored was:

Transitive dependency having version that is overridden by newer version as the dev dependency
npm behavior: [email protected] is symlinked in node_modeules/.bin and [email protected] dependency for sample-dep-eslint-3.10.0 module is ignored.

I ignored this one because to me it seemed like an NPM bug. It didn't make sense to me that a transient dependency would be linked in its nested directory if there was a conflicting dependency but not if there was a conflicting devDependency. Plus, from package-linker I wasn't sure how to tell if a dep was a devDependency anyway. I did add a test case for this behavior but marked it test.skip with an explanation.

Local OSX npm test Test results after these changes:

Test Suites: 58 passed, 58 total
Tests:       6 skipped, 502 passed, 508 total
Snapshots:   53 passed, 53 total
Time:        59.605s
Ran all test suites.

Appveyor : node_version=6 seems to be failing due to:

Summary of all failing tests
 FAIL  __tests__\commands\install\integration.js (228.675s)
  ● install incompatible optional dependency should still install shared child dependencies
    Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
      
      at Timeout._onTimeout (node_modules\jest-jasmine2\vendor\jasmine-2.5.2.js:1977:23)
      at ontimeout (timers.js:380:14)
      at tryOnTimeout (timers.js:244:5)

but that seems to be a common problem. The node_version=4 job passes.

Any additional feedback anyone? If not, I'd love to see this get merged. @snagi @bestander

bestander · 2017-05-08T15:55:06Z

Great work, @rally25rs!
And thanks @snagi for more test data.
This looks good, CI breaks seem unrelated

manishshanker · 2017-05-12T09:42:13Z

Awesome @snagi, @bestander, @rally25rs! Have been waiting to get this sorted out. Thanks a lot.

Since Yarn 0.25 (yarnpkg/yarn#3310), Yarn transitively symlinks binaries into node_modules/.bin. (Previously, it would only sync top-level binaries--i.e., binaries that we directly depend on.) The new behavior results in adding all sorts of junk to the path--notably, a JavaScript which reimplementation that shadows the system-provided which. Ew. Avoid putting node_modules/.bin on the PATH by using full paths to the binaries within, or using `yarn run` if within pkg/ui.

yarnpkg#2874 link bins of transitive deps to top level

1acc829

rally25rs mentioned this pull request May 3, 2017

symlink the metadeps with binaries into the topmost node_modules/.bin #3272

Closed

rally25rs changed the title ~~#2874 link bins of transitive deps to top level~~ link bins of transitive deps to top level May 3, 2017

rally25rs added 4 commits May 3, 2017 15:43

fixing bin link location checks for windows, where they are not symlinks

90f93cf

WIP: adding debug line to debug appveyor build

170f06f

fix link path regex

da024cd

fix Flow type

4d64fee

rally25rs added 3 commits May 7, 2017 11:44

Merge branch 'master' into bin-links-2874

a90b0f6

move install unit tests related to bin linking to own file

58fd750

added tests for NPM bin link behavior mentioned in PR yarnpkg#2874 co…

0422826

…mments. Changed bin linking to match this behavior

bestander merged commit 1b17064 into yarnpkg:master May 8, 2017

DanBuild mentioned this pull request May 12, 2017

yarn-homebrew #10 failed #3386

Closed

bestander mentioned this pull request May 30, 2017

eslint problem #3535

Closed

kaylie-alexa mentioned this pull request Jul 10, 2017

Prioritize bin links at the root level #3877

Merged

This was referenced Aug 29, 2017

Better handling of nested executable script #760

Closed

Scoped packages missing .bin scripts #2758

Closed

javan mentioned this pull request Aug 31, 2017

Bump required yarn version rails/webpacker#727

Merged

benesch mentioned this pull request Sep 2, 2017

build: don't put .node_modules/bin on the path cockroachdb/cockroach#18181

Merged

edmorley mentioned this pull request Oct 20, 2017

yarn install installs wrong version #4721

Open

anders-advisa mentioned this pull request Nov 15, 2017

Sub dependency .bin symlinks can override top level dependency .bin symlink #4937

Closed

lukebatchelor mentioned this pull request Sep 17, 2018

Scoped binary deps are not added to node_modules/.bin #1487

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

link bins of transitive deps to top level #3310

link bins of transitive deps to top level #3310

rally25rs commented May 3, 2017 •

edited

Loading

rally25rs commented May 3, 2017 •

edited

Loading

gonzofish commented May 3, 2017

rally25rs commented May 3, 2017 •

edited

Loading

rally25rs commented May 3, 2017

snagi commented May 3, 2017

rally25rs commented May 3, 2017

snagi commented May 4, 2017

snagi commented May 4, 2017 •

edited

Loading

rally25rs commented May 4, 2017

rally25rs commented May 4, 2017 •

edited

Loading

rally25rs commented May 8, 2017

bestander commented May 8, 2017

manishshanker commented May 12, 2017

link bins of transitive deps to top level #3310

link bins of transitive deps to top level #3310

Conversation

rally25rs commented May 3, 2017 • edited Loading

rally25rs commented May 3, 2017 • edited Loading

gonzofish commented May 3, 2017

rally25rs commented May 3, 2017 • edited Loading

rally25rs commented May 3, 2017

snagi commented May 3, 2017

rally25rs commented May 3, 2017

snagi commented May 4, 2017

snagi commented May 4, 2017 • edited Loading

rally25rs commented May 4, 2017

rally25rs commented May 4, 2017 • edited Loading

rally25rs commented May 8, 2017

bestander commented May 8, 2017

manishshanker commented May 12, 2017

rally25rs commented May 3, 2017 •

edited

Loading

rally25rs commented May 3, 2017 •

edited

Loading

rally25rs commented May 3, 2017 •

edited

Loading

snagi commented May 4, 2017 •

edited

Loading

rally25rs commented May 4, 2017 •

edited

Loading