Don't try to replace modules added from a bundle with moves from a non-bundle #15081

iarna · 2016-12-01T03:17:13Z

No description provided.

coveralls · 2016-12-01T05:08:36Z

Changes Unknown when pulling e5b39cca520bb816ac1cbbf426656769b9e40667 on iarna/fix-move-modules into ** on release-next**.

zkat

This seems like a really good idea! It took me a while to grasp what's going on, why it was important, etc, but that just comes with the territory of figuring out how these bits work :)

zkat · 2016-12-01T07:59:22Z

lib/install/diff-trees.js

@@ -113,7 +113,7 @@ var sortActions = module.exports.sortActions = function (differences) {
  return sorted
 }

-function diffTrees (oldTree, newTree) {
+var diffTrees = module.exports._diffTrees = function (oldTree, newTree) {


[nit]: using the style we use for most other stuff would be nice:

module.exports._diffTrees function diffTrees (...) {

zkat · 2016-12-01T08:01:29Z

test/tap/bundled-no-add-to-move.js

+
+test('test', function (t) {
+  var differences = sortActions(diffTrees(oldTree, newTree)).map(function (diff) { return diff[0] + diff[1].location })
+  t.isDeeply(differences, ['add/abc/one', 'remove/one', 'add/abc'], 'bundled add/remove stays add/remove')


This seems to be the first test we've had that uses diffTrees to check things at this level -- can we have a baseline test that confirms that non-bundle-involved deps do use move? Might be a good first step towards getting rid of some common.npm calls, assuming it's tested elsewhere (I don't know if it is).

Yup, it's the first test of this. I'm hesitant to add requirements to landing this that aren't necessary for the code itself though, seeing as it's been a low enough priority that it's sat around since 2015. I fear adding more work to it will just mean it'll never land.

Does something as simple as just copy-pasting this test module into a new file, getting rid of the bundledDeps bits, and just changing the final test to check for move seem like a reasonable, quick compromise?

zkat · 2016-12-01T08:05:08Z

test/tap/move-no-clobber-dest-node-modules.js

+    t.is(code, 0, 'installed ok')
+    exists(t, fixturepath('node_modules','moda'), 'moda')
+    exists(t, fixturepath('node_modules', 'modb'), 'modb')
+    exists(t, fixturepath('node_modules', 'modb', 'node_modules', 'modc'), 'modc')


I spent a while staring hard at this, I ran the test without the diff-trees patch, etc, and I don't really understand how this test works. I don't understand why modc is missing without the patch after tracing through the steps as I understand them?

Oh hey, this is like proto-tacks... =D freaky =D

Ok, so I'll walk this through in a follow up comment…

Ok, so to start with after setup completes we have an existing node_modules that looks like this:

npm sill currentTree move-no-clobber-dest-node-modules npm sill currentTree └─┬ moda@1.0.0 npm sill currentTree └─┬ modb@1.0.0 npm sill currentTree └── modc@1.0.0

The only caveat here is that moda@1.0.0 bundles both modb and modc.

Now, our top level module has a package.json that depends on moda@1.0.1 and modb@1.0.0. moda@1.0.1 differs from moda@1.0.0 in that it doesn't depend on anything.

So npm install needs to mutate that existing tree such that the result is valid according to the package.json. With this patch that looks like:

npm sill idealTree move-no-clobber-dest-node-modules npm sill idealTree ├── moda@1.0.1 npm sill idealTree └─┬ modb@1.0.0 npm sill idealTree └── modc@1.0.0

The diff for this is:

npm sill diffTrees remove modc@1.0.0 npm sill diffTrees add modc@1.0.0 npm sill diffTrees remove modb@1.0.0 npm sill diffTrees update moda@1.0.1 npm sill diffTrees add modb@1.0.0

Without this patch, the diff is:

npm sill diffTrees remove modc@1.0.0 npm sill diffTrees add modc@1.0.0 npm sill diffTrees update moda@1.0.1 npm sill diffTrees move modb@1.0.0

That looks ok to me, so I'm a bit baffled too. So long as our order of operations is ok then that shoul dwork fine. So let's dig further into the differences between the decomposed actions. The following is from the patched version:

fetch moda@1.0.1, modb@1.0.0 extract moda@1.0.1, modb@1.0.0 preinstall modc@1.0.0, moda@1.0.1, modb@1.0.0 remove moda@1.0.1 remove modb@1.0.0 remove modc@1.0.0 finalize modc@1.0.0 finalize moda@1.0.1 finalize modb@1.0.0 build modc@1.0.0 build moda@1.0.1 build modb@1.0.0 install modc@1.0.0 install moda@1.0.1 install modb@1.0.0 postinstall modc@1.0.0 postinstall moda@1.0.1 postinstall modb@1.0.0

And this is from the unpatched version:

fetch moda@1.0.1 extract moda@1.0.1 preinstall modc@1.0.0, moda@1.0.1 remove moda@1.0.1 remove modc@1.0.0 move modb@1.0.0 finalize modc@1.0.0 finalize moda@1.0.1 build modc@1.0.0 build moda@1.0.1 build modb@1.0.0 install modc@1.0.0 install moda@1.0.1 install modb@1.0.0 postinstall modc@1.0.0 postinstall moda@1.0.1 postinstall modb@1.0.0

The unpatched version is for some reason skipping fetch/extract for modc which is why it doesn't exist when we go to finalize (we get an ENOENT on rename of a .staging/modulename-deadbeef folder).

Ok, so why is that? Well, decompose-actions has this code:

if (!pkg.fromBundle) { decomposed.push(['fetch', pkg]) decomposed.push(['extract', pkg]) decomposed.push(['test', pkg]) }

And it exists because if we're installing a module FROM a bundle then it's included in that other module's tarball and thus shouldn't be fetched/extracted on its own. That makes sense, what's baffling here however is WHY fromBundled would be set in this case as that's an attribute that's ordinarily ONLY set when installing a new module with bundled dependencies, and none of those add's include bundles.

Digging in here, I think the patch this is testing is just papering over a deeper problem and that may be why I sat on this last year and didn't push it out. But now we have a record of WHY at least. =D

er ah, no I'm blind, I was thinking the copy of modb@1.0.0 didn't bundle dependencies itself, but it does. modb@1.0.0 bundles modc@1.0.0. That's why the tree ends up deep, each layer was bundling its deps.

Ok, so I'm back on being ok with this patch!!

The old behavior:

npm sill diffTrees remove modc@1.0.0 npm sill diffTrees add modc@1.0.0 npm sill diffTrees update moda@1.0.1 npm sill diffTrees move modb@1.0.0

The above can't work because modb bundles modc so trying to add it independently of modb is nosensical. Where as, by ditching the move optimization for modb, we get something that can work:

npm sill diffTrees remove modc@1.0.0 npm sill diffTrees add modc@1.0.0 npm sill diffTrees remove modb@1.0.0 npm sill diffTrees update moda@1.0.1 npm sill diffTrees add modb@1.0.0

modc comes along for the ride with the newly added modb.

An alternative approach would be to get the modc add/remove upgrade to a move as well. That should work. Right now it's being disallowed because we don't want to hoist bundled deps out of their containing module. But if the containing module is going along for the ride to it's fine.

It does work, but writing a guard to allow moves only when the bundling module is moving too is beyond my brain meats at this hour.

coveralls · 2016-12-01T08:16:42Z

Coverage increased (+0.005%) to 85.467% when pulling 113911e on iarna/fix-move-modules into a2a9ba7 on release-next.

@iarna

Moving things into deps that have bundledDeps can clobber things unintentionally! Let's not. PR-URL: #15081 Credit: @iarna Reviewed-By: @zkat

iarna added review target-latest labels Dec 1, 2016

iarna changed the base branch from latest to release-next December 1, 2016 03:22

iarna added 2 commits November 30, 2016 21:28

diff-trees: Stop upgrading adds to moves w/ bundled deps

1c6f894

test: Verify that destination is preserved when moving

113911e

iarna force-pushed the iarna/fix-move-modules branch from e5b39cc to 113911e Compare December 1, 2016 05:31

zkat reviewed Dec 1, 2016

View reviewed changes

zkat assigned iarna Dec 1, 2016

zkat removed the review label Dec 1, 2016

zkat unassigned iarna Dec 1, 2016

zkat added this to the next milestone Dec 1, 2016

zkat merged this pull request into release-next Dec 1, 2016

zkat pushed a commit that referenced this pull request Dec 1, 2016

diff-trees: Stop upgrading adds to moves w/ bundled deps

9776d8f

Moving things into deps that have bundledDeps can clobber things unintentionally! Let's not. PR-URL: #15081 Credit: @iarna Reviewed-By: @zkat

iarna deleted the iarna/fix-move-modules branch March 8, 2017 00:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't try to replace modules added from a bundle with moves from a non-bundle #15081

Don't try to replace modules added from a bundle with moves from a non-bundle #15081

iarna commented Dec 1, 2016

coveralls commented Dec 1, 2016

zkat left a comment

zkat Dec 1, 2016

zkat Dec 1, 2016

iarna Dec 1, 2016

zkat Dec 1, 2016

zkat Dec 1, 2016

iarna Dec 1, 2016

iarna Dec 1, 2016

iarna Dec 1, 2016

iarna Dec 1, 2016

coveralls commented Dec 1, 2016

Don't try to replace modules added from a bundle with moves from a non-bundle #15081

Don't try to replace modules added from a bundle with moves from a non-bundle #15081

Conversation

iarna commented Dec 1, 2016

coveralls commented Dec 1, 2016

zkat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 1, 2016