[node] Added option --without-npm, installing npm by default. Closes #13024 #13053

Closed
wants to merge 1 commit into
from

Projects

None yet
@lavoiesl

I don’t really see the problem of using npm inside brew.
For those who still prefer using their own npm, --without-npm will do the trick

  • If you npm uninstall npm, it will break, and you’ll have to reinstall node to get it back, but it’s asking for it.
  • If you brew uninstall node, it will remove node and npm, but npm depends on node, so it’s logical.

@isaacs mentionned using a npmrc, but I am not sure how to use it. Guidance is appreciated.

Related issues: [#13024, #8784, #3762]

@benatkin

Looking good! @isaacs wrote about another change that should be made before this is ready, though. #13024 (comment)

@lavoiesl

Yep, currently working on it

@lavoiesl

Then I believe this PR is ready, unless you want me to squash the commits.
But I like separated commits !

@MikeMcQuaid
Homebrew member

@mxcl and @isaacs will need to sign off on this before it is merged. We removed npm deliberately a while ago (#3762) and I'm not quite sure (despite reading the referenced reports) what has changed to make this now work sensibly when it didn't before. Thanks for your work everyone though.

@isaacs

Almost, but not quite, I think:

; builtin config /usr/local/Cellar/node/0.8.0/lib/node_modules/npm/npmrc
prefix = "/usr/local/Cellar/node/0.8.0/share/npm"

That should be /usr/local/share/npm, shouldn't it?

@lavoiesl

Well, it depends.

Setting it to /usr/local/Cellar/node/0.8.0/share/npm will remove modules upon brew remove node which I think is a good thing since we are removing node, dependency of npm.

@adamv

Is it expected that updates of node, even minor releases (0.8.x) will require reinstalling all of the npm modules?

@isaacs

It will also remove all globally installed packages when you upgrade node, though, along with the bin path you've added to your PATH.

Let's say you do this, and you follow the caveat and add /usr/local/Cellar/node/0.8.0/share/npm/bin to your PATH. Then you do npm install -g jshint. Now you have a jshint command available on the command line. Lovely!

You also use npm link in a bunch of projects, so that you can make changes and see them reflected in your other projects immediately without a reinstall. Life is good.

Then a new version of node comes out, and you do brew update; brew install node. You go into your project, and try to run jshint, but it's gone. Your require('foo') lines all break, because the linked packages are gone. So, you come to the node and npm issues and mailing lists, and post bugs that "0.8.1 broke all my programs".

So, you reinstall everything. Surprise! The jshint command is still missing, because the folder you added to your PATH is now gone. (It's now /usr/local/Cellar/node/0.8.1/share/npm/bin.)

This is almost certainly not what you want.

Alternatively, let's say it's set to /usr/local/share/npm. You add /usr/local/share/npm/bin to your path. You install jshint with -g, and link a bunch of stuff. New version of node comes out, and you install it. You now have a new npm version (/usr/local/bin/npm is now a symlink to /usr/local/Cellar/node/0.8.1/bin/npm), and a new node version. Jshint is right where you left it, your PATH still points at folders that all exist, and your links are unbroken.

@isaacs

Also, removing node will still remove npm, since npm itself will be installed along with the node recipe, in Cellar/node/version/.... But it won't remove all the globally installed programs (which is probably not what you want anyway, but at least they're in a single place, so easily cleaned up.)

@lavoiesl

Oh, I didn’t thought that brew upgrade would remove the modules. Good thinking

@isaacs

LGTM.

Aside/feature request: It'd be nice if homebrew maintained a file with all the "Please set blah blah env to bloo bloo" stuff. Then I could just source it in my bashrc, and continue ignoring caveats :)

@lavoiesl

Yes totally. I have a project like this that I wish to start called bash-includes with basically runs scripts, set aliases and append paths. A basic version exists on my computer. I’ll try to remember to poke you if it comes to life :)

@benatkin

Trying it out, by reinstalling node a couple of times (first with npm already installed, then without npm installed).

It's hard being without node on my mac, but at lest I still have it on my VPS and my Linux box!

@benatkin

I'm getting this, when I try installing with npm already installed, using the installation script:

(mbp) /usr/local/Library/Formula $ brew link node
Linking /usr/local/Cellar/node/0.8.0... 
Error: Could not symlink file: /usr/local/Cellar/node/0.8.0/lib/node_modules/npm/test/update-test.sh
Target /usr/local/lib/node_modules/npm/test/update-test.sh already exists. You may need to delete it.
(mbp) /usr/local/Library/Formula $
@lavoiesl

Well, I could test to check if npm installed outside of /usr/local/Cellar. If so, I would add --without-npm.

Would that work for you ?

@benatkin

Maybe it should detect if npm is already installed and exit out, providing instructions on how to remove it.

Also the text should say Homebrew has installed npm, or perhaps This recipe has installed npm. It might be worth including the full command that needs to be added to .bash_profile as well:

export PATH=/usr/local/share/npm/bin:$PATH

Finally I didn't have to change anything to get npm to work, despite the error message appearing. The other files were linked and my npm was current enough that it still worked.

@benatkin

No, I think it should still install it with the new configuration. Most will want to get a normal install, I think.

@lavoiesl

Let me do some more tests

@lavoiesl

Help would be appreciated, I don’t know how to test if npm is already installed outside of Brew, but conflicting Brew’s path.

Something like checking if which npm is a symlink, following it and checking against where Brew wants to install it.

@benatkin

I think it should print a message if it's installed in /usr/local/lib/node_modules/npm which is where I think it gets installed by default with curl <npm install.sh url> | sh. If it's somewhere else I wouldn't bother.

@lavoiesl

But how do you differentiate from the one installed by brew on earlier versions ?

@benatkin

I think the one brew installs will be somewhere else. I'm installing node again to check.

@benatkin

To clarify: Earlier version means something like 0.8.0 when 0.8.1 is out, right?

@benatkin

Yeah, your recipe results in it going into /usr/local/Cellar/node/0.8.0/bin/npm.

Here's what I think it should do: look for npm in the path (with which), and if it's somewhere outside of Cellar, print out this message:


The homebrew node recipe now (beginning with 0.8.0) comes with npm. It appears you already have npm installed at <path-to-npm>. To use the npm that comes with this recipe, first uninstall npm with npm uninstall npm -g. Then run this command again.

If you would like to keep your installation of npm instead of using the one provided with homebrew, run this command again with the --without-npm option added.

@adamv

Explore what we do with Python and pip.

@benatkin

link https://github.com/mxcl/homebrew/tree/master/Library/Formula/python.rb

There is a difference, though - pip itself gets installed in the pip path. OTOH, node installs and links npm in the homebrew path.

@lavoiesl

There you go, the magic is this:

def external_npm_installed?
  path = Pathname.new("#{modules_folder}/npm")
  begin
    not path.realpath.to_s.include?(HOMEBREW_CELLAR)
  rescue Exception => e
    false
  end
end

/usr/local/lib/node_modules/npm may exist, but it must be a symlink that resides in HOMEBREW_CELLAR. This means that it was installed by brew. Otherwise, throw the error you talked about.

@benatkin

Now it seems overcomplicated. I think the check_npm_installed! stuff should just be rolled into check_npm_installed!. Also I changed my mind on just checking /usr/lib/node_modules. It should check it with which and see where the link points to, and if it points somewhere outside of the Cellar directory, show the message.

But I think maybe this feature just isn't worth the complexity. Maybe you should just roll back the previous two commits and go back to where @isaacs said "LGTM". If we add this it should be a separate pull request.

@lavoiesl

Not really, it looks cool to me.

external_npm_installed? is needed for the caveat at the end

And it is only essential to check for /usr/lib/local/node_modules/npm because this is where brew will install.
If you wish to install a conflicting version of npm to /opt, I won’t bug you for it.

@dwt

Whats the status of this pull request? I'm very interested in this and would like a conclusion.

@benatkin

It should be merged without 85eb44e.

@MikeMcQuaid
Homebrew member

If @mxcl and @isaacs sign off on it it can go in. Until then please be patient.

@isaacs

@benatkin

It should be merged without 85eb44e.

I pointed out the need for this above. Is there something I missed?

@lavoiesl

@benatkin : if we don’t do this, npm and modules get removed upon removal and upgrade of node

@benatkin
@lavoiesl

@benatkin, I don’t really understand what you are saying.

Are you asking that the commit be separated from this PR ? If so, why ? Do you not want the change of the installation directory or simply you want the change to be discussed separatly ?

For upgraders though, we could add more checks and display a better error message. I’m not sure what, just an idea

@isaacs

@benatkin I'm also a little confused... Are you +1 or -1 on 85eb44e? Or do you think it should be done, but a different way?

Fwiw, this pull req LGTM as-is right now.

@benatkin

Oops, it was 3bef323 that I was talking about, not 85eb44e.

I was thinking we could do without it, but now I think I'm wrong, people will expect help when upgrading.

But it is delaying this merge. It isn't ready on the Homebrew side of things according to @adamv. Fortunately @lavoiesl is still interested in this. Mind fixing that issue, @lavoiesl?

I'm sorry for the confusing messages.

@lavoiesl

No, I would definitely keep that part, otherwise, it can confuse the existing installation of npm. If you have a npm installed, either remove it or specify you want node --without-npm.

I’ll check what I can do for the Requierement

@isaacs

@benatkin Oh, ok, yeah, 3bef323 seems like a lot of homebrew stuff I have no opinion about.

@lavoiesl

It is basically crashing with some style

@lavoiesl

There it is. It is the first time I am doing a Requirement, so if someone can verify my work, it would be appreciated.

Cheers !

@mxcl
Homebrew member

The depends_on should have unless ARGV.include? "--without-npm".

If @isaacs is fine with this then this suits me.

It better work well however, or I'll make a big fuss.

@adamv

Needs to be squashed to a single commit and rebased on master for review after that, too

@lavoiesl lavoiesl [node] Added option --without-npm, installing npm by default. Closes #…
…13024.

[node] Adds a (lib + "node_modules/npm/npmrc") with prefix "#{share}/npm" so npm doesn’t conflict brew. See #13024

[node] minor rewording, as suggested by @isaacs

[node] removed global = true, as suggested by @isaacs

[node] Changed npm prefix from /usr/local/Cellar/node/*/share/npm to /usr/local/share/npm

[node] check for external npm installation before allowing node to install with npm

[node] Changed exception when external npm is installed to a Requirement

[node] moved unless ARGV.include? '--without-npm' to the depends_on instead of the Requirement
e98deb6
@lavoiesl

@mxcl It was working because the unless was in the requirement, but I understand it is cleaner this way.

rebase/squash done.

Thanks

@mxcl
Homebrew member

Oh. I'm a noob with the requirements stuff. It should have remained I'm sure. We'll fix in the cherry-pick.

@mxcl
Homebrew member

One last thing to do is remove npm from blacklist.rb. We can do in the cherry-pick.

@lavoiesl

It should have remained I'm sure.

I don’t understand what you mean

@mxcl
Homebrew member

Me telling you to add the unless was incorrect.

@lavoiesl

No, I like it better like this. There shouldn’t be a requirement if an option specifies to ignore it

@nandub

FYI, this method of installing NPM curl https://npmjs.org/install.sh | sh does not work currently.

@lavoiesl
@JeanMertz

What @nandub is talking about, is that the url https://npmjs.org/install.sh now returns a 404. I just found this issue while wading my way through 3 or 4 other GH issues. My dotfiles bootstrap.sh does:

brew install node
curl https://npmjs.org/install.sh | sh

but this now fails (since today, it still worked yesterday) because of the 404. So either it's a temporary glitch on the npm website or this pull requests is now needed if we want to still be able to install node through Homebrew and use npm with it.

@isaacs
@mybuddymichael

Is this pull request good to go?

@adamv

GitHub says it needs to be rebased.

@mxcl mxcl added a commit that closed this pull request Aug 27, 2012
@lavoiesl lavoiesl Install npm by default
Closes #13053. Refs #13024.

Adds option "--without-npm".

Signed-off-by: Max Howell <mxcl@me.com>

Fixed merge. Amended some user-facing texts.
1e70950
@mxcl mxcl closed this in 1e70950 Aug 27, 2012
@malandrew

Just an suggestion next time a change like this is made. Instead of telling the user after the fact that a change like this was made, it may be better to detect if the user already has npm install and warn them of the change so they can choose if they want npm bundled or not.

For people installing node (and by extension npm) for the first time, the execution of brew install node with a post install message telling them to modify $NODE_PATH is fine, but for those doing brew upgrade node there should be a warning so they can choose ahead of time if they want to use --without-npm

I casually dismissed the post-upgrade message and spent a while trying to track down in node and npm where the /usr/local/share/ global directory was introduced.

While I'm making that comment, it'd also be nice if brew supported terminal colors so that important post install messages and warnings could stick out in some color other than the default.

@Nexuapex Nexuapex pushed a commit that referenced this pull request Aug 29, 2012
@lavoiesl lavoiesl Install npm by default
Closes #13053. Refs #13024.

Adds option "--without-npm".

Signed-off-by: Max Howell <mxcl@me.com>

Fixed merge. Amended some user-facing texts.
0b08683
@jtokoph

Running a brew upgrade node today blew away my npm install without any warning and then failed to link new binaries. Was this intended?

Link to my shell session: https://gist.github.com/3561067

@lavoiesl

No, it was not supposed to do that, normally it should refuse to install if you have npm installed and ask you to install with --without-npm.

I see it failed to link, did you install npm with sudo ?

@jtokoph

I don't think so. Running brew link -f node seemed to get the files linked without needing to give a password.

@kgreunke

I had the same problem as @jtokoph with the upgrade failing to link after installing.

edit: After doing a brew link -f node all my installed modules went missing. I think this is due to a path change. I checked out the previous version of homebrew I had installed, reinstalled node and npm and all my previously installed modules were there again.

@ckdaas ckdaas added a commit that referenced this pull request Sep 10, 2012
@lavoiesl lavoiesl Install npm by default
Closes #13053. Refs #13024.

Adds option "--without-npm".

Signed-off-by: Max Howell <mxcl@me.com>

Fixed merge. Amended some user-facing texts.
fce7593
@Sharpie Sharpie pushed a commit to Sharpie/homebrew that referenced this pull request Sep 12, 2012
@lavoiesl lavoiesl Install npm by default
Closes #13053. Refs #13024.

Adds option "--without-npm".

Signed-off-by: Max Howell <mxcl@me.com>

Fixed merge. Amended some user-facing texts.
0e0aec1
@shazron shazron added a commit that referenced this pull request Sep 13, 2012
@lavoiesl lavoiesl Install npm by default
Closes #13053. Refs #13024.

Adds option "--without-npm".

Signed-off-by: Max Howell <mxcl@me.com>

Fixed merge. Amended some user-facing texts.
a532b2c
@mxcl
Homebrew member

@kgreunke brew un/link only manage symlinks into the Cellar.

To everyone: is there anything we can do, clearly this was a poorly considered patch that has caused much user-discomfort and I am upset about it. I am not familiar with npm or node and thus don't know what to do.

@kgreunke

I think the ultimate problem with the patch was the change to the npm modules directory. Had that been left as the default non-brew location, the script could have removed the non-brew installed npm and installed it's own npm and everything would have worked as before.

I realize that might not have been the "brew way", but the modules directory can't be put in version specific brew directory anyway, so why move it at all.

@jtokoph

@mxcl I'm not sure it matters much at this point. Anyone who runs into an issue upgrading will ultimately arrive at this discussion with suggested fixes. Any new installations won't have issues.

@MikeMcQuaid
Homebrew member

@jtokoph I can't agree. People upgrade long after so we really should fix this.

@cogweh

@jtokoph @mxcl this still has problems with new installs. Previously I didn't have to have npm directly in my path in order to access to certain CLI modules i.e. coffee-script. It's not a big pain to add an additional path, but it seems messy to me. Also, it was annoying to find out that the modules directory had moved.

@WickyNilliams

@jtokoph much like @mikemcquaid , I don't agree with your sentiment. I usually forget to do regular updates, and it's only through pure chance I arrived at this dicsussion. So with that, I ran into this issue last night. I have approximately half as much hair left on my head after much pulling!

I could not for the life of me work out where my node modules had gone, or why new modules were being installed at a seemingly new location. I was sure it was my fault since I don't expect breaking changes in minor version increments. Also being a recent migrant to osx compounded things, i was left completely baffled and quite frustrated.

Before i came across this thread i managed to work around the issue by setting the prefix in my .npmrc in home directory to /usr/local/. This seems to have worked, but will it have other mysterious side-effects? What is the recommended fix for this rather than sticking with my hacky workaround? uninstall node, reinstall with --without-npm flag, then install npm manually?

@mxcl
Homebrew member

If people could be more verbose with their replies, it would help. For example I have no idea what the default npm module location is, so I can hardly fix this to go back to that.

Also I think npm installed binaries should be in the path. It's just ridiculous that they aren't. People keep writing formula that do these sorts of things and it is RIDICULOUS.

@WickyNilliams

Apologies.

It is my understanding that previously the prefix for npm was /usr/local whereas it appears to have been changed to /usr/local/share/npm/. If I'm thinking right, npm puts binaries in {prefix}/bin and source in {prefix}/lib/node_mobules. /usr/local/bin was always in my path (doesn't brew doctor even recommend this?), but obviously the change meant that the binaries were being dumped in a location that was no longer in my path.

@isaacs

Also I think npm installed binaries should be in the path. It's just ridiculous that they aren't. People keep writing formula that do these sorts of things and it is RIDICULOUS.

@mxcl That's very good to hear. I can't agree with you more.

IIRC, there was some concern about npm putting things directly in the "normal" prefix directories. For example, /usr/local/lib/node_modules/foo and /usr/local/bin/foo. (I str that I wrote an npm recipe that behaved in this way, and the main bit of feedback was, "That's not allowed, it needs to be separate".)

Earlier versions of this recipe used the prefix /usr/local/Cellar/npm/{npm version}/, so binaries ended up in something like /usr/local/Cellar/npm/1.1.61/bin. Not only is that not in the PATH (and probably never going to be), but it's also destroyed on every upgrade! So, if you did brew install npm; npm install less --global; brew upgrade npm, then you'd lose the binary lessc program that you'd installed with npm!

A compromise was made to have brew-installed npm use /usr/local/share/npm as its prefix. Thus, globally-installed binaries would go in /usr/local/share/npm/bin. That is not in the PATH. I think it's sub-optimal, but I was told that a caveat telling users to put this folder in their PATH would suffice. However, it appears to be causing some issues.

If someone can articulate the exact behavior that is intended, I'll tell you how to make the files and commands line up to make that happen. I'm certainly no homebrew expert, but if Node and npm can't deliver what you need, I'll change them so that they do.

@WickyNilliams

Since the problem seems squarely focussed around updating node via brew, and people not realising the consequences of this, might it be an option to change the flag to --with-npm so that disruption is not caused by default? That way appropriate messaging can be delivered when node is updated to say that if you want npm, use the --with-npm flag and consequences and remedies can be explained?

Please tell me to shut up if I'm talking nonsense :-) It's just since I'm, at this point, pretty much a clueless end-user I thought my perspective might be of use

@isaacs

In my opinion, it seems that the ideal solution would be for things with homebrew to work the same as they would without homebrew, but with the ease of using the commands that homebrew users know.

  1. Whether it's installed independently, or with node, npm should end up in the same state, with the same configuration.
  2. When you install node, if you don't explicitly ask for it, it should also upgrade npm. People see the two as things that "go together" these days, so in most cases, this is the expected behavior.

I'd be happy to take a closer look at this. In the case of upgrading node, it seems liek the issue is that it doesn't preserve npm's homebrew-style configurations. It's an easy change to make it do that, but we should evaluate if it's the most sensible path. Sniffing for whether the user has installed it already or not seems a bit weird to me.

@adamv

Reopening since discussion is happening.

@adamv adamv reopened this Sep 25, 2012
@jtokoph

I guess I was wrong, it should still be fixed. But the biggest issue now might be fixing it in a way that doesn't break things again for people that already upgraded. Is that even possible without a giant mess in the recipe formula?

@adamv adamv closed this Sep 25, 2012
@snakeyroc3 snakeyroc3 pushed a commit to snakeyroc3/homebrew that referenced this pull request Dec 17, 2012
@lavoiesl lavoiesl Install npm by default
Closes #13053. Refs #13024.

Adds option "--without-npm".

Signed-off-by: Max Howell <mxcl@me.com>

Fixed merge. Amended some user-facing texts.
3829055
@xu-cheng xu-cheng locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.