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

Rimraf npm #6048

Closed
wants to merge 2 commits into from
Closed

Rimraf npm #6048

wants to merge 2 commits into from

Conversation

MylesBorins
Copy link
Contributor

build: remove npm on make install

Different versions of npm potentially have different trees. We should
be cleaning the version of npm on every install.

This implements the removal naively by calling
rm -rf $PREFIX/lib/node_modules/npm in the make install command.

This does not implement similar functions for the windows installer,
nor does it implement the removal in the double click installers.

This can be implemented as part of this PR or as part of another.

@MylesBorins MylesBorins added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. npm Issues and PRs related to the npm client dependency or the npm registry. labels Apr 5, 2016
@jbergstroem
Copy link
Member

Doing rm -rf $var/foo will get unixbeards turning in their grave. Suggesting you wrap this with a few checks if you insist on doing so.

@MylesBorins
Copy link
Contributor Author

@jbergstroem I may or may not be implementing things in a naive way to get the ball rolling 😄

@MylesBorins
Copy link
Contributor Author

Also worth mentioning that if we can hit all the edge cases (all installers) we should be able to close #3606

@mscdex
Copy link
Contributor

mscdex commented Apr 5, 2016

I turned over in my grave and I'm not dead yet.

@Fishrock123
Copy link
Contributor

@thealphanerd This isn't the correct fix though. I've never had problems via the Makefile. The installers need to be doing this.

@MylesBorins
Copy link
Contributor Author

@Fishrock123 I've had this problem turn up with the makefile as well when moving between LTS releases.

I'm part way through a fix for the osx installer

@jbergstroem
Copy link
Member

@thealphanerd: perhaps look at implementing this in tools/install.py instead? Would give a few way of making sure that path is correct (and potentially verify the structure).

The question whether deleting the folder or not still remains. Are the edge cases causing installer issues known?

@wacky6
Copy link

wacky6 commented Apr 27, 2016

OSX 10.11, upgrading from v5.10 to v6.0 using official OSX Installer.

This problem still exists.

rm -rf /usr/local/lib/node_modules/npm then reinstall v6.0 solves the problem.

@@ -78,6 +78,7 @@ config.gypi: configure
fi

install: all
-rm -rf '$PREFIX'/lib/node_modules/npm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't do what you think it does, the path gets evaluated to 'REFIX'/lib/node_modules/npm. I don't want to sound harsh but did you actually test it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbqh I was whipping this up while doing a handful of other things. I assumed the rimraf implementation would not be accepted and wanted some suggestions on next steps. I've updated with ${PREFIX) so that it actually works, and will explore doing this in the python script

@bnoordhuis
Copy link
Member

I'd be more inclined to LGTM this if it was a pre- or post-install pass in tools/test.py that checks whether the install directory contains unexpected files. Or perhaps just a warning if the directory already exists.

@MylesBorins MylesBorins force-pushed the rimraf-npm branch 2 times, most recently from 676c842 to 11edcb0 Compare April 27, 2016 15:22
@MylesBorins MylesBorins added the wip Issues and PRs that are still a work in progress. label May 10, 2016
Myles Borins added 2 commits May 12, 2016 11:16
Different versions of npm potentially have different trees. We should
be cleaning the version of npm on every install.

This implements the removal naively by calling
`rm -rf $PREFIX/lib/node_modules/npm` in the `make install` command.

This does not implement similar functions for the windows installer,
nor does it implement the removal in the double click installers.

This can be implemented as part of this PR or as part of another.
@MylesBorins
Copy link
Contributor Author

I'm going to close this for now, I'll revisit later

@MylesBorins MylesBorins closed this Jun 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. npm Issues and PRs related to the npm client dependency or the npm registry. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants