Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

npm@3 given the same module list, not linking in same order #8995

Closed
phated opened this issue Jul 19, 2015 · 15 comments
Closed

npm@3 given the same module list, not linking in same order #8995

phated opened this issue Jul 19, 2015 · 15 comments
Assignees
Milestone

Comments

@phated
Copy link
Contributor

phated commented Jul 19, 2015

As per @iarna at gulpjs/gulp-cli#7 (comment)

I haven't tested this, but a gulp core contributor tested and linked results at gulpjs/gulp-cli#7 (comment)

@othiym23 othiym23 added the bug label Jul 27, 2015
@othiym23 othiym23 added this to the 3.x milestone Jul 27, 2015
@othiym23
Copy link
Contributor

Thanks for the report, @phated. We're probably going to have to take a few passes at getting this resolved in a way that feels good to everybody, but I imagine @iarna has a solution in mind for this (and the other issue you filed related to this) in a nearish term.

@iarna
Copy link
Contributor

iarna commented Jul 27, 2015

I haven't been able to reproduce this one, but as an ordering bug I think some quality time thinking about the code will find the problem area.

@iarna iarna added the blocker label Aug 11, 2015
@yocontra
Copy link

Any ETA on fixing this? We aren't able to release gulp 4.0 until this is fixed

@iamstarkov
Copy link

im waitin gulp4 for almost one year, pls fix it 🐱

@iarna
Copy link
Contributor

iarna commented Aug 14, 2015

I would love the gulp folks to provide some explanation for why this is a blocker for them. This was, if anything, worse in npm@2.

@iarna
Copy link
Contributor

iarna commented Aug 14, 2015

Looking back in gulpjs/gulp-cli#7 it looks like npm@2 used to just explode. With #8996 this may be good enough.

And digging into gulpjs/gulp-cli#7 I'd really like to see this reproduced with a current release, as I'm skeptical that it can still happen– binaries are linked into place by the build action, and that's done serially (and our serial actions happen in a deterministic order):

    [doSerialActions, 'build', staging, todo, trackLifecycle.newGroup('build')],

@heikki
Copy link

heikki commented Aug 14, 2015

#!/bin/sh

counter=0

function install {
    let counter++
    npm install gulp gulp-cli > /dev/null && printf "install $counter, gulp bin -> " && readlink node_modules/.bin/gulp && install
}

echo "Running 'npm install gulp gulp-cli' repeatedly"
rm -fr node_modules
install

With npm 3.2.2 I get this output:

Running 'npm install gulp gulp-cli' repeatedly
install 1, gulp bin -> ../gulp-cli/bin/gulp.js
install 2, gulp bin -> ../gulp/bin/gulp.js
install 3, gulp bin -> ../gulp/bin/gulp.js
install 4, gulp bin -> ../gulp/bin/gulp.js
install 5, gulp bin -> ../gulp/bin/gulp.js
install 6, gulp bin -> ../gulp/bin/gulp.js
install 7, gulp bin -> ../gulp/bin/gulp.js

@iarna
Copy link
Contributor

iarna commented Aug 14, 2015

Ok, what I see is deterministic ordering for the initial run, and for the additional runs, but a difference between the first run and those additional runs.

@iarna
Copy link
Contributor

iarna commented Aug 14, 2015

So followup runs consistently produce this order:

npm sill diffTrees update gulp-cli@0.3.0
npm sill diffTrees update gulp@3.9.0

Which is consistent with @heikki's output and with theory (we there are no cross module dependencies we sort by path length).

@iarna
Copy link
Contributor

iarna commented Aug 14, 2015

Likewise a fresh install consistently produces this order: (gulp and gulp-cli are installed last after allll their deps)

npm sill diffTrees add gulp@3.9.0
npm sill diffTrees add gulp-cli@0.3.0

@iarna
Copy link
Contributor

iarna commented Aug 14, 2015

I think the latter is happening due to gulp being a dev-dep of gulp-cli. The question is, why doesn't this rule also apply with the first scenario.

@iarna iarna removed the needs-repro label Aug 14, 2015
@iarna iarna modified the milestones: 3.x-next-next, 3.x Aug 14, 2015
@iarna
Copy link
Contributor

iarna commented Aug 14, 2015

Scheduling this for Aug 20th's release.

@phated
Copy link
Contributor Author

phated commented Aug 18, 2015

Excellent, thanks!

iarna added a commit that referenced this issue Aug 21, 2015
The install order is determined by:

1) The location of the thing to be installed relative to the root module.
2) Dependency order, such that a -> b -> c will install modules as: c, b, a

1 is deterministic, regardless of what's being installed.

But 2 can change the order of things higher in the dep tree.  Eg, b, or a
might get sorted earlier if c requires them.  This mostly doesn't matter,
but it does mean that if you have two modules with conflicting bins, they
_can_ get installed in different orders.  This changes sorts all of the top
level modules to be LAST, in location order (1), and then sorts all the rest
per (2).  This ensures that top level modules have a deterministic install
order. (Non top level modules can't have bin conflicts as that's treated
the same as a version conflict and the conflicting module would be hoisted.)

PR-URL: #9274
Fixes: #8995
@iarna iarna added ready and removed in-progress labels Aug 21, 2015
iarna added a commit that referenced this issue Aug 21, 2015
The install order is determined by:

1) The location of the thing to be installed relative to the root module.
2) Dependency order, such that a -> b -> c will install modules as: c, b, a

1 is deterministic, regardless of what's being installed.

But 2 can change the order of things higher in the dep tree.  Eg, b, or a
might get sorted earlier if c requires them.  This mostly doesn't matter,
but it does mean that if you have two modules with conflicting bins, they
_can_ get installed in different orders.  This changes sorts all of the top
level modules to be LAST, in location order (1), and then sorts all the rest
per (2).  This ensures that top level modules have a deterministic install
order. (Non top level modules can't have bin conflicts as that's treated
the same as a version conflict and the conflicting module would be hoisted.)

PR-URL: #9274
Fixes: #8995
@othiym23
Copy link
Contributor

Landed as d395a6b, included in npm@3.3.1. Thanks for reporting this and for your patience, Blaine!

@phated
Copy link
Contributor Author

phated commented Aug 28, 2015

🎆 🍰 @othiym23 @iarna

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

No branches or pull requests

6 participants