★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21978Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

tools: convert addon-verify to remark #21978

Closed
wants to merge 4 commits into from

Conversation

Reviewers

@richardlau richardlau

@vsemozhetbyt vsemozhetbyt

@jasnell jasnell

@addaleax addaleax

@lpinca lpinca

@cjihrig cjihrig

@sagirk sagirk

@gireeshpunathil gireeshpunathil

Assignees
No one assigned
Labels
doc tools
Projects
None yet
Milestone
No milestone
@rubys
Copy link
Member

rubys commented Jul 25, 2018

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 
This is the last use of the remark *module*.  tools/remark-cli and
tools/remark-preset-lint-node remain.
Loading status checks…
9c66e71

@nodejs-github-bot nodejs-github-bot added doc tools labels Jul 25, 2018

@addaleax

addaleax approved these changes Jul 25, 2018
View changes

Copy link
Member

addaleax left a comment

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

gireeshpunathil approved these changes Jul 25, 2018
View changes

@richardlau

richardlau reviewed Jul 26, 2018
View changes

tools/doc/addon-verify.js Outdated
@@ -1,6 +1,11 @@
'use strict';

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.

This comment has been minimized.

Sign in to view
Copy link
@richardlau

richardlau Jul 26, 2018

Member

compiles
@vsemozhetbyt

vsemozhetbyt reviewed Jul 26, 2018
View changes

tools/doc/addon-verify.js Outdated
@@ -1,28 +1,38 @@
'use strict';

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.

This comment has been minimized.

Sign in to view
Copy link
@vsemozhetbyt

vsemozhetbyt Jul 26, 2018

Member

complles -> compiles :)
And maybe c++ -> C++
@rubys
fix typos
Loading status checks…
ad6d493
@sagirk

sagirk approved these changes Jul 26, 2018
View changes

Copy link
Member

sagirk left a comment

LGTM!
@vsemozhetbyt

This comment was marked as outdated.

Sign in to view
Copy link
Member

vsemozhetbyt commented Jul 26, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16017/
@lpinca

lpinca approved these changes Jul 26, 2018
View changes

@cjihrig

cjihrig approved these changes Jul 27, 2018
View changes

@jasnell

jasnell approved these changes Jul 27, 2018
View changes

@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Jul 27, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Jul 27, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Jul 27, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Jul 27, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Jul 27, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

addaleax commented Jul 27, 2018

@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 Trott added the author ready label Jul 27, 2018

@refack

This comment has been minimized.

Sign in to view
Copy link
Member

refack commented Jul 27, 2018

#22006 tracking issue for this flakiness.
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Jul 28, 2018

Landed in 3ffd689
Thank you!

@vsemozhetbyt vsemozhetbyt closed this Jul 28, 2018

vsemozhetbyt added a commit that referenced this pull request Jul 28, 2018

@rubys @vsemozhetbyt
tools: convert addon-verify to remark 
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>
Loading status checks…
3ffd689

@targos targos added backport-requested-v10.x and removed author ready labels Jul 31, 2018

@targos

This comment has been minimized.

Sign in to view
Copy link
Member

targos commented Jul 31, 2018

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

@targos targos removed the backport-requested-v10.x label Aug 7, 2018

targos added a commit that referenced this pull request Aug 7, 2018

@rubys @targos
tools: convert addon-verify to remark 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
44edfa4

@rvagg rvagg referenced this pull request Aug 13, 2018

Merged

Release proposal: v10.9.0 #22295

This was referenced Aug 16, 2018

Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.