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

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

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 5, 2016

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

addaleax commented Sep 5, 2016

@@ -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
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 probably drop $(PWD) because that breaks if the path contains spaces (unless you quote its uses below.)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

jbergstroem commented Sep 6, 2016

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

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

Choose a reason for hiding this comment

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

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
Copy link
Member Author

addaleax commented Sep 8, 2016

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

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

@bnoordhuis
Copy link
Member

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

@jbergstroem
Copy link
Member

LGTM

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member Author

Landed in 9391cb0, thanks for the reviews!

@addaleax addaleax closed this Sep 20, 2016
@addaleax addaleax deleted the build-doctool-npm-install branch September 20, 2016 12:05
addaleax added a commit that referenced this pull request Sep 20, 2016
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>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
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>
MylesBorins pushed a commit that referenced this pull request Oct 14, 2016
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>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2016
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>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
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>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
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>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
MylesBorins added a commit that referenced this pull request Oct 26, 2016
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
MylesBorins added a commit that referenced this pull request Oct 26, 2016
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
MylesBorins added a commit that referenced this pull request Nov 7, 2016
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
MylesBorins added a commit that referenced this pull request Nov 8, 2016
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
imyller added a commit to imyller/meta-nodejs that referenced this pull request Nov 11, 2016
    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>
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. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants