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

create/update the "include" #5047

Closed
trevnorris opened this issue Mar 17, 2013 · 10 comments
Closed

create/update the "include" #5047

trevnorris opened this issue Mar 17, 2013 · 10 comments
Milestone

Comments

@trevnorris
Copy link

don't know why took me so long to see it, but realized while working on a recent bug that src/node.h is basically node's "include". i'd like maintainers input on creating an official "include" that has Doxygen comments, the same way that v8 uses. Then it'd be easy to create a complete official documentation for c++ module authors from both.

thoughts?

CURRENT TOPICS

Likely implemented:

  • All headers for node and its deps will available as #include <node/*> (e.g. <node/node.h>, <node/v8.h>).
  • Add NODE_DEPRECATED to be used in tangent with utils.deprecte() for phasing out API changes.
  • Add doxygen style comments to all applicable headers, and a Doxyfile used to generate for node an all deps.

Possibly implemented:

  • Create a single node.h that will contain all user facing API.
@bnoordhuis
Copy link
Member

A +0.1 from me. It's not just src/node.h though, src/node_buffer.h and src/node_object_wrap.h are also part of the API.

@trevnorris
Copy link
Author

@bnoordhuis I'm not for or against this, but want your feedback on creating a single "include/node.h" that contains all the user facing APIs?

@tjfontaine I believe you also had some opinion on header maintenance and how it should be handled.

@bnoordhuis
Copy link
Member

I'm open to the idea. To prevent add-on breakage, node_buffer.h and node_object_wrap.h will have to stick around for a while but they can just include the new header.

Maybe now would be a good time to address an issue that hit people when we installed headers site-wide and will resurface if #5112 goes through: node.h is a rather generic name that conflicts with several packages, notably python-dev and ruby-dev. If you screw up your include path, #include <node.h> includes the wrong file.

Two possible solutions:

  1. Rename the header to e.g. nodejs.h, or
  2. Educate add-on authors to use #include <node/node.h> (and fix node-gyp and our source tree accordingly.)

That said, it doesn't fix the issue for people that have libv8-dev installed - they still run the risk of including the wrong v8.h. Thoughts?

@trevnorris
Copy link
Author

@bnoordhuis I'm still new to c++ so correct me if I'm wrong, but can't you use all the headers automatically included in your includes? I mean, I've gotten into the habit of just leaving out the #include <v8.h> from my node modules. Wouldn't that guarantee the use of the correct v8 headers?

@isaacs
Copy link

isaacs commented Mar 28, 2013

I definitely am strongly in favor of documenting our public C++ interfaces with doxygen, and including that (and the generated V8 docs) with the node documentation.

As for the stuff in #5112, I think that @bnoordhuis makes a strong case. That's why I originally suggested that it should be a non-default make target. Putting it at node/node.h is probably a good idea, and node-gyp can be trivially updated to work with both.

@bnoordhuis
Copy link
Member

I've gotten into the habit of just leaving out the #include <v8.h> from my node modules. Wouldn't that guarantee the use of the correct v8 headers?

Yes, that works but not every add-on author does that. (Including me. I use #include "v8.h" in node-agnostic files.)

@trevnorris
Copy link
Author

I use #include "v8.h" in node-agnostic files.

Forgot about that case. Then if node will also be installing the header files of all deps it'd make sense, to me, to include them all in the same sub-folder structure (e.g. <node/node.h>, <node/v8.h>, <node/uv.h>, etc.)

@chrislea
Copy link

I am not a developer for node (wish I had that skillset), but I do package it for Ubuntu. It seems to me that since you guys ship a very "moving target" v8, and since people also do build modules against each version of node, it would make sense to always include the headers for any given version in a "standard" place that's not /usr/include. Either /usr/include/node or /usr/include/nodejs would seem the likely candidates.

@trevnorris
Copy link
Author

For everyone, I'll be updating the main message body with the current objectives of this ticket as they are discussed. Just to help prevent scattered discussion as this goes on.

@trevnorris
Copy link
Author

Pretty sure @tjfontaine has another idea how to handle this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants