tools: add missing headers

[sung-su]

Two header files were missing when expose the include file.
These headers are used in "node_internals.h".

[rvagg]

/cc @nodejs/build

Perhaps we should explore trimming this so we don't have distributed header-bloat.

@sung-su out of interest, what are you needing in node_internals.h that's making you run in to this problem? I'm not sure I've seen a project reading in there.

[sung-su]

Currently build is successful without "node_internals.h".
If so, can I remove "node_internals.h" from "install.py"?

[rvagg]

node_internals.h is required by node.h which is why I guess it's installed. We probably either need to refactor out the dependency or include the full chain.

@bnoordhuis any thoughts on this?

/cc @nodejs/addon-api

[agnat]

Looks like node_internals.h is included if NODE_WANT_INTERNALS is defined. Since add-ons could define it this PR does the right thing, conservatively.

On the other hand nobody seems to use it. The build would fail and we would have a bug report, right? Also, the name node_internals.h suggests you should not define NODE_WANT_INTERNALS. So, trimming it might be an option, too. Just less conservative...

If we decide to remove the headers I suggest we add something along the lines:

#if defined(BUILDING_NODE_EXTENSION) && defined(NODE_WANT_INTERNALS)
# error node internals are not available in add-ons
#endif

[sung-su]

"node_internals.h" header is copied to the "{_prefix}/include/node" by "install.py".
This is provided to 3rd party for use in add-ons.
In my opinion, If the 3rd party user cannot use this header, it should be removed to provides.

[rvagg]

Yeah, I'm +1 on removing if we can do so safely, @agnat's error warning is probably a nice addition too. Anyone want to prepare a pull request for this?

[jasnell]

@sung-su @rvagg ... any updates on this one?

[rvagg]

Marking semver-major and lgtm, @agnat can you do a quick review and provide an lgtm?

[jasnell]

ping @agnat

[agnat]

Hm, ok. That disables the NODE_WANT_INTERNALS facility altogether. Embedders simply include node_internals.h themselves. If that is what we want... I'm ok with it.

Maybe add a comment to drop the whole thing in major+2...

Sorry for the late reply...

[bnoordhuis]

This breaks building node.js itself.

[bnoordhuis]

Not installing node_internals.h is the right thing to do, adding it in 32478ac was probably an oversight. The change to node.h will end up breaking the build, however.

[Fishrock123]

Is this something we should do in v7 then?

[bnoordhuis]

I'll close, install.py was fixed by #6913.