Stop dropping failed optional deps

[iarna]

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.

[felixrieseberg]

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

[thetrompf]

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

[iarna]

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

[zkat]

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

[iarna]

Merged as 0bea588 into release-next.

[Cito]

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]

@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]

@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]

@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 Stop dropping failed optional deps #19054 : 60% Warn rate 😢
• Fix Rollback failed optional installs only once #19042 : 100% Success rate ✔️

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

[reduckted]

@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]

@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]

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]

@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]

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]

@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?

[Sauraus]

#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

Please consider #19042 as a permanent fix.

[alexeygt]

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