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

Include node_root_dir in include path #492

Closed
wants to merge 1 commit into from

Conversation

rmg
Copy link

@rmg rmg commented Aug 16, 2014

Currently the --nodedir option assumes the path given contains src/ and deps/, but bundled with each release of node is the same files in a somewhat flattened directory structure at $PREFIX/include/node/.

This is a quick hack to allow --nodedir=$PREFIX/include/node work, which lets node-gyp build against the headers bundled with the version of node being run, even if it isn't an official release.

It would work best if either node releases or the bundled version of npm pre-configured nodedir to point at the bundled headers.

Allow --nodedir=/usr/local/include/node which should allow node-gyp to
build addons using the headers bundled with the node release rather
than requiring a release tarball be downloaded.
@sam-github
Copy link
Contributor

/to @tjfontaine @bnoordhuis This seems like an obvious improvement, why isn't like this already?

@bnoordhuis
Copy link
Member

I can think of two reasons:

  1. This creates a discrepancy between release tarballs and source code checkouts.
  2. The auto-generated include/ directory is not the "blessed" way of building add-ons. It was added to appease people with homegrown build systems.

@tjfontaine
Copy link

So my ideal patch checks to see if $PREFIX/include/node exists on the
target install, and use that, otherwise download the tarball.

Concerns I have are around the people doing cross compile or cross install
targeting a separate node from the node running node-gyp

I will note that the goal of having the include directory created and
populated is to behave like the vast majority of products on the market
that support building addons.

Depending on the internet to build addons is simply unacceptable for most
installs, also installs may have custom configurations that are
incompatible with what we distribute from the website

@bnoordhuis
Copy link
Member

So my ideal patch checks to see if $PREFIX/include/node exists on the target install, and use that, otherwise download the tarball.

That, however, is a recipe for disaster when $PREFIX/include/node is from a different node version.

(As you note in the paragraph below, I think, but it's an important enough drawback that it bears repeating.)

@rmg
Copy link
Author

rmg commented Aug 22, 2014

@bnoordhuis is your objection to allowing node-gyp build --nodedir=/usr/local/include/node actually do something? Or is your objection to making $PREFIX/include/node the default? Note that this PR only does the former.

@tjfontaine similar question. Your concerns seem to be more related to defaults than simply allowing $PREFIX/include/node to work, correct?

Even if the defaults were changed, people building from source and people doing cross compiling already have to configure their systems accordingly, so their experience wouldn't change.

There would be a discrepancy between source and binary distributions, as @bnoordhuis points out. I'm not sure that the status quo of using node-gyp to make end-user environments match those of core developers is worth protecting, though. That seems like the kind of thing that is more appropriate early in a project when most users are contributors.

@rmg
Copy link
Author

rmg commented Aug 22, 2014

The "problem" being addressed in this PR is that there is currently no way to tell node-gyp to build against the distributed headers even if one wanted to because the directory layout is different than the one hard-coded into node-gyp to match the source distribution. The result is that everyone using binary addons has the source code for a version of node that matches what node -v reports while the vast majority of them have not actually built from source.

I haven't looked closely, but I think this might also open the door for allowing node-gyp to run without $HOME being set (eliminating a few configuration errors) as well as eliminating a whole class of permissions (sudo anyone?).

@bnoordhuis
Copy link
Member

For the record, I don't object to this change. I do think TJ's proposal is - at least in its naive implementation - a change for the worse but that's really a separate discussion (both the proposal and my objections.) So, LGTM FWIW.

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.

5 participants