Skip to content

tools: convert addon-verify to remark - #21978

Closed
rubys wants to merge 4 commits into
nodejs:masterfrom
rubys:addon-verify-remark
Closed

tools: convert addon-verify to remark#21978
rubys wants to merge 4 commits into
nodejs:masterfrom
rubys:addon-verify-remark

Conversation

@rubys

@rubysrubys commented Jul 25, 2018

Copy link
Copy Markdown
Member

This is the last use of the remark module. tools/remark-cli and
tools/remark-preset-lint-node remain.

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

@rubys
tools: convert addon-verify to remark
9c66e71
This is the last use of the remark *module*. tools/remark-cli and
tools/remark-preset-lint-node remain.
@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Jul 25, 2018
addaleax
addaleax approved these changes Jul 25, 2018
View reviewed changes

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only suggestion I’d have is that it might be nice to add a comment to the script that explains what it does. :)

richardlau
richardlau reviewed Jul 26, 2018
View reviewed changes
Comment threadtools/doc/addon-verify.js Outdated

const { mkdir, readFileSync, writeFile } = require('fs');
// doc/api/addons.md has a bunch of code. Extract it for verification
// that the c++ code comples and the js code runs.

@richardlaurichardlauJul 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

compiles

vsemozhetbyt
vsemozhetbyt reviewed Jul 26, 2018
View reviewed changes
Comment threadtools/doc/addon-verify.js Outdated

const { mkdir, readFileSync, writeFile } = require('fs');
// doc/api/addons.md has a bunch of code. Extract it for verification
// that the c++ code complles and the js code runs.

@vsemozhetbytvsemozhetbytJul 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

complles -> compiles :)
And maybe c++ -> C++

sagirk
sagirk approved these changes Jul 26, 2018
View reviewed changes

@sagirksagirk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@vsemozhetbyt

This comment has been minimized.

Sign in to view
@vsemozhetbyt

vsemozhetbyt commented Jul 27, 2018

Copy link
Copy Markdown
Contributor

Another CI: https://ci.nodejs.org/job/node-test-pull-request/16031/

@vsemozhetbyt

vsemozhetbyt commented Jul 27, 2018

Copy link
Copy Markdown
Contributor

Rebuild one failed OSX job: https://ci.nodejs.org/job/node-test-commit-osx/20042/

@vsemozhetbyt

vsemozhetbyt commented Jul 27, 2018

Copy link
Copy Markdown
Contributor

Can anybody look into the failing OSX job? Is this related to the PR?

@Trott

Trott commented Jul 27, 2018

Copy link
Copy Markdown
Member

Can anybody look into the failing OSX job? Is this related to the PR?

No, it's a problem related to one or more of:

  • Recently moving to a new macOS provider in CI and the machines are a bit thinner on resources than they used to be, so builds are failing more often.
  • Recently changing addons to build in parallel rather than in series. It's possible all these failures in building addons are revealing a race condition there that isn't showing up elsewhere.

Those bullet points above are actually me just paraphrasing what others have told me, not anything I have much knowledge of, so.... @rvagg@refack@addaleax

@Trott

Trott commented Jul 27, 2018

Copy link
Copy Markdown
Member

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/16036/

@addaleax

addaleax commented Jul 27, 2018

Copy link
Copy Markdown
Member

@Trott Yes, it’s odd – it looks like it starts building addons before out/Release/node is created … that shouldn’t happen, as I understand the Makefile? @nodejs/build-files

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 27, 2018
@refack

refack commented Jul 27, 2018

Copy link
Copy Markdown
Contributor

#22006 tracking issue for this flakiness.

@vsemozhetbyt

vsemozhetbyt commented Jul 28, 2018

Copy link
Copy Markdown
Contributor

Landed in 3ffd689
Thank you!

vsemozhetbyt pushed a commit that referenced this pull request Jul 28, 2018
@rubys@vsemozhetbyt
tools: convert addon-verify to remark
3ffd689
This is the last use of the remark *module*. tools/remark-cli and
tools/remark-preset-lint-node remain.
PR-URL: #21978
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targostargos added backport-requested-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 31, 2018
@targos

targos commented Jul 31, 2018

Copy link
Copy Markdown
Member

Depends on #21697 to land on v10.x-staging

targos pushed a commit that referenced this pull request Aug 7, 2018
@rubys@targos
tools: convert addon-verify to remark
44edfa4
This is the last use of the remark *module*. tools/remark-cli and
tools/remark-preset-lint-node remain.
PR-URL: #21978
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvaggrvagg mentioned this pull request Aug 13, 2018
Merged
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@snyk-botsnyk-bot mentioned this pull request Apr 1, 2020
Open
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
@rubys@rvagg
tools: convert addon-verify to remark
0c67d32
This is the last use of the remark *module*. tools/remark-cli and
tools/remark-preset-lint-node remain.
PR-URL: nodejs/node#21978
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richardlaurichardlaurichardlau left review comments
@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@lpincalpincalpinca approved these changes
@cjihrigcjihrigcjihrig approved these changes
@gireeshpunathilgireeshpunathilgireeshpunathil approved these changes
+2 more reviewers
@vsemozhetbytvsemozhetbytvsemozhetbyt left review comments
@sagirksagirksagirk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

docIssues and PRs related to the documentations.toolsIssues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

13 participants

@rubys@vsemozhetbyt@Trott@addaleax@refack@targos@jasnell@lpinca@cjihrig@sagirk@richardlau@gireeshpunathil@nodejs-github-bot