FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: convert addon-verify to remark by rubys · Pull Request #21978 · nodejs/node · GitHub

/ node Public

tools: convert addon-verify to remark - #21978

Closed
rubys wants to merge 4 commits into
nodejs:masterfrom
rubys:addon-verify-remark
ConversationCommits (4)ChecksFiles changed
Closed

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

Conversation

rubys 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

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-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Jul 25, 2018
addaleax approved these changes Jul 25, 2018
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

richardlau reviewed Jul 26, 2018
View reviewed changes
Comment thread tools/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.

richardlau Jul 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

compiles

vsemozhetbyt reviewed Jul 26, 2018
View reviewed changes
Comment thread tools/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.

vsemozhetbyt Jul 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

sagirk approved these changes Jul 26, 2018
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM!

This comment has been minimized.

Sign in to view

vsemozhetbyt commented Jul 27, 2018

Copy link
Copy Markdown
Contributor

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

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 commented Jul 27, 2018

Copy link
Copy Markdown
Contributor

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

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 commented Jul 27, 2018

Copy link
Copy Markdown
Member

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

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

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 27, 2018

refack commented Jul 27, 2018

Copy link
Copy Markdown
Contributor

#22006 tracking issue for this flakiness.

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
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>
targos added backport-requested-v10.x and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Jul 31, 2018

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
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>
rvagg 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-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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

richardlau richardlau left review comments
jasnell jasnell approved these changes
addaleax addaleax approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
gireeshpunathil gireeshpunathil approved these changes
+2 more reviewers
vsemozhetbyt vsemozhetbyt left review comments
sagirk sagirk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL