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.

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

v0.18.0 postinstall missing dependencies #2142

Closed
daltones opened this issue Dec 4, 2016 · 35 comments · Fixed by #2164
Closed

v0.18.0 postinstall missing dependencies #2142

daltones opened this issue Dec 4, 2016 · 35 comments · Fixed by #2164
Assignees

Comments

@daltones
Copy link

daltones commented Dec 4, 2016

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
I think some dependencies are missing at the time when postinstall scripts are ran.

I'm having this error:

yarn install v0.18.0
info No lockfile found.
warning test: No license field
[1/4] Resolving packages...
[2/4] Fetching packages...
warning [email protected]: The platform "linux" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
[1/3] ⠐ gifsicle
[2/3] ⠐ jpegtran-bin
[3/3] ⠐ optipng-bin
[-/3] ⠐ waiting...
error [my-project]/node_modules/jpegtran-bin: Command failed.
Exit code: 1
Command: sh
Arguments: -c node lib/install.js
Directory: [my-project]/node_modules/jpegtran-bin
Output:
module.js:472
    throw err;
    ^

Error: Cannot find module 'ini'
    at Function.Module._resolveFilename (module.js:470:15)
    at Function.Module._load (module.js:418:25)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> ([my-project]/node_modules/rc/lib/utils.js:3:12)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

In another project, after a yarn upgrade I also encountered this problem with other packages (gifsicle's postinstall missing deep-extend). So it seems like a general problem.

If the current behavior is a bug, please provide the steps to reproduce.
Given this package.json:

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

Run yarn install.

I noticed that if I try to install only one of these two, it will work. I guess that's an issue with shared dependencies between them.

What is the expected behavior?
Installation completes successful.

Please mention your node.js, yarn and operating system version.
node.js: v7.2.0
yarn: v0.18.0
OS: Arch Linux

@olingern
Copy link
Contributor

olingern commented Dec 4, 2016

This appears to be a dependency problem with fsevents. Your OS, Arch Linux, doesn't support it. If you head over to Strongloop's fsevents Github page, you'll see this in their README

Native access to OS X FSEvents in Node.js

The FSEvents API in OS X allows applications to register for notifications of changes to a given directory tree. It is a very fast and lightweight alternative to kqueue.

So, I would say this isn't a Yarn issue. The other errors are probably a result of the previous failing.

@daltones
Copy link
Author

daltones commented Dec 4, 2016

@olingern No, it's just an expected warning. It was always there (even with npm or older versions of yarn).

I pointed a problem of missing dependencies that must have been installed before postinstall.

Just to be clear about the problem with common dependencies (somewhere in the tree of babel-cli and gulp-imagemin):

These two will complete successful:

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

But this will lead to the error:

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

@mvestergaard
Copy link
Contributor

I was able to replicate this on windows. I think it may be related to #2116. I'll try to investigate.

@olingern
Copy link
Contributor

olingern commented Dec 4, 2016

@daltones This isn't a warning, it's an error, but install continues.

info "[email protected]" is an optional dependency and failed compatibility check. *Excluding it from installation.*

That dependency isn't being met because of your OS. This would hold true on Windows, too

Looking a little closer, it is optional, so I'll attempt on OSX as well.

@mvestergaard
Copy link
Contributor

@olingern You're misunderstanding what the problem is.
deep-extend is a transient dependency of fsevents, it is also a dependency of something else. Because fsevents is being skipped, so is deep-extend and therefore something else reliant on it, is failing.

This is a bug.

@daltones
Copy link
Author

daltones commented Dec 4, 2016

@olingern It's just a warning that an optional dependency won't be installed. But the problem is that it messes up with other dependencies required somewhere else.

@olingern
Copy link
Contributor

olingern commented Dec 4, 2016

The package.json configuration you listed above installs without issue on OSX. I still believe this is an OS compatibility issue.

screen shot 2016-12-04 at 2 09 39 pm

@mvestergaard
Copy link
Contributor

@olingern fsvents isn's skipped on OSX so you can't replicate it.

I'm attempting to write a test that catches this problem. I know this worked with #1997 but it's broken with #2116

@olingern
Copy link
Contributor

olingern commented Dec 4, 2016

@daltones @mvestergaard I see. You guys are right. It looks like dependencies are being ignored when an optional doesn't resolve.

@mvestergaard
Copy link
Contributor

It's proving difficult to replicate in a test though, it seems like it has to go multiple levels deep. I tried this contrived example:

dep-a - optional (incompatible with any os)
  - dep-c
dep-b
  - dep-c

But that works fine. So I'm going to put the shared dependency deeper and see if that's causing it.

@olingern
Copy link
Contributor

olingern commented Dec 4, 2016

@mvestergaard Since I was able to install on OSX, looking through the lockfile made figuring the dependency tree easier for this. Interestingly enough, it seems to be rooted in multi-leveled optional dependencies

- babel-cli
  - chokidar 
    - fsevents   
      - node-pre-gyp
        - rc
          - ini

Important to note above, chokidar and fsevents are optional dependencies

@mvestergaard
Copy link
Contributor

mvestergaard commented Dec 4, 2016

I've also found that:

- a (optional, incompatible)
  - b
    - c
      - g
- d
  - e
    - f
      - g

Results in: a, c, d, e, f, g being installed, only b is skipped. It should be d, e, f, g.
I'm trying to write some tests that points me in the right direction, but I find it a bit difficult as I can't seem to figure out how to setup tooling to be able to debug yarn.

The above was my bad, that works.

I've sort of given up on writing a test that replicates this across all platforms. Seems like some sort of edge case.
@kittens @bestander maybe you guys can look into this?

@fernandofleury
Copy link

In case someone is looking for a temporarily solution, executing a "vanilla" npm install will do the trick.

@bestander
Copy link
Member

Anyone eager to send a PR or a PR with a breaking unit test for starters?

@ai
Copy link

ai commented Dec 5, 2016

I have same Error: Cannot find module 'ini' issue after yarn install, but for global-prefix dependency.

Everything if fine on Fedora Desktop, but fails on Ubuntu 16.04.1 server (both system uses same yarn 0.17.10 and node 7.2 versions).

@daltones
Copy link
Author

daltones commented Dec 5, 2016

I'm sorry I can't contribute with formal tests or implementation. But maybe this simplified tree could help:

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]

So the pattern I found was:

  • If some optional dependency is incompatible, it will be potentially skipped with all children dependencies.
  • Skipped dependencies could be rescued. But just the first level.
  • We can see that rc would be skipped, but it is required somewhere else. So it is rescued.
  • deep-extend, ini, strip-json-comments are the second level of dependencies that had to be rescued. They aren't required in anywhere else. The problem of missing dependencies starts here.
  • minimist is rescued, because unlike the other three it is required somewhere else. So it's the same case of rc, but one level down.
  • minimist has no dependencies at all. But if it had, I can suppose all of them would be wrongly skipped if they were required just by minimist itself.

@gaearon
Copy link
Contributor

gaearon commented Dec 5, 2016

Another example tree with this issue:

[email protected] /Users/dan/p/create-react-app/packages/react-scripts
├─┬ [email protected]
│ └─┬ [email protected]
│   └─┬ [email protected]
│     └─┬ [email protected]
│       └─┬ [email protected]
│         └── [email protected] 
└─┬ [email protected]
  └─┬ [email protected]
    └─┬ [email protected]
      └─┬ [email protected]
        └─┬ [email protected]
          └─┬ [email protected]
            └── [email protected] 

If fsevents gets skipped, so does extend, and Jest (which also needs extend) fails.
You can see this in our CI log (which only fails for Yarn, and only since 0.18).

@mvestergaard
Copy link
Contributor

mvestergaard commented Dec 5, 2016

@gaearon @daltones Those dependency trees can probably be very helpful. Thanks.
I think I'll try simulating one of them 1:1 in a test when I get a few.

@mvestergaard
Copy link
Contributor

I've attempted to replicate this using the graph provided by @gaearon .. but the test passes.. so I'm still pretty unsure what causes this.

I've used the exact version numbers from the graph, including two different versions of request, fsevents as an optional dependency of chokidar, and with an invalid os.

Everything except fsevents, node-pre-gyp, and [email protected] is installed as expected.

The test is available here: https://github.com/mvestergaard/yarn/commit/9521157ace19bb4d42240532091d8b5402b05ef5 perhaps it can help someone else replicate it.

@gaearon
Copy link
Contributor

gaearon commented Dec 5, 2016

Oh, I just realized my tree is from a working build (locally). So maybe CI has a different tree.
If you'd like to play with a repro, you can make a PR to Create React App that:

  1. Reverts Fix end-to-end test on Yarn facebook/create-react-app#1167
  2. Modifies the place where we call yarn for the first time to add any additional arguments
  3. Adds any logs to our bash script here (e.g. you can npm ls there)

Then you can send the PR and see the output in the Travis logs.


Updated: I created facebook/create-react-app#1170, let's see how it goes.

@gaearon
Copy link
Contributor

gaearon commented Dec 5, 2016

Can you help me by specifying what would be good diagnostic commands to run on the failing environment? npm ls extend wasn't very helpful, apparently because extend wasn't there..

@mvestergaard
Copy link
Contributor

I wish I could. I'm really just a concerned citizen trying to help out 😊
Don't you facebook guys have a slack channel or something you can ping one of the maintainers in? 😁

@bestander
Copy link
Member

bestander commented Dec 5, 2016 via email

@mvestergaard
Copy link
Contributor

@bestander Do you know a way to debug yarn? I may be able to step through it on my windows machine and see if I can find the cause that way.

@olingern
Copy link
Contributor

olingern commented Dec 5, 2016

@mvestergaard You can attach to a process with vscode or debug using node-inspector

@mvestergaard
Copy link
Contributor

Yea i tried, but got some minified code. I'll give it another shot.

@olingern
Copy link
Contributor

olingern commented Dec 5, 2016

@mvestergaard
npm install -g node-inspector

In your Yarn directory:
npm run watch
node-debug bin/yarn.js

That works for me. Not minified.

You can also set breakpoints inside Chrome http://127.0.0.1:8080/?port=5858 in the lib folders to step through. I've found callbacks can get painful, though.

@bestander
Copy link
Member

bestander commented Dec 6, 2016 via email

@torifat
Copy link
Member

torifat commented Dec 6, 2016

@mvestergaard You have to pass --sourcemaps flag. Either like yarn watch -- --sourcemaps or gulp watch --sourcemaps. Note that you have to remove lib before running watch with --sourcemaps.

@mvestergaard
Copy link
Contributor

mvestergaard commented Dec 6, 2016

Thanks I managed to debug, not with sourcemaps though. But I think I figured out what's going on.

I think I've managed to get this to work, but still haven't been able to replicate in a test though.

In package-hoister.js@115 by skipping ignored packages:

      for (const info of infos) {
        // skip hoisting ignored packages
        if (info.ignore) {
          continue;
        }

        this.hoist(info);
      }

However this breaks the test hoisting should factor ignored dependencies. I think @kittens needs to look into this.

Using @daltones dependency graph above 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;
    }

I hope this makes sense to some of you.

@mvestergaard
Copy link
Contributor

mvestergaard commented Dec 6, 2016

So with that change I was able to install on windows, plus yarn check passes. The only thing is the failing test, but I'm unsure if the test may be wrong?

It says this in the test, so I'm not gonna touch it:

  // you should only modify this test if you know what you're doing
  // when we calculate hoisting we need to factor in ignored dependencies in it
  // so we get deterministic hoisting across environments, for example in production mode
  // we should still be taking dev dependencies into consideration

It appears that installed packages align with npm after the change above.
After installing with yarn and then running npm install and npm prune make no changes to installed packages.

Meanwhile without the fix npm install results in:

The change is available in https://github.com/mvestergaard/yarn/commit/9c8426c5d3331249cb5971115cfc0a3cba0d59a9
I'm not sure if I should create a PR, since i know tests are broken.

@wyze
Copy link
Member

wyze commented Dec 6, 2016

@mvestergaard Nice work. I would submit the PR with the change that fixes the issue. Discussion on the implementation/failing test can happen on that PR.

@mvestergaard
Copy link
Contributor

@wyze Done #2164

@jkrems
Copy link
Contributor

jkrems commented Dec 13, 2016

I think this was accidentally closed by merging #2164.

@bestander
Copy link
Member

Fix is coming

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

Successfully merging a pull request may close this issue.

10 participants