chore(check-node-modules): make check/reinstall node_modules work across platforms #12792

gkalpak · 2015-09-09T00:58:03Z

The previous implementations (based on shell scripts) threw errors on Windows, because it was not able to rm -rf 'node_modules' (due to the 255 character limit in file-paths).

This implementation works consistently across platforms and is heavily based on 'https://github.com/angular/angular/blob/3b9c08676a4c921bbfa847802e08566fb601ba7a/tools/npm/check-node-modules.js'.

Fixes #11143
Closes #11353

…oss platforms The previous implementations (based on shell scripts) threw errors on Windows, because it was not able to `rm -rf` 'node_modules' (due to the 255 character limit in file-paths). This implementation works consistently across platforms and is heavily based on 'https://github.com/angular/angular/blob/3b9c08676a4c921bbfa847802e08566fb601ba7a/tools/npm/check-node-modules.js'. Fixes angular#11143 Closes angular#11353

gkalpak · 2015-09-09T01:04:58Z

This might also be related to #12649.

TODO:

See what Travis thinks.
Test more thoroughly (espacially on platforms other than Windows).
Decide if it is better to show the output of npm install or hide it and just show a progress indication (printing dots every few seconds).
(Currently, the output is not shown.)
UPDATE: This is now obsolete, since the implementation has changed.

gkalpak · 2015-09-09T09:23:30Z

scripts/npm/check-node-modules.js

+      fs.unlinkSync(curPath);
+    }
+  }
+}


Would we care to use rm -rf on *NIX based systems for performance reasons ?
I didn't bother, because this will be running only rarely and then again npm install is the main bottleneck.

In the initial discussion, we preferred this. But imo we can see if this works, and then change it back afterwards if it's much slower.

Narretz · 2015-09-14T21:10:06Z

I noticed that this changes the behavior - previously, if there was a mismatch between the current and the cached lock file, it would automatically install, now it only warns It should work with the --reinstall option, but can you actually use this from the grunt level? I tried grunt npm:install --reinstall but it didn't work.

petebacondarwin · 2015-09-14T21:11:33Z

The fact that it doesn't automatically reinstall is a good thing. It gives you a chance to finish what you are doing before wiping out all your node_modules. We should make the force reinstallation process reliable though

Narretz · 2015-09-14T21:27:04Z

Hmm I'm getting quite a few of these (on Windows):
Error: ENOTEMPTY, directory not empty 'c:\Users\Martin\static\angular.js\node_modules\angular-benchpress\node_modules\browserify\node_modules\module-deps\node_modules\resolve\test\node_path\y'

It also takes extremly long.

petebacondarwin · 2015-09-14T21:36:08Z

Is that the old path too long problem?

petebacondarwin · 2015-09-14T21:39:42Z

With this PR you force an install by running node scripts/npm/check-node-modules.js --reinstall

petebacondarwin · 2015-09-14T21:41:43Z

scripts/npm/check-node-modules.js

+  if (nodeModulesOk) {
+    console.log(':-) npm dependencies are looking good!');
+  } else {
+    console.warn(':-( npm dependencies are stale or in an unknown state!');


We ought to make this BIGGER and add a message about how to fix it. Something like:

console.warn('====================================================================='); console.warn(':-( npm dependencies are stale or in an unknown state!'); console.warn('You can rebuild the dependencies by running `node scripts/npm/check-node-modules.js --reinstall`'); console.warn('=====================================================================');

Very good idea!

petebacondarwin · 2015-09-14T21:49:07Z

It is working well on my Mac

Narretz · 2015-09-14T21:58:40Z

Strange, Now it finally uninstalled everything and installed. I got this:

Running `npm install` (this may take a while)............................................................................................................................................................................................................................................
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
Operation completed with errors:
[Error: stderr maxBuffer exceeded.]
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

But I still get modules are stale error.
I assume it's because I'm not really running in Windows cmd, but in git bash inside ConEmu. Maybe the problem lies there somewhere.

petebacondarwin · 2015-09-14T22:00:00Z

I think this is because, if npm install failed then the npm-shrinkwrap.json file never gets copied into the node_modules folder, which is what is used to check for out of date dependencies.

petebacondarwin · 2015-09-14T22:03:09Z

Trying on windows 10

petebacondarwin · 2015-09-14T22:14:06Z

Still waiting ...

petebacondarwin · 2015-09-14T22:16:16Z

I just got the same maxBuffer exception. Will look into this tomorrow.

Narretz · 2015-09-14T22:19:55Z

I've also got the buffer thing again. This time I've tried with admin rights, and that fixed the purge errors. Btw, they are actually different than path too long errors, it's probably some process holding on to the file. But if it works as admin, it's fine by me.

petebacondarwin · 2015-09-14T22:22:02Z

I have been looking at the Angular 2 version. They don't bother trying to run npm install from the script.
Instead they have a preinstall hook that purges the old node_modules and a postinstall hook that copies the npm-shrinkwrap file. I quite like this since to ensure you have exactly the right modules you just run npm install and you are done.

gkalpak · 2015-09-15T07:11:42Z

I tried it a few times on Windows 10 and never got that maxBuffer error. (But it is indeed too slow; it's npm install though that takes much time, so I don't think we can do better here.)

BTW (as I'm sure you've already figured out :)), if you uncomment line 125 and line 126 you get to see the stdout and stderr of npm install, which might provide useful contxt for errors (if they happen during npm install).

Could that also help with the stderr maxBuffer exceeded error ?

They don't bother trying to run npm install from the script.
Instead they have a preinstall hook that purges the old node_modules and a postinstall hook that copies the npm-shrinkwrap file.

@petebacondarwin, you mean you like it as a convenience if you want to update the dependencies ?
(I.e. you can run npm install instead of node scripts/npm/check-node-modules.js --reinstall)
Or is there any other benefit as well ?

petebacondarwin · 2015-09-15T11:26:47Z

There are two benefits.

It is intuitive to simply run npm install if you want to refresh the dependencies; rather than having to "know" about the script
It is less magic to have the npm install script run directly rather than using exec() to do it.

gkalpak · 2015-09-15T11:59:26Z

Agreed :)

So, I will make the following changes:

grunt shell:npm-install: Will only check if the modules are up-to-date (via 'npm-shrinkwrap.json') and inform the user (with a BIGGER message in case of outdated deps, instructing them to run npm install to update the deps).
npm's preinstall: Will check if the modules are up-to-date and purge 'node_modules/' if they aren't.
npm's postinstall: Will copy 'npm-shrinkwrap.json' to 'node_modules/npm-shrinkwrap.cached.json'.
In .travis.yml: Will run npm install (instead of calling our script).

petebacondarwin · 2015-09-15T12:10:47Z

Perfick!

Changes: 1. Make warning about stale dependencies stand out. 2. Check 'node_modules/' and purge at `preinstall` phase. 3. Copy 'npm-shrinkwrap.json' at `postinstall` phase. 4. Just run `npm install` to install dependencies (e.g. in '.travis.yml').

gkalpak · 2015-09-15T15:29:52Z

Added a 2nd (squashable upon merging) commit with the aforementioned changes.

Unfortunately, I am getting errors when purging from npm install (but it works fine when running node scripts/npn/check-node-modules.js --purge directly). It seems like the same ENOTEMPTY error you reported earlier :(

npm install output:

> angularjs@ preinstall C:\...\angular.js
> node scripts/npm/check-node-modules.js --purge

:-( npm dependencies are stale or in an unknown state!
    Purging 'node_modules'...
C:\...\angular.js\scripts\npm\check-node-modules.js:0
(function (exports, require, module, __filename, __dirname) { // Implementatio
^
Error: ENOTEMPTY, directory not empty 'C:\...\angular.js\node_modules\karma\node_modules\chokidar\node_modules'
    at Error (native)
    at Object.fs.rmdirSync (fs.js:711:18)
    at deleteDirSync (C:\...\angular.js\scripts\npm\check-node-modules.js:61:8)
    at deleteDirOrFileSync (C:\...\angular.js\scripts\npm\check-node-modules.js:69:7)
    at Array.forEach (native)
    at deleteDirSync (C:\...\angular.js\scripts\npm\check-node-modules.js:60:26)
    at deleteDirOrFileSync (C:\...\angular.js\scripts\npm\check-node-modules.js:69:7)
    at Array.forEach (native)
    at deleteDirSync (C:\...\angular.js\scripts\npm\check-node-modules.js:60:26)
    at deleteDirOrFileSync (C:\...\angular.js\scripts\npm\check-node-modules.js:69:7)

npm ERR! Windows_NT 6.3.9600
npm ERR! argv "C:\\Program Files\\nodejs\\\\node.exe" "C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "install"
npm ERR! node v0.12.7
npm ERR! npm  v2.11.3
npm ERR! code ELIFECYCLE
npm ERR! angularjs@ preinstall: `node scripts/npm/check-node-modules.js --purge`
npm ERR! Exit status 1
...

It fails consistently, but on a different directory each time.
I'll look into it tomorrow (unless anyone has any insights in the meantime 😄).

Narretz · 2015-09-15T16:16:30Z

I had this error yesterday. Have you tdied running the cmd prompt / shell as admin

gkalpak · 2015-09-17T09:30:59Z

Well, I tried it again today (with user rights - no admin) and it worked like a charm !?
I can't reproduce the ENOTEMPTY error (or any other error).
I also tried on a Linux (Debian) VM a couple of times and didn't run into any issues.

/cc @Narretz , @petebacondarwin

Narretz · 2015-09-22T10:56:40Z

I think we should merge it and see if it causes any issues / makes it better on Windows.

petebacondarwin · 2015-09-22T12:39:49Z

I agree. Let's merge this.

petebacondarwin · 2015-09-22T16:51:41Z

Woohoo!

petebacondarwin · 2015-09-22T16:52:07Z

Can we cherry pick it to 1.3 and 1.4 too @gkalpak ?

gkalpak · 2015-09-22T18:18:13Z

I guess we could (since it only touches "chore" stuff).
Should I go for it right away, or give it a little time to see how it behaves ?

petebacondarwin · 2015-09-22T19:54:43Z

Go for it. I have only seen it fail on windows. And since even that is intermittent, it is better than we have right now.

…oss platforms The previous implementations (based on shell scripts) threw errors on Windows, because it was not able to `rm -rf` 'node_modules' (due to the 255 character limit in file-paths). This implementation works consistently across platforms and is heavily based on 'https://github.com/angular/angular/blob/3b9c08676a4c921bbfa847802e08566fb601ba7a/tools/npm/check-node-modules.js'. Fixes #11143 Closes #11353 Closes #12792

gkalpak · 2015-09-23T20:56:45Z

Backported to:

v1.4.x (9c1f8ea)
v1.3.x (bce1d8e)

petebacondarwin · 2015-09-24T07:23:01Z

Smashing!

googlebot added the cla: yes label Sep 9, 2015

gkalpak added type: chore needs: review component: misc core severity: inconvenient labels Sep 9, 2015

gkalpak reviewed Sep 9, 2015
View reviewed changes

Narretz added this to the 1.4.6 milestone Sep 14, 2015

petebacondarwin mentioned this pull request Sep 14, 2015

refactor npm install (again) #11353

Closed

petebacondarwin reviewed Sep 14, 2015
View reviewed changes

petebacondarwin modified the milestones: 1.4.6, 1.4.7 Sep 17, 2015

petebacondarwin added needs: merging and removed needs: review labels Sep 22, 2015

gkalpak changed the title ~~WIP - chore(check-node-modules): make check/reinstall node_modules work across platforms~~ chore(check-node-modules): make check/reinstall node_modules work across platforms Sep 22, 2015

gkalpak closed this in d3de006 Sep 22, 2015

gkalpak mentioned this pull request Sep 22, 2015

Can't build Cygwin and Windows 10 without --force #12649

Closed

gkalpak deleted the chore-fix-install-dependencies-on-windows branch October 6, 2015 09:51

schmod mentioned this pull request Oct 19, 2015

Cannot run npm install on a newly-cloned repository #13123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(check-node-modules): make check/reinstall node_modules work across platforms #12792

chore(check-node-modules): make check/reinstall node_modules work across platforms #12792

gkalpak commented Sep 9, 2015

gkalpak commented Sep 9, 2015

gkalpak Sep 9, 2015

Narretz Sep 9, 2015

Narretz commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

Narretz commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

petebacondarwin Sep 14, 2015

Narretz Sep 14, 2015

petebacondarwin commented Sep 14, 2015

Narretz commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

Narretz commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

gkalpak commented Sep 15, 2015

petebacondarwin commented Sep 15, 2015

gkalpak commented Sep 15, 2015

petebacondarwin commented Sep 15, 2015

gkalpak commented Sep 15, 2015

Narretz commented Sep 15, 2015

gkalpak commented Sep 17, 2015

Narretz commented Sep 22, 2015

petebacondarwin commented Sep 22, 2015

petebacondarwin commented Sep 22, 2015

petebacondarwin commented Sep 22, 2015

gkalpak commented Sep 22, 2015

petebacondarwin commented Sep 22, 2015

gkalpak commented Sep 23, 2015

petebacondarwin commented Sep 24, 2015

chore(check-node-modules): make check/reinstall node_modules work across platforms #12792

chore(check-node-modules): make check/reinstall node_modules work across platforms #12792

Conversation

gkalpak commented Sep 9, 2015

gkalpak commented Sep 9, 2015

gkalpak Sep 9, 2015

Choose a reason for hiding this comment

Narretz Sep 9, 2015

Choose a reason for hiding this comment

Narretz commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

Narretz commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

petebacondarwin Sep 14, 2015

Choose a reason for hiding this comment

Narretz Sep 14, 2015

Choose a reason for hiding this comment

petebacondarwin commented Sep 14, 2015

Narretz commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

Narretz commented Sep 14, 2015

petebacondarwin commented Sep 14, 2015

gkalpak commented Sep 15, 2015

petebacondarwin commented Sep 15, 2015

gkalpak commented Sep 15, 2015

petebacondarwin commented Sep 15, 2015

gkalpak commented Sep 15, 2015

Narretz commented Sep 15, 2015

gkalpak commented Sep 17, 2015

Narretz commented Sep 22, 2015

petebacondarwin commented Sep 22, 2015

petebacondarwin commented Sep 22, 2015

petebacondarwin commented Sep 22, 2015

gkalpak commented Sep 22, 2015

petebacondarwin commented Sep 22, 2015

gkalpak commented Sep 23, 2015

petebacondarwin commented Sep 24, 2015