Fix an issue in package hoisting

[mvestergaard]

Summary

This is an attempt to fix #2142

Using this package.json

{
  "name": "test",
  "devDependencies": {
    "babel-cli": "^6.18.0",
    "gulp-imagemin": "^3.1.1"
  }
}

Should create a scenario along the lines of this (on any platform except OSX):

babel-cli [installed]
└─ chokidar [installed]
   └─ fsevents [skipped, potentially the whole tree]
      └─ node-pre-gyp [skipped good]
         └─ rc [not skipped, good]
            ├─ deep-extend [skipped, bad!]
            ├─ ini [skipped, bad!]
            ├─ minimist [not skipped, good]
            └─ strip-json-comments [skipped, bad!]

gulp-imagemin [installed]
└─ imagemin-jpegtran [installed]
   └─ jpegtran-bin [installed]
      └─ bin-build [installed]
         └─ download [installed]
            └─ caw [installed]
               └─ get-proxy [installed]
                  └─ rc [installed]
                     ├─ deep-extend [missing]
                     ├─ ini [missing]
                     ├─ minimist [installed]
                     └─ strip-json-comments [missing]

[...]
{various-other-packages} [installed]
└─ minimist [installed]

My findings from debugging is that this happens because of the following:

It seems like what happens is that rc is ignored in the fsevents branch.

This causes get-proxy#rc to be removed from the tree in the method hoist@283: this.tree.delete(key); and when it looks at the dependency deep-extend in _seed@141 it will skip it because get-proxy#rc was removed from the tree:

      if (!this.tree.get(parent.key)) {
        return null;
      }

get-proxy#rc is removed from the tree due to:

    if (duplicate) {
      info.addHistory(`Satisfied from above by ${newKey}`);
      this.declareRename(info, rawParts, parts);
      return;
    }

With this change, installed packages align with installiation through npm. npm install and npm prune yield no changes.

At this time, this seems to break the test hoisting should factor ignored dependencies.

Test plan

I have not been able to replicate the original scenario in a test.
I've made multiple attempts including https://github.com/mvestergaard/yarn/commit/9521157ace19bb4d42240532091d8b5402b05ef5

[bestander]

Existing tests are failing

[mvestergaard]

Yep I know. The test hoisting should factor ignored dependencies may need to be refactored, unless my fix is completely wrong.

[bestander]

You probably should just add a new test with your example and test that all expected folders are in place.

  "babel-cli": "^6.18.0",
  "gulp-imagemin": "^3.1.1"

We cache network requests, so no need to worry about them.

[mvestergaard]

@bestander It won't fail on OSX. So it's not very reliable.

[bestander]

We have a few tests that are skipped on some platforms. Clout CI run Windows and Linux, so it should be fine. An isolated custom test is always better but real world examples are good too

…

On Wed, 7 Dec 2016 at 12:42, Mathias Vestergaard ***@***.***> wrote: @bestander <https://github.com/bestander> It won't fail on OSX. So it's not very reliable. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2164 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWLNPLUjJcOunV8r5PUgZz8HustzZks5rFqmogaJpZM4LFbCu> .

[mvestergaard]

Fair enough. I'll add a test later.
Looking into whether the hoisting should factor ignored dependencies test should be refactored is not a task I feel I have enough knowledge of the internals to do however.

[bestander]

It is ok to do it in steps. If you add a test that fails properly we can merge it and then do a new PR that fixes the issue based on the test

…

On Wed, 7 Dec 2016 at 12:51, Mathias Vestergaard ***@***.***> wrote: Fair enough. I'll add a test later. Looking into whether the hoisting should factor ignored dependencies test should be refactored is not a task I feel I have enough knowledge of the internals to do however. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2164 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWKET2Z6sGtJzl5lF9USvn1aFHQdMks5rFqvmgaJpZM4LFbCu> .

[mvestergaard]

@bestander do I need to do anything special to do an actual install in tests?
I seem to be hitting some sort of different registry because i get the following:

    Error: Couldn't find any versions for "babel-register" that matches "^6.18.0". Possible versions: "6.1.4\n6.1.17\n6.1.18\n6.2.0\n6.2.4\n6.3.2\n6.3.13\n6.4.3\n6.5.0\n6.5.0-1\n6.5.1\n6.5.2\n6.6.0\n6.6.5\n6.7.2\n6.8.0\n6.9.0\n6.11.5\n6.11.6\n6.14.0\n6.16.0\n6.16.3"

I tried passing {production: true} as a flag to the install helper. Same result.

Here's the test for reference:

test.concurrent('issue #2142 regression test',
  (): Promise<void> => {
    return runInstall({production: true}, 'install-issue-2142-regression', async (config) => {
      assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'deep-extend')));
      assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'ini')));
      assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'strip-json-comments')));
    });
  });

[bestander]

@mvestergaard, could you commit it here?
I'll have a look

[mvestergaard]

@bestander Done

[bestander]

For tests we cache http requests in tests/fixtures/request-cache so that our CI does not depend on network when running them.
Just remove the file for that package and rerun the test

[mvestergaard]

Ok that helped on my machine. It fails on CI too though.

[QWp6t]

I was having this issue as well. Just tested your fix and it worked for me on Windows 10. Optional dependency fsevents was being skipped, which was causing all of the imagemin plugins to fail due to ini and other dependencies of fsevents also being skipped.

I nuked all caches and my entire yarn installation before and after applying the patch to confirm that it was this patch that resolved my issue.

[mvestergaard]

@bestander I've modified the hoisting should factor ignored dependencies test to something I believe is more correct. Can you please take a look?
I've added comments that explain the reasoning behind the changes.

[mvestergaard]

@bestander Have you had a chance to look at this yet? Feedback would be appreciated.

[bestander]

@mvestergaard will look right now, thanks a lot!

[bestander]

I am not 100% sure when we want to split all tests into separate files.
How about moving it into integration.js for now?
Could you also give it a descriptive name so that people don't go to the web searching what issue 2142 was about?

[mvestergaard]

@bestander Done. Having it in it's own file was mostly for my own convenience, so fair point.

[bestander]

That is the idea of Yarn - it keeps packages hoisting deterministic between normal and production build.
The test checked that hoisting of C should be stable and I think it was correct

[mvestergaard]

In that case, my attempt at a fix probably won't work.

The root problem from what I could find is when you have two different packages with dependencies required by both. When one of the parent packages are ignored, either by --production flag or being incompatible, it ends up skipping the shared dependencies because the ignored package goes through the hoisting logic, and the dependency thereby end up being skipped due to being seen as a duplicate, even though it hasn't been installed at all.

[bestander]

Let's undo your code change and merge your test as disabled in the meantime. We'll come up with a fix in the next few days.

…

On Mon, 12 Dec 2016 at 19:09, Mathias Vestergaard ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In __tests__/commands/install/integration.js <#2164>: > assert.ok( !await fs.exists(path.join(config.cwd, 'node_modules', 'b')), ); - assert.ok( - !await fs.exists(path.join(config.cwd, 'node_modules', 'c')), + // c is only depended upon by d in version 2.0.0, so it should be hoisted + assert.equal( + (await fs.readJson(path.join(config.cwd, 'node_modules', 'c', 'package.json'))).version, In that case, my attempt at a fix probably won't work. The root problem from what I could find is when you have two different packages with dependencies required by both. When one of the parent packages are ignored, either by --production flag or being incompatible, it ends up skipping the shared dependencies because the ignored package goes through the hoisting logic, and the dependency thereby end up being skipped due to being seen as a duplicate, even though it hasn't been installed at all. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2164>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWMnNbKLm0A1fwgkEpdp77QAFrykCks5rHZvggaJpZM4LFbCu> .

[mvestergaard]

@bestander I reverted the changes, and set the new test to .skip