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: doc how to update node-gyp independently from npm #2448

Merged

Conversation

owl-from-hogvarts
Copy link
Contributor

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

#2272

@cclauss
Copy link
Contributor

cclauss commented Jun 28, 2021

My sense is that we should have a separate docs for Windows and Linux/macOS. Most of the open issues and complexity is on the Windows side. The Linux/macOS doc would be more streamlined.

@owl-from-hogvarts
Copy link
Contributor Author

IMO what you are suggesting should be done in separate pull request. This one is only for incldujg important notes to docs.

After including that we can start work on separating docs by platforms.


## Windows
@joaocgreis' Windows instructions from [#1753 (comment)](https://github.saobby.my.eu.orgnodejs/node-gyp/issues/1753#issuecomment-501827267)
Copy link
Member

Choose a reason for hiding this comment

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

not sure we need this line if you've captured the essence of it below

@rvagg
Copy link
Member

rvagg commented Jun 30, 2021

Seems fine to me but can you please go over the text and look for missing spaces, there's a bunch of run-ins in many of the lines. I did start a suggestion review but it impacts most of the first half of this at least. Maybe a copy/paste problem?

Also I'm fine with just getting this into a good state and then we could fork it into separate docs if it gets large enough.

@@ -1,63 +1,40 @@
`npm` bundles its own, internal, copy of `node-gyp`. This internal copy is independent of any globally installed copy of node-gyp that
you may have installed via `npm install -g node-gyp`.
npm ships with its own version of node-gyp. Older versions of npm have olderversions of node-gyp. npm has caught up recently to node-gyp's release cycle butit's expected we'll get out of sync and there will be cases where you need an oldnpm but a newer node-gyp, or npm doesn't ship yet with a node-gyp that we'vereleased and it has fixes you need.
Copy link
Contributor

Choose a reason for hiding this comment

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

My sense is that this sentence is more meandering and less clear than the original.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree

@owl-from-hogvarts
Copy link
Contributor Author

@cclauss can this be merged?

@@ -1,63 +1,41 @@
`npm` bundles its own, internal, copy of `node-gyp`. This internal copy is independent of any globally installed copy of node-gyp that
you may have installed via `npm install -g node-gyp`.

This means that while `node-gyp` doesn't get installed into your `$PATH` by default, npm still keeps its own copy to invoke when you
attempt to `npm install` a native add-on.
Generally, npm's library files are installed inside your global "node_modules",where npm is installed (run `npm prefix` and add `lib/node_modules`, or just `node_modules` for Windows). There are some exceptions to this. Inside this global `node_modules/`there will be an `npm/` directory and inside this you'll find a `node_modules/node-gyp/` directory. So it may look something like `/usr/local/lib/node_modules/npm/node_modules/node-gyp/`. This is the version of node-gyp that ships with npm.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Generally, npm's library files are installed inside your global "node_modules",where npm is installed (run `npm prefix` and add `lib/node_modules`, or just `node_modules` for Windows). There are some exceptions to this. Inside this global `node_modules/`there will be an `npm/` directory and inside this you'll find a `node_modules/node-gyp/` directory. So it may look something like `/usr/local/lib/node_modules/npm/node_modules/node-gyp/`. This is the version of node-gyp that ships with npm.
Generally, npm's library files are installed inside your global "node_modules", where npm is installed (run `npm prefix` and add `lib/node_modules`, or just `node_modules` for Windows). There are some exceptions to this. Inside this global `node_modules/` there will be an `npm/` directory and inside this you'll find a `node_modules/node-gyp/` directory. So it may look something like `/usr/local/lib/node_modules/npm/node_modules/node-gyp/`. This is the version of node-gyp that ships with npm.

Copy link
Contributor Author

@owl-from-hogvarts owl-from-hogvarts Jul 6, 2021

Choose a reason for hiding this comment

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

On windows global node_modules and node's node_modules are two diffrent dirs, aren't they? Thats why

Generally, npm's library files are installed inside your global "node_modules", where npm is installed

can really mislead.

I propose this:

So npm's internal copy of node-gyp isn't stored inside global node_modules and thus isn't available for use as a standalone package. Npm uses it's internal copy of node-gyp to automatically build native addon when you are trying to it install via npm install

docs/Updating-npm-bundled-node-gyp.md Outdated Show resolved Hide resolved
docs/Updating-npm-bundled-node-gyp.md Outdated Show resolved Hide resolved
docs/Updating-npm-bundled-node-gyp.md Outdated Show resolved Hide resolved
@cclauss
Copy link
Contributor

cclauss commented Jul 6, 2021

Conflicting files docs/Updating-npm-bundled-node-gyp.md

Are we convinced that the majority of users are going to remember to undo/redo when upgrading?

@owl-from-hogvarts owl-from-hogvarts force-pushed the docs/update-indepenetly-from-npm branch from d87ebb5 to 3a4c369 Compare July 6, 2021 10:29
@owl-from-hogvarts
Copy link
Contributor Author

When updating npm or node-gyp?

As i understand npm config writes to .npmrc which i asume should persist when updating npm. And also does path to global node-gyp really breaks when updating?

@owl-from-hogvarts
Copy link
Contributor Author

Does my own addition to this file should be done as separate PR?

@owl-from-hogvarts
Copy link
Contributor Author

@rvagg @cclauss Can this finally be merged, pls?

@rvagg
Copy link
Member

rvagg commented Jul 23, 2021

yes, it can! sorry, I didn't see that you'd updated based on the review, thanks @owl-from-hogvarts!

@rvagg rvagg merged commit f0882b1 into nodejs:master Jul 23, 2021
@DeeDeeG
Copy link
Contributor

DeeDeeG commented Nov 14, 2021

Hi folks.

Short summary: This doesn't work in npm 7 or newer.

This advice is not applicable to npm 7 or 8 at the moment. npm authors inadvertently broke the node_gyp npm config var's functionality, starting in npm 7, and it has not been fixed since. Still broken in the latest npm 8.1.3 as I write this comment. npm maintainers have acknowledged the issue.

See these open issues I filed at the npm repo: https://github.com/npm/cli/issues?q=is%3Aissue+is%3Aopen+node-gyp+author%3ADeeDeeG

The advice in this file, as of this PR, boils down to setting that config var. So it won't work on npm 7 or npm 8.

The catch is that npm won't use this version unless you tell it to, it'll keep on using the one you have installed. You need to instruct it to by setting the node_gyp config variable (which goes into your ~/.npmrc). You do this by running the npm config set command as below. Then npm will use the command in the path you supply whenever it needs to build a native addon.

That advice is unfortunately not true on npm 7 and 8 at the moment.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Nov 14, 2021

Short summary: The content of this docs page from before this PR had steps that work in npm 7. Please host/restore the old content of this docs page somewhere. Furthermore, this PR caused the title/filename to not match the content. Please make it so the titles/filenames match the content.


Also: The title and filename of this do not match the content anymore, as of this PR. This advice is more about "Having npm use an independent copy of node-gyp instead of its bundled copy".

The title ("Updating the npm-bundled version of node-gyp") matches the previous content.

I would request that the previous content of this file be hosted somewhere in the docs... Preferably under this filename, to make the edit history follow-able. (In other words, please consider restoring the previous content of this file, and moving the new content to another file.)

I would humbly suggest to move the updated content to a new file, under an appropriate filename and title, with the understanding that it applies ONLY to npm 6 and lower, and saying so somewhere in this file, so users don't get surprised when it doesn't work in npm 7 or higher.

The method described as of this PR is a more durable, maybe less hacky fix for npm 6 users. So I do like this method. It's just a minor disappointment it doesn't work on newer npm right now.

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.

4 participants