nodejs / node-v0.x-archive

Moved to https://github.com/nodejs/node
34.44k stars 7.31k forks source link

Add `install-dev` make target to install header files #5112

Closed isaacs closed 10 years ago

isaacs commented 11 years ago

Yes, yes, npm and node-gyp solve this.

But people want to have a way to install the node.h, v8.h, etc, like we used to.

Let's just add a make target to do this, and have it be an option that they can do if they really want to manually.

dpemmons commented 11 years ago

+1

Thank you - we use chris lea's Ubuntu package + our own build system for node modules and got bit when the package no longer included the header files. Didn't know node-gyp is supposed to put them somewhere since we don't use it.

bnoordhuis commented 11 years ago

-1 from me for reasons I've outlined in #5047 and will repeat here:

  1. node.h is a rather generic file name that conflicts with files in e.g. python-dev and ruby-dev. Screw up your include path and #include <node.h> will include the wrong file.
  2. Chromium on the BSDs builds against a shared libv8 that installs its headers in /usr/include - which means #include "v8.h" has a pretty good chance of picking up the wrong header file.

People will just have to get used to node-gyp.

dpemmons commented 11 years ago
  1. That's why they lived in $C_INCLUDE_PATH/nodejs/. Those of us using the package do #include <nodejs/node.h> as is convention.
  2. If that's an issue we could put node's v8.h in $C_INCLUDE_PATH/nodejs/v8/ and use it with#include <nodejs/v8/v8.h>

Saying 'just get used to our build system" is easy to say when you're not the one with an exiting build system and 4.5m lines of code.

bnoordhuis commented 10 years ago

No one has done any work on this and the reasons I outlined above are as valid as ever. I'm closing this as WONTFIX.

tjfontaine commented 10 years ago

I still think this is important, no work having been done doesn't mean I don't want to fix it, assigning to me

ghost commented 10 years ago

There is no value in not delivering header files that are required to use documented public interfaces (e.g., http://nodejs.org/api/addons.html). The node-gyp "workaround" of downloading its own copy of node and using its headers is unsatisfactory in three critical ways:

  1. It's incorrect. The interfaces specified in the headers used to build an addon must match those provided by the copy of node (and any shared libraries required to use node, if it was built that way) that the addon will be used with, at least to the extent that any public interfaces or definitions may have changed. When this goes wrong, it leads to failures that are extremely difficult and frustrating to debug. To the extent that this works at all today, it does so by accident.
  2. Not everyone uses (or wants to use, or can use) node-gyp. Leaving this as it is forces everyone to either adopt an approach similar to that of node-gyp (which is incorrect; see above) or to deliver their own frozen-in-time copy of node, v8, etc. headers in their addon source (also incorrect for the same reason).
  3. It worked in 0.8, so this is a regression. Even if the change had been documented, which it was not, there's no reason to break this in 0.10 when it worked before.
davepacheco commented 10 years ago

wesolows: +1. We have a bunch of software that used the headers delivered with v0.8. I'm just now getting to migrate this stuff to 0.10, and I can't do it without significant changes to the build system. With the headers in /usr/include/node as they were in 0.8, there's no namespace conflict there, since it's in a node-specific subdirectory that was carved out when 0.8 became widely used. A component wanting to build against Ruby or Python can skip "-I" and #include <node/node.h>, #include <python/.../node.h>, and so on.

bnoordhuis commented 10 years ago

It's incorrect. The interfaces specified in the headers used to build an addon must match those provided by the copy of node (and any shared libraries required to use node, if it was built that way) that the addon will be used with, at least to the extent that any public interfaces or definitions may have changed. When this goes wrong, it leads to failures that are extremely difficult and frustrating to debug. To the extent that this works at all today, it does so by accident.

That argument doesn't make sense. One of the things node-gyp does is make sure that add-ons are built against the header files of the node version that will load the add-on.

Compare that with site-wide installed headers; if those are outdated, you get build errors if you're lucky. If you're not so lucky: spurious run-time failures, aborts, segmentation faults, etc., etc.

TooTallNate commented 10 years ago

So am I correct in understanding that part of the problem here is that #include <node/node.h> doesn't work with node-gyp, whereas it did with node-waf (or more specifically, when the headers were being installed to /usr/local/include/node)? We can likely fix that by adding an include line in the addon.gyp file in node-gyp.

Additionally, node-gyp has the ability for you to specify the location of the node header files, rather than having it manually download the tarball and extract the headers. Anybody having issues with the fact that node-gyp connects to the internet should probably be using the --nodedir flag.

davepacheco commented 10 years ago

[Edit for formatting; copy/pasting from a gist didn't go well.]

The proposal is for Node.js to deliver header files into $PREFIX/include/node for itself and for the libraries against which it is statically linked. This makes it look like most other packages that allow users to build binaries against them, which helps it integrate into existing build systems. This doesn't break how node-gyp works, but allows it to work smoother going forward. (See below for details.)

The add-on documentation indicates that users are supposed to include header files like so:

#include <node.h>
#include <v8.h>

It's helpful to enumerate the various build-time and runtime possibilities, using V8 as the example library:

Case 1: Node built statically linked with V8, PREFIX=/usr Delivered files: /usr/include/node/node.h, /usr/include/node/v8.h, and related files. Required steps for the user: User must specify "-I /usr/include/node". Result: Add-ons build normally against the correct headers.

Case 2: Node built statically linked with V8, user-defined PREFIX Delivered files: $PREFIX/include/node/node.h, $PREFIX/include/node/v8.h Required steps for the user: User must specify "-I $PREFIX/include/node", but this is SOP for building against a component installed into a custom location. Result: Add-ons build normally against the correct headers.

Case 3: Node built dynamically linked with system V8, PREFIX=/usr (This is the second objection from the earlier comment.) Delivered files: $PREFIX/include/node/node.h (no v8.h) Required steps for the user: User must specify "-I /usr/include/node". Result: Add-ons build normally against the correct headers.

Case 4: Node built dynamically linked with V8 in custom V8PREFIX Delivered files: $PREFIX/include/node/node.h (no v8.h) Required steps for the user: User must specify "-I /usr/include/node" and also make sure that their V8 library is on the include path, but that's SOP for building against a library in a custom prefix.

Case 5: Node add-on built against Python as well (Python is assumed to have its own "node.h" header. This is the first objection from the earlier comment.) Delivered files: $PREFIX/include/node/node.h and/or v8.h, depending on whether V8 is being statically or dynamically linked. Required steps for the user: User must specify "-I $PREFIX/include" and use #include <node/node.h> or #include <python/node.h>.

Observation: it would be better of users were advised to #include <node/node.h>, and if including node.h also included the appropriate v8.h header file. (If built with a static v8, it would #include "v8.h". If dynamic, it would #include <v8.h>.) In this case, the "Required steps for the user" in cases 1, 3, and even 5 would be nothing at all, and cases 2 and 4 would be reduced to what the user already expects to do in these cases. Most of the time, it's likely safe to replace #include <node.h> with #include <node/node.h> in an add-on source because the prefix into which Node is installed will usually be on the include path anyway.

In all of the above, V8 could be replaced with other libraries that Node depends on. The full matrix is large, but each choice is independent, and the same policy should apply to each one.

There are a few other cases that come up for libraries like SSL:

Case 6: Node built statically with OpenSSL, add-on wants to link dynamically with OpenSSL.
In general, this will not work because when the add-on uses an OpenSSL symbol that was built into Node, the version in Node will be used instead of the one from the dynamically linked shared library. When the add-on uses an OpenSSL symbol that was not built into Node, the version from the library will be used. This results in mismatched structures being passed to functions, resulting in arbitrarily broken behavior. (Sometimes it will work by accident, sometimes the program will crash, and sometimes data will be corrupted.)

Case 7: Node built dynamically with OpenSSL, add-on wants to link dynamically with the same OpenSSL version. This will work, with both Node and the add-on using the same set of header files and library files delivered with the system, so there are no symbol or structure mismatches.

Case 8: Node built dynamically with OpenSSL, add-on wants to link statically with OpenSSL.
This can work, but only if OpenSSL structure are never passed between Node and the add-on.

Case 9: Node built statically with OpenSSL, add-on wants to be built statically with OpenSSL.
Same as case 8.

What about node_gyp?

The suggested change doesn't break node_gyp, but allows it to use a much simpler algorithm in the future: instead of trying to infer the version of Node for which the add-on is being built and then making sure the source is available (usually by downloading it at build-time), node_gyp doesn't need to do anything except make sure that the PREFIX in which node is installed is on the header include path. It doesn't need to download the Node source or anything. (Obviously, it will need to fallback to that logic for existing installations.)

Importantly, this approach works exactly the same whether you're working with an official Node release or a custom build of Node. There's no need for the --nodedir option, or for people to have to think about it.

A few people have pointed out that this "works by accident" today: an example case where the current approach is logically wrong but happens to work is Case 3 above. In that case, node-gyp puts $node_checkout/... on the include path to pick up node.h, but that causes the add-on to pick up the v8.h in that directory instead of the one in /usr/include (which is, by definition of Case 3, the correct one). This will generate a program with arbitrarily broken behavior at runtime unless the V8 versions in question are "close enough".

tjfontaine commented 10 years ago

@TooTallNate fwiw in the change I'm preparing we'll be shipping the generated config.gypi which can then be used to inspect the variables we already use in the build system to determine what and where the libraries were used from, so from a perspective of node-gyp changes they should be relatively trivial in the form of path.exists(path.join($PREFIX, 'include', 'node', 'config.gypi')) do the new stuff, otherwise do the old thing. Then you can make your decisions in addon.gypi appropriately

tjfontaine commented 10 years ago

After #6332 lands I will also do the work to submit the PR for node-gyp as well

TooTallNate commented 10 years ago

Can somebody please tell me how Windows fits into this whole equation?

trevnorris commented 10 years ago

There's no need for the --nodedir option, or for people to have to think about it.

I'm just going to assume you were over generalizing the topic. I use that option several times a day for things like bisect'ing core to find the breaking commit with a module.

tjfontaine commented 10 years ago

The option isn't pertinent in a world where node-gyp can infer the information based on what was available from make install

TooTallNate commented 10 years ago

@tjfontaine I'm sure Trev doesn't do a make install every time he builds a debug build trying to bisect a bug...

tjfontaine commented 10 years ago

I am not saying the option should be removed, just that for the majority of use cases it will be redundant

tjfontaine commented 10 years ago

Also when I bisect on manta I do make install for every commit :)

trevnorris commented 10 years ago

Heh, and I don't have node globally installed at all.

davepacheco commented 10 years ago

@trevnorris My comment was supposed to be about the automated building and packaging Node as part of a broader system, not the development process within Node itself. Today, if I develop patches to Node (e.g., to float a local patch before it lands upstream), all of the consumers of my Node have to build their modules with the --nodedir option. With this proposal, users of my Node build could treat it just like any other Node build with respect to building. As a result, I think end users of Node would likely never need to use --nodedir. But I wouldn't claim to know enough about other use cases to suggest removing it or anything. (Indeed, it sounds like it's still very useful for core developers!) Sorry for the confusion.

bnoordhuis commented 10 years ago

My overarching beef with this proposed change is that it's basically saying "hey, node-gyp is broken, let's fix that by introducing yet another build system."

No, that's not how it works - if node-gyp doesn't cover all cases, we fix node-gyp. We picked node-gyp as the way to build add-ons and we should stick with it.

On the subject of node.js linked against shared libraries: that's never been the recommended way to run node, there's a reason we ship our deps in the tarball.

If packagers insist on that folly (and note that for example Gentoo is revisiting the unbundling of V8), the best thing we can do is to hide (in the RTLD_LOCAL sense) our dependencies; if an add-on then wants to link against e.g. OpenSSL, it gets its own copy. The major benefit is that add-ons can evolve at their own pace; they're not intrinsically linked to node's deps upgrade cycle.

The only dependencies that won't work for are libv8 and libuv. Well, too bad - linking against versions of those libraries other than the ones we shipped has never been supported.

So, still a firm -1 from me. And because I'm the guy that gets to triage and fix the inevitable fallout, that should count quite heavily.

davepacheco commented 10 years ago

I apologize if I made it sound like I thought node_gyp was broken. node_gyp is terrific, and I think this change will allow it to work better in a few less common (but still important) cases. Nobody's proposed another build system, either. The reason I enumerated those various cases was just to show that delivering headers should not break anybody trying to integrate Node into their own build system, including the Chromium on BSD case that you mentioned, the couple of cases of mine that I alluded to, and @dpemmons's.

It's important for a project to define boundaries on what's supported and what's unsupported, and it's good to have a golden path. But in this case, we have multiple users trying to do something that was supported in v0.8, was not on the list of things removed in 0.10 but no longer works, and requires significant work for those users to rewrite for 0.10. You have someone offering to do the work -- all 36 lines of code -- to fix the problem, and the resulting change hurts absolutely nobody and should be better even for the vast majority of people who will only ever use node_gyp.

tjfontaine commented 10 years ago

@TooTallNate the windows installer will need tweaked slightly to include the files, but from node-gyp's perspective there shouldn't be any difference in operation. I will add that to #6332

Also, all node core members are responsible for issue triage and mitigation, no one member should feel singled out in the process. I'm sorry @bnoordhuis if you feel like node's issue tracker is solely your responsibility, we can all do better so that you don't feel that way.

In the meantime issues that are submitted that involve the build system (either of node itself or of binary addons) feel free to assign to me, and I will be responsible for that subsystem.

@isaacs if you could weigh in that would be appreciated.

isaacs commented 10 years ago

I think @davepacheco adequate addressed the concerns above. Yes, #6332 needs to be tested on Windows, and probably needs some updates to be made to the Windows build scripts. And, at least eventually, node-gyp should be updated to work with the header files installed with node (for installs) and perhaps sniff for the files in the dev location as if nodedir was used (for cases where it's not installed globally, but being run from a dev folder), and only then fall back to downloading the tarball.

Apart from that, I'm not sure what other objections actually are relevant here. @bnoordhuis, your dissatisfaction with this approach is well known, but since @tjfontaine is willing to take responsibility for owning this portion of the node system (and, indeed, over the last year has been wrestling with it, on every platform, more than anyone), I think it should be his call. If you have use cases in mind, other than Windows, and the ones @davepacheco addressed exhaustively above, then I'd love to hear them.

Even if node-gyp is the only build system we officially support, I see no reason why we should go out of our way to make sure it is the only one that works at all. And, in fact, this allows us to adequately address one complaint I hear frequently about binary addons and node-gyp: they require network access just to build, even if the source is all locally available. Usually I then tell the person about the --nodedir option, and they're reaction is, "Why isn't that the default." I have no good answer for that. If @tjfontaine is willing to do the work to finish this up, and own it going forward, I am all for it.

bnoordhuis commented 10 years ago

And, at least eventually, node-gyp should be updated to work with the header files installed with node (for installs) and perhaps sniff for the files in the dev location as if nodedir was used (for cases where it's not installed globally, but being run from a dev folder), and only then fall back to downloading the tarball.

Yes, add more failure cases, why don't you. We should strive to make software simpler, not more complex.

Even if node-gyp is the only build system we officially support, I see no reason why we should go out of our way to make sure it is the only one that works at all.

Because there is no such thing as "unofficially supported". Once you put it out there, people have a reasonable expectation that you maintain it and address issues in a timely manner.

I am more than a little skeptical that's actually going to happen; I will go out on a limb and say that there's overwhelming historical data that if I don't pick up bug reports, no one will. I don't actually mind that all that much but I don't want to have to support something that I feel is a stupid idea to begin with.

If you, TJ and everyone else show yourself to be good bug tracker citizens in the next six months, hey, I might reconsider.

isaacs commented 10 years ago

@bnoordhuis

Yes, add more failure cases, why don't you. We should strive to make software simpler, not more complex.

This removes a common failure case, in fact.

I am more than a little skeptical that's actually going to happen; I will go out on a limb and say that there's overwhelming historical data that if I don't pick up bug reports, no one will. I don't actually mind that all that much but I don't want to have to support something that I feel is a stupid idea to begin with.

1) Your data is skewed by the fact that you generally do pick up bug reports before anyone else does, and that's your choice. Do you have some data showing that when you've taken an extended vacation, there was no bug tracker activity?

2) Picking up every bug until you are feeling burdened and frustrated is not being a good bug tracker citizen. Please exercise self-care, and know your limits so that you do not get burned out. Heroics aren't helpful in the long run, for you, or for Node.

I appreciate all that you do, and I understand that you feel frustrated that you do more than everyone else. This is me explicitly asking you to do a little bit less.

There is no need for you to be the primary maintainer of the build system, and your reconsideration is not required. Just as you are always willing to send bugs related to streams, npm, and the module system my way, feel free sending bugs related to the build system to @tjfontaine. I know that you are good at delegating, and the build system is already not your primary responsibility. To the extent that it affects your workflow, I'm sure that @tjfontaine will be sympathetic.

bnoordhuis commented 10 years ago

1) Your data is skewed by the fact that you generally do pick up bug reports before anyone else does, and that's your choice. Do you have some data showing that when you've taken an extended vacation, there was no bug tracker activity?

Whenever I'm away for a few days, I come home to a pile of unanswered bug reports. Does that answer your question?

Land this if you want it so much. I'm handing over the triage job to you; from now on I'm only going to focus on stuff that directly affects me and my business.

tjfontaine commented 10 years ago

Most of this was landed in 32478ac