Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

build: install node and static library headers #6332

Closed
wants to merge 1 commit into from

Conversation

tjfontaine
Copy link

fixes #5112

@TooTallNate
Copy link

This should probably be opt-in with a configure flag (--install-headers, or something). This is mostly for .deb maintainers and the likes right?

@tjfontaine
Copy link
Author

This is to restore the functionality that we used to have in v0.8 largely, it is useful for everyone, but especially helpful for platform distributors, these are tiny files and I see no reason for it to be opt in, especially since it will be most helpful moving forward for node-gyp.

@trevnorris
Copy link

It feels strange doing an #include <node/node_buffer.h>

Feel like this needs some more thinking through. Like, just include
everything user facing in node.h, or the include should be like
<node/buffer.h>

@bnoordhuis
Copy link
Member

You can find a more elaborate reply here but the tl;dr is "-1, do not want."

@TooTallNate
Copy link

I still feel link bundling the headers, gzipped, in the node executable would be way more reliable. So basically a -1 from me as well for the reasons that @bnoordhuis has been pointing out. Plus we gotta remember about Windows, which I think you guys are starting to overlook ;)

@trevnorris
Copy link

I'm still not for how we'd be exporting the headers. having node_<name>.h seems really clunky. Instead we should clean up internally how we export stuff. I don't get why we can't just have a single include/node.h that includes everything we deem appropriate for public consumption.

@tjfontaine
Copy link
Author

We can talk about changing the names in master but for backward compat
we still need to provide those names that addons are expecting

files = [dirpath + '/' + f for f in filenames if f.endswith('.h')]
ret[dest + dirpath.replace(path, '')] = files
for subdir, files in ret.items():
action(files, subdir + '/')
Copy link
Member

Choose a reason for hiding this comment

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

Why not move dest + there ?

@indutny
Copy link
Member

indutny commented Dec 13, 2013

Linux part LGTM

@tjfontaine
Copy link
Author

after discussion with @isaacs this was partially landed, restoring the unix portions of this in 32478ac

the windows portions will land after I've addressed @piscisaureus's concerns

@jasnell
Copy link
Member

jasnell commented Aug 13, 2015

@misterdjules @orangemocha ... any reason to keep this one open? I can't imagine any.

@jasnell jasnell added the build label Aug 15, 2015
@jasnell
Copy link
Member

jasnell commented Aug 16, 2015

Closing this here. If there is any reason to pursue this, a new PR would need to be opened in nodejs/master once the v0.10 branch moves over.

@jasnell jasnell closed this Aug 16, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants