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

Fix for installing llnode as a global #94

Closed
wants to merge 2 commits into from
Closed

Fix for installing llnode as a global #94

wants to merge 2 commits into from

Conversation

No9
Copy link
Member

@No9 No9 commented Jun 7, 2017

When npm explore is ran with a -g option the default folder is set to npm.globalDir
which is the root $GLOBAL/lib/node_modules folder containing npm.
But when it's ran in local mode it uses path.resolve(npm.prefix, 'node_modules')
See https://github.com/npm/npm/blob/latest/lib/npm.js#L395
This is causing the resolution of node-gyp to fail in global mode.

This patch tests the npm environment variable npm_config_global to see if it's a global install and appends the required sub directories to get it to work.

Tested on Ubuntu 16.04 with

$ git clone https://github.com/no9/llnode
$ cd llnode
$ npm install -g 

Fixes: #77

@hhellyer
Copy link
Contributor

hhellyer commented Jun 8, 2017

The fix looks good to me, I've tested locally and both local and global installs now work.

@rnchamberlain
Copy link
Contributor

LGTM - tested on Ubuntu 16.04. It's very useful to have install -g working for this - thanks!

indutny

This comment was marked as off-topic.

@No9
Copy link
Member Author

No9 commented Jun 8, 2017

Thanks for the reviews - nits are resolved and PR updated.

indutny pushed a commit that referenced this pull request Jun 15, 2017
PR-URL: #94
Reviewed-By: Howard Hellyer <hhellyer@uk.ibm.com>
Reviewed-By: Richard Chamberlain <richard_chamberlain@uk.ibm.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@indutny
Copy link
Member

indutny commented Jun 15, 2017

Landed in fb24599, thank you! Sorry for delay.

@indutny indutny closed this Jun 15, 2017
@hhellyer
Copy link
Contributor

@indutny - Since this is a fix to npm installation I'll update the version to 1.5.1 and publish a new version of the npm on Monday, unless you object!
(The brew installation probably doesn't need an update for this.)

@rnchamberlain
Copy link
Contributor

@indutny @hhellyer could we land the README fixes as well, then publish? #85 and #86

@hhellyer
Copy link
Contributor

@rnchamberlain - Yep, I'll do that today.

This was referenced Jun 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

npm install -g llnode fails
4 participants