-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Add task for building docs only, using existing Node #3888
Conversation
@nodejs/documentation |
out/doc/api/%.html: doc/api/%.markdown $(NODE_EXE) | ||
$(NODE) tools/doc/generate.js --format=html --template=doc/template.html $< > $@ | ||
out/doc/api/%.html: doc/api/%.markdown | ||
$(NODE) tools/doc/generate.js --node-version=$(RAWVER) --format=html --template=doc/template.html $< > $@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should probably be FULLVERSION
to take into account nightlies and other other versions with prerelease tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rvagg Ah, thank you! I pushed a new commit that corrects that. I realized it was a mistake anyway since I overlooked that process.version
is prefixed with v
and RAWVER
isn't.
nice, I like this. /cc @nodejs/build |
This LGTM once @rvagg's suggestion has been addressed. Great work! |
+1 on this... I haven't gone through the commit myself so I won't sign off but this is quite valuable I think. Thank you! |
Hmmm,
Looks like this is the culprit:
If you are trying, as stated, to |
@tflanagan Thanks for taking a look. The issue that I was having is that the |
@jmm, ah! With that, its working like a charm. Well done, I like it! Just one more issue:
Getting blank HTML files. Edit: |
@jmm Hmm, started from scratch and the HTML files have content now. Not sure what happened, going to chalk it up to a dirty environment. |
Thanks @jasnell @rvagg @chrisdickinson! I pushed new commits that fix the issue @rvagg pointed out and I took a crack at updating the |
@tflanagan Thanks! Not sure what would've resulted in the blank HTML files, I haven't run into that. Hopefully it is just a fluke like you said. |
@@ -102,6 +102,8 @@ the binary verification command above. | |||
|
|||
### Unix / Macintosh | |||
|
|||
Build Node.js (see below if you just want to build documentation with an existing Node.js): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather this line not be modified, it doesn't add enough value to justify the noise. If someone comes looking for how to build docs they will see the section below.
@jmm squash commits and prefix the message with |
@rvagg Ok, thanks for the feedback, I removed that line and squashed, with |
lgtm, thanks for the work on this @jmm |
I think part of the slowness of building node is due to not recommending |
Good point @Fishrock123 . |
Huh, didn't know about that. |
Thanks @rvagg. @Fishrock123 If there's an easy recommendation that speeds up the build in general that sounds great, but for practical purposes it doesn't help [me at least] with this. With |
what is holding this up? @jmm can you do a rebase against master and force push. I'll see about getting this landed 😄 |
@thealphanerd Thanks, done 🎉. Slight change: moved this part after the code:
BTW (msg, [offset, length,] port, address[, callback]) I can probably fix that if this gets merged and anyone cares about that. |
ci: https://ci.nodejs.org/job/node-test-pull-request/1919/ I'm going to test this more manually later today |
Allows building just docs using existing Node instead of building Node first. PR-URL: nodejs#3888 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
These signatures were originally converted to opts hashes in nodejs#3888. That change was misinterpreted as the intrinsic cause of a test failure and reverted in nodejs#6680. PR-URL: nodejs#6690 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
After the nodejs#3888 it was not possible to "make doc-only" in some situations. This now fallsback to any installed node version and throws "node not found" in error case. Ref: nodejs#3888 PR-URL: nodejs#6906 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Allows building just docs using existing Node instead of building Node first. PR-URL: #3888 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Allows building just docs using existing Node instead of building Node first. PR-URL: #3888 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Allows building just docs using existing Node instead of building Node first. PR-URL: #3888 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Allows building just docs using existing Node instead of building Node first. PR-URL: #3888 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
This is a WIP attempt to make it easy to build just the docs using an existing Node, instead of having to build Node first, which can be prohibitively slow. Related #3749. The primary reason I call it a WIP is because Make is not my forte :/
/cc @nodejs/documentation @chrisdickinson (per suggestion).
I saw that in
Makefile
there's a conditionally setNODE
var that's used for building the docs (among other things), so I based this on utilizing that:NODE=node make doc-only
.Summary of changes:
NODE
usingNODE_EXE
to reduce duplication.tools/doc/generate.js
to read a--node-version
argument, which will be used to output the version to the docs instead ofprocess.version
. Setup the relevant Make recipe to pass it.tools/doc/html.js
'stoHTML
andrender()
to accept options hashes (the signatures would be getting pretty unwieldy if I'd addednode_version
, which I'm propagating to those functions fromgenerate.js
).doc-only
phony target.$(NODE_EXE)
anddoc-only
prerequisites ofdoc
.$(NODE_EXE)
as a prerequisite of theout/doc/api/%.json
andout/doc/api/%.html
targets. I'm hoping that it being a prerequisite ofdoc
takes care of it for any case where it's needed by these targets (the only one I'm sure of ismake doc
).The changes to the Make targets are the parts I'm least confident about. I'd appreciate if someone who understands Make better can check that it's a sensible setup, and especially that it won't break stuff.