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

doc: make doc-only -> fallback to user binary #6906

Closed
wants to merge 1 commit into from
Closed

doc: make doc-only -> fallback to user binary #6906

wants to merge 1 commit into from

Conversation

eljefedelrodeodeljefe
Copy link
Contributor

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

build, doc

Description of change

Fixes:
When checking out a fresh clone or rm -rf out/ or rm ./node make doc-only was not working (Mac OS), failing to find ./node. I think this has slipped, because likely contributors have at least some binary, from e.g. a build of another branch still in out/.

This then would check if $(NODE) was set correctly take it or fallback to a user pre-installed node binary.

Bash syntax and implementation is a little too clever unfortunately, but I guess that won't hurt anyone there.

@eljefedelrodeodeljefe eljefedelrodeodeljefe added doc Issues and PRs related to the documentations. build Issues and PRs related to build files or the CI. labels May 20, 2016
@addaleax addaleax added the tools Issues and PRs related to the tools directory. label May 20, 2016
out/doc/api/%.json: doc/api/%.md
$(NODE) tools/doc/generate.js --format=json $< > $@
[ $(NODE) ] && $(NODE) $(gen-json) || node $(gen-json)
Copy link
Member

Choose a reason for hiding this comment

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

This won’t really work like you want it to, [ $(foobar) ] only checks that foobar is nonempty, not that it corresponds . It should probably be something like [ -x "$(NODE)" ] to make sure that the file is there + executable.

Or you just leave the check out, || takes care of a failure in the first version anyway…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know about -x. Thought -n, -z or nothing at all would be appropriate . amended

@eljefedelrodeodeljefe
Copy link
Contributor Author

@addaleax output is ugly though.

[ -x ./node ] && ./node tools/doc/generate.js --format=json doc/api/vm.md > out/doc/api/vm.json || node tools/doc/generate.js --format=json doc/api/vm.md > out/doc/api/vm.json
Input file = doc/api/vm.md

Do you think having it in ifdefs is better?

@addaleax
Copy link
Member

@eljefedelrodeodeljefe Makefile output is never truly beautiful. :)

It works, so LGTM.

@eljefedelrodeodeljefe
Copy link
Contributor Author

@jasnell because of #3888, could I have your review on this one?

@eljefedelrodeodeljefe
Copy link
Contributor Author

cc @nodejs/build extending #3888 a little to fallback to user binary.

@MylesBorins
Copy link
Contributor

This seems reasonable enough. Works on my machine.

If we are going to be doing this we may want to give some sort of better error if node doesn't exist on the system.

LGTM either way.

eljefedelrodeodeljefe added a commit that referenced this pull request May 23, 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>
@eljefedelrodeodeljefe
Copy link
Contributor Author

Landed in c111cf2. I have some work for error handling in mind. For now I think something like "node not found" is okay and better than "./node not found"

@eljefedelrodeodeljefe eljefedelrodeodeljefe deleted the doc/make-doc-only branch May 23, 2016 21:41
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 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>
rvagg pushed a commit that referenced this pull request Jun 2, 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>
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
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
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
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
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. 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.

3 participants