Skip to content
This repository has been archived by the owner. It is now read-only.

Stop dropping failed optional deps #19054

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
@iarna
Copy link
Contributor

iarna commented Nov 3, 2017

Makes full package-locks even the package.json has optional deps that are currently uninstallable. Also stops deleting optionals from package-locks when we can't install them.

@iarna iarna requested a review from npm/cli-team as a code owner Nov 3, 2017

@felixrieseberg

This comment has been minimized.

Copy link
Contributor

felixrieseberg commented Nov 3, 2017

Rebecca! On behalf of everyone at Slack building a cross-platform app with one package.json:

@zkat

This comment has been minimized.

Copy link
Member

zkat commented on lib/install.js in 4f7ada6 Nov 3, 2017

Doesn't uniq do identity comparisons? Actions are arrays, right?

@thetrompf

This comment has been minimized.

Copy link

thetrompf commented Nov 7, 2017

Thank you so much! Will this make it for 5.5.x?

@iarna

This comment has been minimized.

Copy link
Contributor Author

iarna commented Nov 16, 2017

@zkat That uniq is running over a map that maps to the pkg objects, which have a shared identity.

@zkat

This comment has been minimized.

Copy link
Member

zkat commented Nov 16, 2017

Ah! I missed where the parens were for that. 👍👍👍 To this

finalize: failed removes in the rollback shouldn't fail the install
As a rule this only happens if it doesn't exist, though it could also be
Windows holding a file open. Either way it shouldn't break the install.

@iarna iarna referenced this pull request Nov 16, 2017

Closed

ENOENT during npm install #9633

iarna added a commit that referenced this pull request Nov 16, 2017

iarna added a commit that referenced this pull request Nov 16, 2017

iarna added a commit that referenced this pull request Nov 16, 2017

iarna added a commit that referenced this pull request Nov 16, 2017

iarna added a commit that referenced this pull request Nov 16, 2017

iarna added a commit that referenced this pull request Nov 16, 2017

finalize: failed removes in the rollback shouldn't fail the install
As a rule this only happens if it doesn't exist, though it could also be
Windows holding a file open. Either way it shouldn't break the install.

Credit: @iarna
Reviewed-By: @zkat
PR-URL: #19054
@iarna

This comment has been minimized.

Copy link
Contributor Author

iarna commented Nov 16, 2017

Merged as 0bea588 into release-next.

@iarna iarna closed this Nov 16, 2017

@Cito

This comment has been minimized.

Copy link

Cito commented Nov 16, 2017

Please see my comment under #19042: This PR is a great relief already, but still does not solve the issue caused by concurrent removal of nested directories which I am addressing in #19042 as well. It only makes it less frequently by removing the duplicate rollbacks and less annoying by converting the errors to warnings. But they can still happen, and the warning text "this is probably harmless" that now will appear every once in a while for Windows users may still be irritating.

@Karasuni Karasuni referenced this pull request Nov 20, 2017

Closed

npm install fails in windows using fsevents #17671

2 of 4 tasks complete
@Karasuni

This comment has been minimized.

Copy link

Karasuni commented Nov 20, 2017

@redukted Could you run a simulation of this PR similar to https://github.com/reduckted/repro-npm-17671 to see the issue still manifests with this PR?

@reduckted

This comment has been minimized.

Copy link

reduckted commented Nov 20, 2017

@Karasuni I've run that repro on these changes, and while it's better it has DEFINITELY NOT FIXED THE BUG.

Edit: The initial tests I ran were invalid (I hadn't set it up correctly). Here are the correct results...

Since this now ignores the error and produces a warning, I've changed the repro to consider an execution of npm that outputs "EPERM" to be a warning instead of a success.

As I did over in #19042 (comment), I let it do 200 iterations. The results are:

  • Success: 120 times
  • Failure: 0 times
  • Warning: 80 times

An example of one of the warnings is:

npm WARN rollback Rolling back readable-stream@2.2.9 failed (this is probably harmless): EPERM: operation not permitted, lstat 'C:\repro-npm-17671\node_modules\fsevents\node_modules'
@Karasuni

This comment has been minimized.

Copy link

Karasuni commented Nov 20, 2017

@reduckted Thanks so much :)
Yup, that's the exact error I get to see every single day!

So according to your tests:

  • NPM v5.5.1 : 68% Success rate
  • Fix #19054 : 60% Warn rate 😢
  • Fix #19042 : 100% Success rate ✔️

(Please note that these tests are specifically related to concern #17671)

@reduckted

This comment has been minimized.

Copy link

reduckted commented Nov 20, 2017

@Karasuni Sorry, I messed up the first test run 😨. I've updated my comment with the correct test results.

It now doesn't fail, but instead it produces a warning about failing to rollback quite a lot.

@Karasuni

This comment has been minimized.

Copy link

Karasuni commented Nov 20, 2017

@reduckted OK, no problem! I've also updated my comment. From those numbers it looks like the PR triggers the race condition at the same rate but catches it instead of handling it.

@SiggeSeb

This comment has been minimized.

Copy link

SiggeSeb commented Nov 21, 2017

I am curious as to why @Cito 's fix #19042 was closed instead of merged, since it actually resolves our issue instead of displays warnings.

@Cito

This comment has been minimized.

Copy link

Cito commented Nov 21, 2017

@SiggeSeb Actually there are two reasons that cause these errors: 1) duplicate rollbacks, and 2) nested rollbacks. Both PRs solve problem 1) in a similar way. #19054 does not solve 2) but alleviates the pain by turning the error into a warning. #19042 solves 2) by simply skipping nested rollbacks assuming that a rollback of a parent package also rolls back the subpackages anyway. I guess this latter assumption might not always be true if packages implement some special custom rollback methods, and that might be the reason why it was not merged. But I'm not sure.

As I explained in #19042, the default rollback method uses the rimraf and fs-vacuum packages to remove files and empty directories on disk. The problem is that only rimraf is safe for concurrent overlapping remove operations under Windows, by catching EPERM errors. So, as another fix without skipping nested rollbacks, we could make fs-vacuum safe as well, in a similar way as it is done in rimraf (see npm/fs-vacuum#8). This is a bit of work, particularly writing the corresponding tests. I still hope the maintainers of fs-vacuum will work on this, so I don't have to do it ;)

For the time being, #19054 is an acceptable solution. And in the meantime if anybody wants to work on npm/fs-vacuum#8 that would be highly appreciated.

@toebens

This comment has been minimized.

Copy link

toebens commented Nov 23, 2017

it is annoying to see how long windows users are struggling with this kind of npm install issue(s) for months and people dont want to accept merge request(s) because it is not the best solution right of the shelf.
I really would like to have this stuff working finally in npm on windows. Please consider to merge the PR #19042 of @Cito and do a release. Thousands of win users would be very grateful....take care of a better solution later ...if there is one...

@luislobo

This comment has been minimized.

Copy link
Contributor

luislobo commented Dec 26, 2017

@iarna First of all, Merry Christmas! And thanks for all the work you have been doing with the NPM team.
Now to business: can #19042 be reviewed if it's needed?

Also, how one can test #19054? Does npmc have this patch?

sramam added a commit to sramam/typescript-starter-node that referenced this pull request Dec 31, 2017

ci: Attempt to fix cross-platform package-lock problems.
Optional dependencies pre npm@5.6.0 caused troubles. Specifically, fs-events which is MacOSX only.
npm/npm#19054 fixed this. But the npm version installed by default for
various node modules needs to be upgraded for this to work. We add a pre `npm install` step to do
the needful for both travis-ci & appveyor. YMMV
@Sauraus

This comment has been minimized.

Copy link

Sauraus commented Jan 23, 2018

#17671 is not resolved in 5.6.0, this is from our CI server, the build in the red box were done with npm 5.5.1 and cache verify those above with npm 5.6.0 and NO cache verify, in our case they all fail on fsevents
screen shot 2018-01-23 at 12 58 22 pm
Please consider #19042 as a permanent fix.

@alexeygt

This comment has been minimized.

Copy link

alexeygt commented Feb 8, 2018

This PR does not fix EPERM error in a correct way. Please consider #19042 as a better fix.

npm v5.6.0 still throws same EPERM error:

...
npm info linkStuff requirejs@2.3.5
npm ERR! path D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js.1796620347
npm ERR! code EPERM
npm ERR! errno -4048
npm ERR! syscall rename
npm ERR! Error: EPERM: operation not permitted, rename 'D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js.1796620347' -> 'D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js'
npm ERR! { Error: EPERM: operation not permitted, rename 'D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js.1796620347' -> 'D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js'
npm ERR! cause:
npm ERR! { Error: EPERM: operation not permitted, rename 'D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js.1796620347' -> 'D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js'
npm ERR! errno: -4048,
npm ERR! code: 'EPERM',
npm ERR! syscall: 'rename',
npm ERR! path: 'D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js.1796620347',
npm ERR! dest: 'D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js' },
npm ERR! stack: 'Error: EPERM: operation not permitted, rename 'D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js.1796620347' -> 'D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js'',
npm ERR! errno: -4048,
npm ERR! code: 'EPERM',
npm ERR! syscall: 'rename',
npm ERR! path: 'D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js.1796620347',
npm ERR! dest: 'D:\npm-projects\fsm\FlashSetManager.FrontEnd\node_modules\requirejs\bin\r.js',
npm ERR! parent: 'flash-set-manager' }
npm ERR!
npm ERR! Please try running this command again as root/Administrator.

npm ERR! A complete log of this run can be found in:
npm ERR! D:\npm-projects\npm-cache_logs\2018-02-08T19_39_35_541Z-debug.log

babichea@USPLMWN116020 MINGW64 /d/npm-projects/fsm/FlashSetManager.FrontEnd (tokens-login-logout)

$ npm -v
5.6.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.