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

ENOENT and ENOTEMPTY while rolling back failed optional dependencies #6043

Closed
othiym23 opened this Issue Aug 26, 2014 · 31 comments

Comments

Projects
None yet
9 participants
@othiym23
Contributor

othiym23 commented Aug 26, 2014

npm install grunt-imagemin fails spectacularly on both master and v1.4 due to npm sometimes simultaneously trying to install and remove packages when optional dependencies fail to install. That makes me sad.

See imagemin/imagemin#53 for more details.

@brykov

This comment has been minimized.

Show comment
Hide comment
@brykov

brykov Aug 28, 2014

I'm suffering from these errors for several days already trying to install gulp-imagemin

brykov commented Aug 28, 2014

I'm suffering from these errors for several days already trying to install gulp-imagemin

@gabegorelick

This comment has been minimized.

Show comment
Hide comment
@gabegorelick

gabegorelick Sep 3, 2014

Is there an ETA on a fix? I see it's in progress.

Is there an ETA on a fix? I see it's in progress.

@kole

This comment has been minimized.

Show comment
Hide comment
@kole

kole Sep 4, 2014

+1 on the ETA request

kole commented Sep 4, 2014

+1 on the ETA request

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Sep 4, 2014

Contributor

This will be fixed before npm@2.0.0 is released, as it's the primary blocker for a 2.0 release. I would like to land it today, but the fix is relatively complex and because this bug is a regression caused by another fix, I want to make sure that it's not causing regressions itself. So, it'll be done when it's done?

Contributor

othiym23 commented Sep 4, 2014

This will be fixed before npm@2.0.0 is released, as it's the primary blocker for a 2.0 release. I would like to land it today, but the fix is relatively complex and because this bug is a regression caused by another fix, I want to make sure that it's not causing regressions itself. So, it'll be done when it's done?

@vladikoff

This comment has been minimized.

Show comment
Hide comment
@vladikoff

vladikoff Sep 6, 2014

Please backport the fix to 1.4.

Please backport the fix to 1.4.

@brittanystoroz brittanystoroz referenced this issue in mozilla/generator-recroom Sep 8, 2014

Closed

Fix grunt dependencies for npm 2.0.0 #18

isaacs added a commit that referenced this issue Sep 10, 2014

wip: Wait until end-of-process to handle rollbacks from install
This ought to nail the ENOENT issues we're seeing in #6043,
albeit in a somewhat kludgey way.

Needs tests, ideally, but this is a tricky one to test for.

@sebdeckers sebdeckers referenced this issue in drywallio/drywall-web Sep 10, 2014

Closed

Build failing in gifsicle #75

isaacs added a commit that referenced this issue Sep 12, 2014

wip: Wait until end-of-process to handle rollbacks from install
This ought to nail the ENOENT issues we're seeing in #6043,
albeit in a somewhat kludgey way.

Needs tests, ideally, but this is a tricky one to test for.

isaacs added a commit that referenced this issue Sep 12, 2014

wip: Wait until end-of-process to handle rollbacks from install
This ought to nail the ENOENT issues we're seeing in #6043,
albeit in a somewhat kludgey way.

Needs tests, ideally, but this is a tricky one to test for.

isaacs added a commit that referenced this issue Sep 13, 2014

Defer rollbacks to end of install.
This ought to nail the ENOENT issues we're seeing in #6043 by deferring
all rollback unbuilds to the end of the install process. This also
depends on the upgrade to slide@1.1.5.

Includes a test with a very racy optional dependency situation. (*wink*)

isaacs added a commit that referenced this issue Sep 13, 2014

Defer rollbacks to end of install.
This ought to nail the ENOENT issues we're seeing in #6043 by deferring
all rollback unbuilds to the end of the install process. This also
depends on the upgrade to slide@1.1.6.

The test lives on master, but wasn't ported to this branch.
@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Sep 13, 2014

Contributor

@isaacs and I have addressed this issue two ways:

  1. We changed slide's asyncMap function to only continue on to its final callback when all of the map calls have completed. It used to invoke the callback as soon as it had an error, which caused bad things to happen when the callback had side effects (like wiping out module directories) that other callbacks, that hadn't finished running yet, could be affected by.
  2. All rollbacks are now queued until the npm CLI process is ready to exit, at which point those directories are unbuilt. This also helps prevent race conditions when rolling back failed installations of optionalDependencies.

These changes are included in npm@2.0.0, which is currently labeled npm@next. Unless we find any showstoppers, it will be set to latest next Thursday. As requested, these two fixes have also been backported to the v1.4 branch, and are included in npm@1.4.28 (even though that version probably will not ever be published as latest).

Because npm@2.0.0 and npm@1.4.28 are not tagged as the latest releases of npm (yet), you will need to install them explicitly to test this fix:

npm -g install npm@2.0.0

OR

npm -g install npm@1.4.28

Please do so, and let us know if you're still having problems with this issue. We are reasonably confident we've put a stake through this one's heart, but if you have evidence otherwise, we want to know about it sooner rather than later.

Contributor

othiym23 commented Sep 13, 2014

@isaacs and I have addressed this issue two ways:

  1. We changed slide's asyncMap function to only continue on to its final callback when all of the map calls have completed. It used to invoke the callback as soon as it had an error, which caused bad things to happen when the callback had side effects (like wiping out module directories) that other callbacks, that hadn't finished running yet, could be affected by.
  2. All rollbacks are now queued until the npm CLI process is ready to exit, at which point those directories are unbuilt. This also helps prevent race conditions when rolling back failed installations of optionalDependencies.

These changes are included in npm@2.0.0, which is currently labeled npm@next. Unless we find any showstoppers, it will be set to latest next Thursday. As requested, these two fixes have also been backported to the v1.4 branch, and are included in npm@1.4.28 (even though that version probably will not ever be published as latest).

Because npm@2.0.0 and npm@1.4.28 are not tagged as the latest releases of npm (yet), you will need to install them explicitly to test this fix:

npm -g install npm@2.0.0

OR

npm -g install npm@1.4.28

Please do so, and let us know if you're still having problems with this issue. We are reasonably confident we've put a stake through this one's heart, but if you have evidence otherwise, we want to know about it sooner rather than later.

@gabegorelick

This comment has been minimized.

Show comment
Hide comment
@gabegorelick

gabegorelick Sep 13, 2014

You guys are awesome. Seriously, thanks for backporting the fix to 1.4.

It used to invoke the callback as soon as it had an error, which caused bad things to happen when the callback had side effects

async does this too, and it always bites me. I usually end up resorting to promises and .settle. Glad you were able to fix it for slide.

You guys are awesome. Seriously, thanks for backporting the fix to 1.4.

It used to invoke the callback as soon as it had an error, which caused bad things to happen when the callback had side effects

async does this too, and it always bites me. I usually end up resorting to promises and .settle. Glad you were able to fix it for slide.

@vladikoff

This comment has been minimized.

Show comment
Hide comment
@vladikoff

vladikoff Sep 13, 2014

1.4.28 seems to work, thanks for porting the fix. However, could you please clarify which npm 2 should we test?
npm -g install npm@2.0.0 or npm -g install npm@next or npm -g install npm@latest?

1.4.28 seems to work, thanks for porting the fix. However, could you please clarify which npm 2 should we test?
npm -g install npm@2.0.0 or npm -g install npm@next or npm -g install npm@latest?

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Sep 14, 2014

Contributor

@vladikoff npm@2.0.0 (which, for now, is the same as npm@next). npm@latest (which is the same as plain ol npm -g install npm) points at npm@1.4.27 right now; next Thursday it will be pointed at npm@2.0.0.

Contributor

othiym23 commented Sep 14, 2014

@vladikoff npm@2.0.0 (which, for now, is the same as npm@next). npm@latest (which is the same as plain ol npm -g install npm) points at npm@1.4.27 right now; next Thursday it will be pointed at npm@2.0.0.

@budmc29

This comment has been minimized.

Show comment
Hide comment
@budmc29

budmc29 Sep 24, 2014

Hello, i installed yeoman-webapp today and when i run grunt serve, grunt test or grunt it still displays the error:
Loading "imagemin.js" task...ERROR

Error: Cannot find module 'imagemin-gifsicle'.

npm --version is 1.4.28

I tried all the commands i see on #6043 and imagemin/imagemin#53.
I am new to grunt, so please excuse me if i don't know how to explain better. I took the steps on the yeoman getting started page without modifying anything.

budmc29 commented Sep 24, 2014

Hello, i installed yeoman-webapp today and when i run grunt serve, grunt test or grunt it still displays the error:
Loading "imagemin.js" task...ERROR

Error: Cannot find module 'imagemin-gifsicle'.

npm --version is 1.4.28

I tried all the commands i see on #6043 and imagemin/imagemin#53.
I am new to grunt, so please excuse me if i don't know how to explain better. I took the steps on the yeoman getting started page without modifying anything.

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Sep 25, 2014

Contributor

@Chirica-Mugurel The issue you're running into is actually fixed by the resolution to #5920. To get the fix, you'll need to upgrade to npm@2.0.2: npm install -g npm@2.0.2. See #5920 for details on that issue.

Contributor

othiym23 commented Sep 25, 2014

@Chirica-Mugurel The issue you're running into is actually fixed by the resolution to #5920. To get the fix, you'll need to upgrade to npm@2.0.2: npm install -g npm@2.0.2. See #5920 for details on that issue.

@kuhnroyal

This comment has been minimized.

Show comment
Hide comment
@kuhnroyal

kuhnroyal Sep 27, 2014

Still having that problem.
npm_problem

Still having that problem.
npm_problem

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Sep 27, 2014

Contributor

@kuhnroyal npm is doing what it's supposed to in this case – the optional dependencies failed to install, it warned you, and then it finished running without crashing.

Do you have a C compiler installed? All of imagemin's optional dependencies require developer tools to be installed.

Also, pasting a screenshot is the least helpful way to get support. It cuts off useful information and isn't searchable (not to mention being hard to read).

Contributor

othiym23 commented Sep 27, 2014

@kuhnroyal npm is doing what it's supposed to in this case – the optional dependencies failed to install, it warned you, and then it finished running without crashing.

Do you have a C compiler installed? All of imagemin's optional dependencies require developer tools to be installed.

Also, pasting a screenshot is the least helpful way to get support. It cuts off useful information and isn't searchable (not to mention being hard to read).

@kuhnroyal

This comment has been minimized.

Show comment
Hide comment
@kuhnroyal

kuhnroyal Sep 27, 2014

Right, i'll write it next time.
GCC is installed, should be sufficient?

Right, i'll write it next time.
GCC is installed, should be sufficient?

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Sep 27, 2014

Contributor

@kuhnroyal That's an issue for the imagemin team. I'm just pointing out that npm is doing its job and you're not encountering the issue the others were.

Contributor

othiym23 commented Sep 27, 2014

@kuhnroyal That's an issue for the imagemin team. I'm just pointing out that npm is doing its job and you're not encountering the issue the others were.

@addyosmani addyosmani referenced this issue in google/web-starter-kit Oct 17, 2014

Merged

Fixes #82 - use LibSass instead of Ruby Sass #484

@kevva

This comment has been minimized.

Show comment
Hide comment
@kevva

kevva Oct 18, 2014

@othiym23, they don't require anything. Only if the pre-built binary doesn't work. Then it'll try and compile if from source.

I'm seeing (but don't experiencing it myself) a lot of issues regarding the optional deps. It comes down (I think) to npm running the postinstall scripts before all the dependencies are actually installed, and therefore errors are happening (see the npm install stuff in https://travis-ci.org/google/web-starter-kit/builds/38239448).

Is this fixed in 2.1.5? It seems like it's still happening in 2.1.0.

kevva commented Oct 18, 2014

@othiym23, they don't require anything. Only if the pre-built binary doesn't work. Then it'll try and compile if from source.

I'm seeing (but don't experiencing it myself) a lot of issues regarding the optional deps. It comes down (I think) to npm running the postinstall scripts before all the dependencies are actually installed, and therefore errors are happening (see the npm install stuff in https://travis-ci.org/google/web-starter-kit/builds/38239448).

Is this fixed in 2.1.5? It seems like it's still happening in 2.1.0.

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Oct 18, 2014

Contributor

@kevva If you look at the release notes for 2.1.x release, you'll see that all of

include patches for race conditions. I don't believe that we've eliminated all of the race conditions within npm, but I do believe we've fixed the ones most developers are ever likely to hit, including all of the problems that were causing problems with grunt-contrib-imagemin.

The bugs were due to lock contention, packages being written to and read from the cache non-atomically, and peerDependencies causing multiple build phases to be run for the same package at the same time during a single install, which sometimes led to the ENOENT issues seen here. One of @dylang's coworkers may be seeing a lingering issue related to this, but at this point I've been over the relevant cache and installation code so closely that it's hard for me to imagine that any serious problems could be remaining. There is very little locking being done by npm anymore, and we've taken great pains to avoid allowing the installer to do the same thing at the same time.

@iarna is also reworking the installer's internals right now to make the installer run in discrete stages, in a way that should be more deterministic and less liable to lead to the kinds of race conditions we've spent so much time on lately. It's our hope that this will help us tackle a whole host of issues at once.

Contributor

othiym23 commented Oct 18, 2014

@kevva If you look at the release notes for 2.1.x release, you'll see that all of

include patches for race conditions. I don't believe that we've eliminated all of the race conditions within npm, but I do believe we've fixed the ones most developers are ever likely to hit, including all of the problems that were causing problems with grunt-contrib-imagemin.

The bugs were due to lock contention, packages being written to and read from the cache non-atomically, and peerDependencies causing multiple build phases to be run for the same package at the same time during a single install, which sometimes led to the ENOENT issues seen here. One of @dylang's coworkers may be seeing a lingering issue related to this, but at this point I've been over the relevant cache and installation code so closely that it's hard for me to imagine that any serious problems could be remaining. There is very little locking being done by npm anymore, and we've taken great pains to avoid allowing the installer to do the same thing at the same time.

@iarna is also reworking the installer's internals right now to make the installer run in discrete stages, in a way that should be more deterministic and less liable to lead to the kinds of race conditions we've spent so much time on lately. It's our hope that this will help us tackle a whole host of issues at once.

@kevva

This comment has been minimized.

Show comment
Hide comment
@kevva

kevva Oct 21, 2014

@othiym23, yo, thanks for your answer. Yeah, I know a lot of work were done to fix this, but it seems to fail so randomly at random versions. A few days ago this happened during Travis builds (using npm 1.4.28) but somehow that magically got fixed.

Now I'm seeing issues being created where their logs just say npm WARN optional dep failed, continuing imagemin-optipng@3.1.0 without even running the postinstall script from the looks of it.

I feel kinda bad for just having to advice people to update npm etc lol, but without any real errors there's really not much else I can do.

kevva commented Oct 21, 2014

@othiym23, yo, thanks for your answer. Yeah, I know a lot of work were done to fix this, but it seems to fail so randomly at random versions. A few days ago this happened during Travis builds (using npm 1.4.28) but somehow that magically got fixed.

Now I'm seeing issues being created where their logs just say npm WARN optional dep failed, continuing imagemin-optipng@3.1.0 without even running the postinstall script from the looks of it.

I feel kinda bad for just having to advice people to update npm etc lol, but without any real errors there's really not much else I can do.

@kevva kevva referenced this issue in google/web-starter-kit Oct 21, 2014

Closed

fresh build fails on imagemin-gifsicle module #490

@kevva

This comment has been minimized.

Show comment
Hide comment
@kevva

kevva Oct 21, 2014

Ok, so we managed to track down the issue to a deeply nested dependency relying on a git remote URL which caused npm to fail because of EACCESS errors in ~.npm/_git-remotes/_templates. Not related to this issue so ignore my previous comment.

kevva commented Oct 21, 2014

Ok, so we managed to track down the issue to a deeply nested dependency relying on a git remote URL which caused npm to fail because of EACCESS errors in ~.npm/_git-remotes/_templates. Not related to this issue so ignore my previous comment.

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Oct 22, 2014

Contributor

@kevva what version of npm did you encounter that with? I recently landed a change that disabled templates altogether, so if you're seeing issues in you git repos, we should take another look at that.

Contributor

othiym23 commented Oct 22, 2014

@kevva what version of npm did you encounter that with? I recently landed a change that disabled templates altogether, so if you're seeing issues in you git repos, we should take another look at that.

@kevva

This comment has been minimized.

Show comment
Hide comment
@kevva

kevva Oct 22, 2014

@othiym23, I didn't encounter it but it happened with 2.1.4 and 2.1.5 (because of this). I'm also using 2.1.5 and it's not happening for me.

kevva commented Oct 22, 2014

@othiym23, I didn't encounter it but it happened with 2.1.4 and 2.1.5 (because of this). I'm also using 2.1.5 and it's not happening for me.

@addyosmani

This comment has been minimized.

Show comment
Hide comment
@addyosmani

addyosmani Oct 22, 2014

To add to @kevva's comments, we're starting to run into an increasing numbers of folks being bitten by this but can't reproduce it reliably. e.g 2.1.5 is working fine for me, but on the issue linked above Paul (and outside of that thread, others) are not seeing that build address the problem.

To add to @kevva's comments, we're starting to run into an increasing numbers of folks being bitten by this but can't reproduce it reliably. e.g 2.1.5 is working fine for me, but on the issue linked above Paul (and outside of that thread, others) are not seeing that build address the problem.

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Oct 23, 2014

Contributor

My guess is this is a permissions issue caused by running sudo npm. We set the permissions on $HOME/.npm correctly when run via sudo, but we probably need to do the same when we're creating _templates. A simple workaround is just to chown $(whoami) $HOME/.npm/_git-remotes/_templates, but I'll file a bug to add a test / permissions fixup for the templates directory as well.

Contributor

othiym23 commented Oct 23, 2014

My guess is this is a permissions issue caused by running sudo npm. We set the permissions on $HOME/.npm correctly when run via sudo, but we probably need to do the same when we're creating _templates. A simple workaround is just to chown $(whoami) $HOME/.npm/_git-remotes/_templates, but I'll file a bug to add a test / permissions fixup for the templates directory as well.

@davidemoro davidemoro referenced this issue in MoonshotProject/moondash Nov 27, 2014

Merged

Dm devdependencies #22

@miketaylr miketaylr referenced this issue in webcompat/webcompat.com Oct 6, 2015

Closed

Error setting up webcompat locally #750

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.