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: make test-doc and lint addon docs #16377

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

Prepping for nodejs/build#929

  • Implements the make test-doc target that build, verify and lint docs
  • Lint the C++ snippets in addon docs
  • When generating addons and running the JS linter, use the global node executable if it is not built. Therefore one does not have to build node in order to run make test-doc.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test, tools

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 22, 2017
@joyeecheung
Copy link
Member Author

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Oct 22, 2017

BTW, why we abstain from making docs and running recently added doc tests on Windows? Is it from vcbuild.bat complication? It would be handy to have an option to make test doc builds on Windows locally.

cc @nodejs/platform-windows

@gibfahn
Copy link
Member

gibfahn commented Oct 22, 2017

BTW, why we abstain from making docs and running recently added doc tests on Windows? Is it from vcbuild.bat complication? It would be handy to have an option to make test doc builds on Windows locally.

I think it's just waiting for someone to implement.

@gibfahn
Copy link
Member

gibfahn commented Oct 22, 2017

@joyeecheung so what's the difference between make test-doc and make lint-md (#12756)?

Is test-doc going to call lint-md?

@joyeecheung
Copy link
Member Author

@gibfahn That's the plan

@@ -262,7 +263,7 @@ endif

test/addons/.docbuildstamp: $(DOCBUILDSTAMP_PREREQS)
$(RM) -r test/addons/??_*/
$(NODE) $<
[ -x $(NODE) ] && $(NODE) $< || node $<
Copy link
Contributor

Choose a reason for hiding this comment

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

Why just here? shouldn't this be handled in a global manner? Or just err with the message "to run with a precompiled node binary run make NODE=<path_to_node> <target>"

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea

@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 28, 2017

Figured out why the linter failed (ci doesn't build the addon docs before linting them).

@refack I gave the global $(NODE) a try and turns out it's trickier than I thought. Many rules use this pattern but some of them depend on the actual build rule indirectly, so the status of -x ./node might change. I'll separate the refactor into another PR.

New CI: https://ci.nodejs.org/job/node-test-pull-request/11038/

@@ -1095,7 +1095,7 @@ static void at_exit_cb1(void* arg) {
Isolate* isolate = static_cast<Isolate*>(arg);
HandleScope scope(isolate);
Local<Object> obj = Object::New(isolate);
assert(!obj.IsEmpty()); // assert VM is still alive
assert(!obj.IsEmpty()); // assert VM is still alive
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a lint issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, needs two spaces here

@refack
Copy link
Contributor

refack commented Oct 28, 2017

Change looks good, but I'm assuming it's not well covered by CI?
I guess it would be nice if someone manually checked the changed targets (me Windows, no good make tester):

  • test
  • (test/addons/.docbuildstamp)
  • test-ci (CI)
  • test-doc
  • lint-js
  • lint-js-ci (CI)
  • lint-addon-docs
  • lint-ci (CI)

- Implements the make test-doc target that build, verify
  and lint docs
- Lint the C++ snippets in addon docs
- When generating addons and running the JS linter,
  use the global node executable if it is not built.
  Therefore one does not have to build node in order to
  run make test-doc.
@joyeecheung
Copy link
Member Author

Rebased & squashed. New CI: https://ci.nodejs.org/job/node-test-pull-request/11063/

@joyeecheung
Copy link
Member Author

Going to land this later today.

joyeecheung added a commit that referenced this pull request Oct 30, 2017
- Implements the make test-doc target that build, verify
  and lint docs
- Lint the C++ snippets in addon docs
- When generating addons and running the JS linter,
  use the global node executable if it is not built.
  Therefore one does not have to build node in order to
  run make test-doc.

PR-URL: #16377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@joyeecheung
Copy link
Member Author

Landed in 390eda1, thanks!

gibfahn pushed a commit that referenced this pull request Oct 30, 2017
- Implements the make test-doc target that build, verify
  and lint docs
- Lint the C++ snippets in addon docs
- When generating addons and running the JS linter,
  use the global node executable if it is not built.
  Therefore one does not have to build node in order to
  run make test-doc.

PR-URL: #16377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
- Implements the make test-doc target that build, verify
  and lint docs
- Lint the C++ snippets in addon docs
- When generating addons and running the JS linter,
  use the global node executable if it is not built.
  Therefore one does not have to build node in order to
  run make test-doc.

PR-URL: #16377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
- Implements the make test-doc target that build, verify
  and lint docs
- Lint the C++ snippets in addon docs
- When generating addons and running the JS linter,
  use the global node executable if it is not built.
  Therefore one does not have to build node in order to
  run make test-doc.

PR-URL: #16377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
- Implements the make test-doc target that build, verify
  and lint docs
- Lint the C++ snippets in addon docs
- When generating addons and running the JS linter,
  use the global node executable if it is not built.
  Therefore one does not have to build node in order to
  run make test-doc.

PR-URL: nodejs/node#16377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
- Implements the make test-doc target that build, verify
  and lint docs
- Lint the C++ snippets in addon docs
- When generating addons and running the JS linter,
  use the global node executable if it is not built.
  Therefore one does not have to build node in order to
  run make test-doc.

PR-URL: nodejs/node#16377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
- Implements the make test-doc target that build, verify
  and lint docs
- Lint the C++ snippets in addon docs
- When generating addons and running the JS linter,
  use the global node executable if it is not built.
  Therefore one does not have to build node in order to
  run make test-doc.

PR-URL: nodejs#16377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
- Implements the make test-doc target that build, verify
  and lint docs
- Lint the C++ snippets in addon docs
- When generating addons and running the JS linter,
  use the global node executable if it is not built.
  Therefore one does not have to build node in order to
  run make test-doc.

PR-URL: nodejs/node#16377
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label Dec 20, 2017
@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants