Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

lavoiesl
Copy link
Contributor

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
Copy link
Contributor

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

@lavoiesl
Copy link
Contributor Author

Yep, currently working on it

@lavoiesl
Copy link
Contributor Author

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

@MikeMcQuaid
Copy link
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
Copy link
Contributor

isaacs commented Jun 27, 2012

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
Copy link
Contributor Author

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
Copy link
Contributor

adamv commented Jun 27, 2012

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

@isaacs
Copy link
Contributor

isaacs commented Jun 27, 2012

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
Copy link
Contributor

isaacs commented Jun 27, 2012

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
Copy link
Contributor Author

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

@isaacs
Copy link
Contributor

isaacs commented Jun 27, 2012

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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

@lavoiesl
Copy link
Contributor Author

Let me do some more tests

@lavoiesl
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@benatkin
Copy link
Contributor

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

@benatkin
Copy link
Contributor

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

@benatkin
Copy link
Contributor

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
Copy link
Contributor

adamv commented Jun 27, 2012

Explore what we do with Python and pip.

@benatkin
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

@JeanMertz
Copy link
Contributor

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
Copy link
Contributor

isaacs commented Aug 11, 2012

On Sat, Aug 11, 2012 at 4:01 AM, Jean Mertz notifications@github.comwrote:

What he is talking about, is that the url https://npmjs.org/install.shnow 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.

It's a bug. The bug is now fixed.

@mybuddymichael
Copy link
Contributor

Is this pull request good to go?

@adamv
Copy link
Contributor

adamv commented Aug 25, 2012

GitHub says it needs to be rebased.

@mybuddymichael
Copy link
Contributor

@lavoiesl Rebase?

@mxcl mxcl closed this in 1e70950 Aug 27, 2012
@andrewdeandrade
Copy link
Contributor

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.

@jtokoph
Copy link

jtokoph commented Aug 31, 2012

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
Copy link
Contributor Author

lavoiesl commented Sep 1, 2012

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
Copy link

jtokoph commented Sep 1, 2012

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

@kgreunke
Copy link

kgreunke commented Sep 4, 2012

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.

Sharpie pushed a commit to Sharpie/homebrew that referenced this pull request Sep 12, 2012
Closes Homebrew#13053. Refs Homebrew#13024.

Adds option "--without-npm".

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

Fixed merge. Amended some user-facing texts.
@mxcl
Copy link
Contributor

mxcl commented Sep 13, 2012

@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
Copy link

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
Copy link

jtokoph commented Sep 14, 2012

@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
Copy link
Member

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

@cogweh
Copy link
Contributor

cogweh commented Sep 14, 2012

@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
Copy link

@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
Copy link
Contributor

mxcl commented Sep 25, 2012

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
Copy link

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
Copy link
Contributor

isaacs commented Sep 25, 2012

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
Copy link

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
Copy link
Contributor

isaacs commented Sep 25, 2012

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
Copy link
Contributor

adamv commented Sep 25, 2012

Reopening since discussion is happening.

@adamv adamv reopened this Sep 25, 2012
@jtokoph
Copy link

jtokoph commented Sep 25, 2012

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 pushed a commit to snakeyroc3/homebrew that referenced this pull request Dec 17, 2012
Closes Homebrew#13053. Refs Homebrew#13024.

Adds option "--without-npm".

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

Fixed merge. Amended some user-facing texts.
@Homebrew Homebrew 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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet