build: run `npm install` for doc builds in tarball #8413

addaleax · 2016-09-05T20:41:51Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build

Description of change

Run npm install before building the documentation from release tarballs. The doctool currently depends on js-yaml, which is imported from the tools/eslint subtree; however, release tarballs don’t contain that directory.

Running npm install is clearly not a beautiful solution, but it works, and this has been on my to-do list for long enough… If anyone has better ideas, please voice them!

Fixes: #7872

addaleax · 2016-09-05T20:43:01Z

/cc @jbergstroem ?

CI: https://ci.nodejs.org/job/node-test-commit/4926/

bnoordhuis · 2016-09-06T08:09:21Z

Makefile

@@ -39,6 +39,7 @@ EXEEXT := $(shell $(PYTHON) -c \
 NODE_EXE = node$(EXEEXT)
 NODE ?= ./$(NODE_EXE)
 NODE_G_EXE = node_g$(EXEEXT)
+NPM ?= $(PWD)/deps/npm/bin/npm-cli.js


I'd probably drop $(PWD) because that breaks if the path contains spaces (unless you quote its uses below.)

@bnoordhuis Thanks for the hint! I’ve went with quoting the uses below, if that’s okay, that makes it a bit less messy there…

jbergstroem · 2016-09-06T12:09:13Z

~~@addaleax you probs want to use the freshly built node $(NODE) instead of a global one~~

Edit: missed that you only did it if the local one didn't exist. I guess path to $(NODE) might be an issue seeing how you cwd prior.

bnoordhuis · 2016-09-06T12:46:12Z

Makefile

@@ -308,11 +309,17 @@ out/doc/%: doc/%
 # check if ./node is actually set, else use user pre-installed binary
 gen-json = tools/doc/generate.js --format=json $< > $@
 out/doc/api/%.json: doc/api/%.md
+	[ -e tools/doc/node_modules/js-yaml/package.json ] || \
+		[ -e tools/eslint/node_modules/js-yaml/package.json ] || \
+		( cd tools/doc && ( $(NODE) "$(NPM)" install || node "$(NPM)" install ) )


Maybe prefix with [ -x $(NODE) ] or wrap the whole block in a if [ -x $(NODE) ]; then ... fi? That's what the surrounding code does.

Run `npm install` before building the documentation from release tarballs. The doctool currently depends on `js-yaml`, which is imported from the `tools/eslint` subtree; however, release tarballs don’t contain that directory. Running `npm install` is clearly not a beautiful solution, but it works. Fixes: nodejs#7872

addaleax · 2016-09-08T13:08:45Z

Okay, slightly different (and even more verbose…) approach based on @bnoordhuis’ suggestion.

CI: https://ci.nodejs.org/job/node-test-commit/4955/

bnoordhuis · 2016-09-13T12:05:58Z

Not a work of beauty but shell-in-makefiles hardly ever is. LGTM.

jbergstroem · 2016-09-13T12:31:33Z

LGTM

jasnell

LGTM

addaleax · 2016-09-20T12:04:55Z

Landed in 9391cb0, thanks for the reviews!

Run `npm install` before building the documentation from release tarballs. The doctool currently depends on `js-yaml`, which is imported from the `tools/eslint` subtree; however, release tarballs don’t contain that directory. Running `npm install` is clearly not a beautiful solution, but it works. Fixes: #7872 PR-URL: #8413 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com>

This LTS release comes with 219 commits. This includes 80 commits that are docs related, 58 commits that are test related, 20 commits that are build / tool related, and 9 commits that are updates to dependencies. Notable Changes * build: - It is now possible to build the documentation from the release tarball (Anna Henningsen) #8413 * buffer: - Buffer will no longer incorrectly return a zero filled buffer when an encoding is passed (Teddy Katz) #9238 * deps: - upgrade npm in LTS to 2.15.11 (Kat Marchán) #8928 * repl: - Enable tab completion for global properties (Lance Ball) #7369 * url: - `url.format()` will now encode all `#` in `search` (Ilkka Myller) #8072 PR-URL: #9298

This LTS release comes with 219 commits. This includes 80 commits that are docs related, 58 commits that are test related, 20 commits that are build / tool related, and 9 commits that are updates to dependencies. Notable Changes * build: - It is now possible to build the documentation from the release tarball (Anna Henningsen) #8413 * buffer: - Buffer.alloc() will no longer incorrectly return a zero filled buffer when an encoding is passed (Teddy Katz) #9238 * deps: - upgrade npm in LTS to 2.15.11 (Kat Marchán) #8928 * repl: - Enable tab completion for global properties (Lance Ball) #7369 * url: - `url.format()` will now encode all `#` in `search` (Ilkka Myller) #8072 PR-URL: #9298

This LTS release comes with 219 commits. This includes 80 commits that are docs related, 58 commits that are test related, 20 commits that are build / tool related, and 9 commits that are updates to dependencies. Notable Changes * build: - It is now possible to build the documentation from the release tarball (Anna Henningsen) nodejs/node#8413 * buffer: - Buffer.alloc() will no longer incorrectly return a zero filled buffer when an encoding is passed (Teddy Katz) nodejs/node#9238 * deps: - upgrade npm in LTS to 2.15.11 (Kat Marchan) nodejs/node#8928 * repl: - Enable tab completion for global properties (Lance Ball) nodejs/node#7369 * url: - `url.format()` will now encode all `#` in `search` (Ilkka Myller) nodejs/node#8072 PR-URL: nodejs/node#9298 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

addaleax added doc Issues and PRs related to the documentations. build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Sep 5, 2016

bnoordhuis reviewed Sep 6, 2016
View reviewed changes

addaleax force-pushed the build-doctool-npm-install branch from 654f615 to e921852 Compare September 8, 2016 13:06

jasnell approved these changes Sep 15, 2016

View reviewed changes

addaleax added the lts-watch-v4.x label Sep 20, 2016

addaleax closed this Sep 20, 2016

addaleax deleted the build-doctool-npm-install branch September 20, 2016 12:05

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Oct 24, 2016

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

This was referenced Mar 9, 2024

[Snyk] Upgrade @typescript-eslint/eslint-plugin from 6.21.0 to 7.0.1 B020239/node#2

Open

[Snyk] Upgrade @typescript-eslint/parser from 6.21.0 to 7.0.1 B020239/node#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: run `npm install` for doc builds in tarball #8413

build: run `npm install` for doc builds in tarball #8413

addaleax commented Sep 5, 2016

addaleax commented Sep 5, 2016

bnoordhuis Sep 6, 2016

addaleax Sep 6, 2016

jbergstroem commented Sep 6, 2016 •

edited

Loading

bnoordhuis Sep 6, 2016

addaleax commented Sep 8, 2016

bnoordhuis commented Sep 13, 2016

jbergstroem commented Sep 13, 2016

jasnell left a comment

addaleax commented Sep 20, 2016

build: run npm install for doc builds in tarball #8413

build: run npm install for doc builds in tarball #8413

Conversation

addaleax commented Sep 5, 2016

Checklist

Affected core subsystem(s)

Description of change

addaleax commented Sep 5, 2016

bnoordhuis Sep 6, 2016

Choose a reason for hiding this comment

addaleax Sep 6, 2016

Choose a reason for hiding this comment

jbergstroem commented Sep 6, 2016 • edited Loading

bnoordhuis Sep 6, 2016

Choose a reason for hiding this comment

addaleax commented Sep 8, 2016

bnoordhuis commented Sep 13, 2016

jbergstroem commented Sep 13, 2016

jasnell left a comment

Choose a reason for hiding this comment

addaleax commented Sep 20, 2016

build: run `npm install` for doc builds in tarball #8413

build: run `npm install` for doc builds in tarball #8413

jbergstroem commented Sep 6, 2016 •

edited

Loading