Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

jmm
Copy link
Contributor

@jmm jmm commented Nov 17, 2015

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 set NODE 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:

  • When it's not explicitly set, set NODE using NODE_EXE to reduce duplication.
  • Setup tools/doc/generate.js to read a --node-version argument, which will be used to output the version to the docs instead of process.version. Setup the relevant Make recipe to pass it.
  • Refactor tools/doc/html.js's toHTML and render() to accept options hashes (the signatures would be getting pretty unwieldy if I'd added node_version, which I'm propagating to those functions from generate.js).
  • Make targets
    • I added a doc-only phony target.
    • I made $(NODE_EXE) and doc-only prerequisites of doc.
    • I removed $(NODE_EXE) as a prerequisite of the out/doc/api/%.json and out/doc/api/%.html targets. I'm hoping that it being a prerequisite of doc takes care of it for any case where it's needed by these targets (the only one I'm sure of is make 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.

@mscdex mscdex added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Nov 17, 2015
@jasnell
Copy link
Member

jasnell commented Nov 18, 2015

@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 $< > $@
Copy link
Member

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

Copy link
Contributor Author

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.

@rvagg
Copy link
Member

rvagg commented Nov 18, 2015

nice, I like this. /cc @nodejs/build

@chrisdickinson
Copy link
Contributor

This LGTM once @rvagg's suggestion has been addressed. Great work!

@jasnell
Copy link
Member

jasnell commented Nov 18, 2015

+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!

@tflanagan
Copy link
Contributor

Hmmm,

~/$ node -v
v5.0.0
~/$ git clone https://github.com/jmm/node.git
~/$ cd node
~/node$ git checkout build-just-docs
~/node$ ./configure
~/node$ make doc-only
./node tools/doc/generate.js --node-version=6.0.0 --format=html --template=doc/template.html doc/api/addons.markdown > out/doc/api/addons.html
/bin/sh: 1: ./node: not found
make: *** [out/doc/api/addons.html] Error 127

Looks like this is the culprit:

NODE ?= ./$(NODE_EXE)

If you are trying, as stated, to build just the docs using an existing Node, the ./ shouldn't be there?

@jmm
Copy link
Contributor Author

jmm commented Nov 18, 2015

@tflanagan Thanks for taking a look. The issue that I was having is that the make doc task and its prerequisites depend on the task that builds the Node executable, which takes a long time and isn't necessary for building the docs if you already have a working Node as I do (and I suspect most people do where they'd be working on the docs). The line you highlighted there provides a default value if NODE isn't set, defaulting to the path of the built executable. So because that line conditionally sets NODE that actually provided a ready solution for part of the problem: pointing to the existing Node. I utilized that to make this work via NODE=node make doc-only. The other part was making it so there's a task for building the docs that doesn't build the executable as a prerequisite.

@tflanagan
Copy link
Contributor

@jmm, ah! NODE=node, missed that!

With that, its working like a charm. Well done, I like it!

Just one more issue:

~/node$ NODE=node make doc-only
...
~/node$ stat out/doc/api/assert.html
  File: `out/doc/api/assert.html'
  Size: 0               Blocks: 0          IO Block: 4096   regular empty file
Device: ca01h/51713d    Inode: 2074372     Links: 1
Access: (0644/-rw-r--r--)  Uid: ( 1001/***)   Gid: ( 1001/   ***)
Access: 2015-11-18 18:58:10.000000000 +0000
Modify: 2015-11-18 18:58:10.000000000 +0000
Change: 2015-11-18 18:58:10.000000000 +0000
 Birth: -

Getting blank HTML files.

Edit:
The JSON files look good though.

@tflanagan
Copy link
Contributor

@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.

@jmm
Copy link
Contributor Author

jmm commented Nov 18, 2015

Thanks @jasnell @rvagg @chrisdickinson! I pushed new commits that fix the issue @rvagg pointed out and I took a crack at updating the README. Not sure if you need me to do anything with combining any / all commits or prefixing the messages with anything particular (since these changes affect the build system / README, not an actual subsystem)

@jmm
Copy link
Contributor Author

jmm commented Nov 18, 2015

@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):
Copy link
Member

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.

@rvagg
Copy link
Member

rvagg commented Nov 18, 2015

@jmm squash commits and prefix the message with build: and it should be fine.

@jmm
Copy link
Contributor Author

jmm commented Nov 18, 2015

@rvagg Ok, thanks for the feedback, I removed that line and squashed, with build: prefix.

@rvagg
Copy link
Member

rvagg commented Nov 18, 2015

lgtm, thanks for the work on this @jmm

@Fishrock123
Copy link
Contributor

I think part of the slowness of building node is due to not recommending make -j8 or make -j4, would that help more?

@orangemocha
Copy link
Contributor

Good point @Fishrock123 . make -j $(getconf _NPROCESSORS_ONLN) is how we build Node in CI so it is supported and probably worth adding to the README.

@Fishrock123
Copy link
Contributor

getconf _NPROCESSORS_ONLN

Huh, didn't know about that.

@jmm
Copy link
Contributor Author

jmm commented Nov 20, 2015

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 make -j $(getconf _NPROCESSORS_ONLN) (2) or make -j4 it takes the Intel Pentium T4500 laptop dev server I have this running on ~20 minutes to build. (With -j8 the system is pretty choked and the total time is much slower, at least in one trial run.) With this PR I can do NODE=node make doc-only in ~20 seconds.

@MylesBorins
Copy link
Contributor

what is holding this up?

@jmm can you do a rebase against master and force push. I'll see about getting this landed 😄

@jmm
Copy link
Contributor Author

jmm commented Mar 14, 2016

@thealphanerd Thanks, done 🎉. Slight change: moved this part after the code:

(Where node is the path to your executable.)

BTW tools/doc/json.js is currently issuing a warning and generating incorrect output for dgram socket.send() because it's not designed to handle this kind of signature with multiple params that are optional as a group:

(msg, [offset, length,] port, address[, callback])

I can probably fix that if this gets merged and anyone cares about that.

@MylesBorins
Copy link
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/1919/

I'm going to test this more manually later today

@MylesBorins MylesBorins self-assigned this Mar 14, 2016
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 12, 2016
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>
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 12, 2016
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>
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 12, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
These signatures were originally converted to opts hashes in #3888. That
change was misinterpreted as the intrinsic cause of a test failure and
reverted in #6680.

PR-URL: #6690
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
After the #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: #3888

PR-URL: #6906
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
These signatures were originally converted to opts hashes in #3888. That
change was misinterpreted as the intrinsic cause of a test failure and
reverted in #6680.

PR-URL: #6690
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
After the #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: #3888

PR-URL: #6906
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
These signatures were originally converted to opts hashes in #3888. That
change was misinterpreted as the intrinsic cause of a test failure and
reverted in #6680.

PR-URL: #6690
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
After the #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: #3888

PR-URL: #6906
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
These signatures were originally converted to opts hashes in #3888. That
change was misinterpreted as the intrinsic cause of a test failure and
reverted in #6680.

PR-URL: #6690
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
After the #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: #3888

PR-URL: #6906
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.