build: add crypto check to markdown lint target #21326

danbev · 2018-06-14T06:52:43Z

Currently, if configured --without-ssl the following error will be
repored by remark-cli:

Running Markdown linter on misc docs...
internal/util.js:100
    throw new ERR_NO_CRYPTO();
    ^

Error [ERR_NO_CRYPTO]: Node.js is not compiled with OpenSSL crypto support
    at assertCrypto (internal/util.js:100:11)
    at crypto.js:31:1
    at NativeModule.compile (internal/bootstrap/loaders.js:235:7)
    at Function.NativeModule.require
      (internal/bootstrap/loaders.js:155:18)
    at Function.Module._load (internal/modules/cjs/loader.js:530:25)
    at Module.require (internal/modules/cjs/loader.js:650:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous>
      (/node/tools/remark-cli/node_modules/math-random/node.js:1:76)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js
      (internal/modules/cjs/loader.js:713:10)
make[1]: *** [tools/.miscmdlintstamp] Error 1
make: *** [lint] Error 2

This commit adds a check for crypto to avoid this error when node has
been configured without crypto support. The alternative was to try to
fix this in randomatic but that lead to another dependency failing
(uuid) and felt like this might be simpler.

Checklist

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

Currently, if configured --without-ssl the following error will be repored by remark-cli: Running Markdown linter on misc docs... internal/util.js:100 throw new ERR_NO_CRYPTO(); ^ Error [ERR_NO_CRYPTO]: Node.js is not compiled with OpenSSL crypto support at assertCrypto (internal/util.js:100:11) at crypto.js:31:1 at NativeModule.compile (internal/bootstrap/loaders.js:235:7) at Function.NativeModule.require (internal/bootstrap/loaders.js:155:18) at Function.Module._load (internal/modules/cjs/loader.js:530:25) at Module.require (internal/modules/cjs/loader.js:650:17) at require (internal/modules/cjs/helpers.js:20:18) at Object.<anonymous> (/node/tools/remark-cli/node_modules/math-random/node.js:1:76) at Module._compile (internal/modules/cjs/loader.js:702:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10) make[1]: *** [tools/.miscmdlintstamp] Error 1 make: *** [lint] Error 2 This commit adds a check for crypto to avoid this error when node has been configured without crypto support. The alternative was to try to fix this in randomatic but that lead to another dependency failing (uuid) and felt like this might be simpler.

nodejs-github-bot · 2018-06-14T06:52:44Z

@danbev build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/65/pipeline

bnoordhuis · 2018-06-14T11:32:01Z

Makefile

@@ -1048,9 +1048,13 @@ LINT_MD_DOC_FILES = $(shell ls doc/**/*.md)
 run-lint-doc-md = tools/remark-cli/cli.js -q -f $(LINT_MD_DOC_FILES)
 # Lint all changed markdown files under doc/
 tools/.docmdlintstamp: $(LINT_MD_DOC_FILES)
+ifeq ("$(HAS_CRYPTO)", "true")


Slightly more idiomatic: ifeq ($(HAS_CRYPTO),true)

I'll update this shortly.

bnoordhuis · 2018-06-14T11:33:35Z

configure

@@ -1517,6 +1517,7 @@ config = {
  'BUILDTYPE': 'Debug' if options.debug else 'Release',
  'PYTHON': sys.executable,
  'NODE_TARGET_TYPE': variables['node_target_type'],
+  'HAS_CRYPTO': variables['node_use_openssl'],


I'd call this NODE_USE_OPENSSL for consistency with NODE_TARGET_TYPE (and also grep -i greppability.)

I'll change this as well. Thanks

richardlau

So this is causing the linter jobs (both on Travis and the CI) to skip doc linting:
https://travis-ci.com/nodejs/node/jobs/129356058
https://ci.nodejs.org/blue/rest/organizations/jenkins/pipelines/node-linter/runs/27/nodes/25/log/?start=0

$ NODE=$(which node) make lint-ci
Running JS linter...
Running C++ linter...
Total errors found: 0
Skipping Markdown linter on misc docs (no crypto)
Skipping Markdown linter on docs (no crypto)

The lint jobs use an existing Node.js binary.

danbev · 2018-06-14T14:23:03Z

I’ll take a look. Thanks tors 14 juni 2018 kl. 16:04 skrev Richard Lau <notifications@github.com>:

…

***@***.**** requested changes on this pull request. So this is causing the linter jobs (both on Travis and the CI) to skip doc linting: https://travis-ci.com/nodejs/node/jobs/129356058 https://ci.nodejs.org/blue/rest/organizations/jenkins/pipelines/node-linter/runs/27/nodes/25/log/?start=0 $ NODE=$(which node) make lint-ci Running JS linter... Running C++ linter... Total errors found: 0 Skipping Markdown linter on misc docs (no crypto) Skipping Markdown linter on docs (no crypto) The lint jobs use an existing Node.js binary. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21326 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaY37nWro8IZq3zR22qwb8lA-dcSLL2ks5t8m1ygaJpZM4UnZs4> .

The lint task my be run without configure being run in which case there will be no config.mk. The value is set to true as this would be the most common configuration I think.

richardlau

Travis linter now works: https://travis-ci.com/nodejs/node/jobs/129401932

danbev · 2018-06-14T16:30:34Z

@richardlau I think I'm looking at the correct log and it looks like it is not skipping anymore.

refack · 2018-06-14T16:32:33Z

Makefile

@@ -1048,9 +1049,13 @@ LINT_MD_DOC_FILES = $(shell ls doc/**/*.md)
 run-lint-doc-md = tools/remark-cli/cli.js -q -f $(LINT_MD_DOC_FILES)
 # Lint all changed markdown files under doc/
 tools/.docmdlintstamp: $(LINT_MD_DOC_FILES)
+ifeq ($(NODE_USE_OPENSSL),true)


Since this step anyway requires an existing node binary, why not simply test @$(call available-node,"-p require('crypto')"). Then you can remove all the other "breadcrumbs".

Or -p process.config.variables.node_use_openssl

Good point, it would be safer to do that as the existing node binary might have been compiled without openssl (might not be the common case but could happen). I'll take a look and update. Thanks

This commit checks the node environment to determine if openssl (crypto) support if available.

danbev · 2018-06-19T05:32:28Z

@refack Would you be able to take a look at the latest commit and see what you think? Thanks

refack

💯

refack · 2018-06-21T14:43:12Z

/CC @nodejs/build-files

danbev · 2018-06-24T04:59:25Z

Landed in 6ced651.

Currently, if configured --without-ssl the following error will be repored by remark-cli: Running Markdown linter on misc docs... internal/util.js:100 throw new ERR_NO_CRYPTO(); ^ Error [ERR_NO_CRYPTO]: Node.js is not compiled with OpenSSL crypto support at assertCrypto (internal/util.js:100:11) at crypto.js:31:1 at NativeModule.compile (internal/bootstrap/loaders.js:235:7) at Function.NativeModule.require (internal/bootstrap/loaders.js:155:18) at Function.Module._load (internal/modules/cjs/loader.js:530:25) at Module.require (internal/modules/cjs/loader.js:650:17) at require (internal/modules/cjs/helpers.js:20:18) at Object.<anonymous> (/node/tools/remark-cli/node_modules/math-random/node.js:1:76) at Module._compile (internal/modules/cjs/loader.js:702:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10) make[1]: *** [tools/.miscmdlintstamp] Error 1 make: *** [lint] Error 2 This commit adds a check for crypto to avoid this error when node has been configured without crypto support. The alternative was to try to fix this in randomatic but that lead to another dependency failing (uuid) and felt like this might be simpler. PR-URL: #21326 Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jun 14, 2018

ryzokuken approved these changes Jun 14, 2018

View reviewed changes

bnoordhuis approved these changes Jun 14, 2018

View reviewed changes

tniessen approved these changes Jun 14, 2018

View reviewed changes

danbev added 2 commits June 14, 2018 15:14

squash: rename HAS_CRYPTO to NODE_USE_OPENSSL

043bcaa

squash: fix if statement

ea20291

cjihrig approved these changes Jun 14, 2018

View reviewed changes

richardlau requested changes Jun 14, 2018

View reviewed changes

squash: add default value for NODE_USE_OPENSSL

d8aa677

The lint task my be run without configure being run in which case there will be no config.mk. The value is set to true as this would be the most common configuration I think.

richardlau approved these changes Jun 14, 2018

View reviewed changes

refack suggested changes Jun 14, 2018

View reviewed changes

danbev added 2 commits June 15, 2018 08:58

squash: remove NODE_USE_OPENSSL variable

030a1ee

squash: check process.versions.openssl

5df7691

This commit checks the node environment to determine if openssl (crypto) support if available.

jasnell approved these changes Jun 21, 2018

View reviewed changes

refack approved these changes Jun 21, 2018

View reviewed changes

danbev closed this Jun 24, 2018

danbev deleted the markdown-linter-crypto-check branch June 24, 2018 04:59

targos mentioned this pull request Jul 3, 2018

v10.6.0 proposal #21629

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add crypto check to markdown lint target #21326

build: add crypto check to markdown lint target #21326

danbev commented Jun 14, 2018

nodejs-github-bot commented Jun 14, 2018

bnoordhuis Jun 14, 2018

danbev Jun 14, 2018

bnoordhuis Jun 14, 2018

danbev Jun 14, 2018

richardlau left a comment

danbev commented Jun 14, 2018 via email

richardlau left a comment

danbev commented Jun 14, 2018

refack Jun 14, 2018 •

edited

Loading

refack Jun 14, 2018

danbev Jun 15, 2018

danbev commented Jun 19, 2018

refack left a comment

refack commented Jun 21, 2018

danbev commented Jun 24, 2018

build: add crypto check to markdown lint target #21326

build: add crypto check to markdown lint target #21326

Conversation

danbev commented Jun 14, 2018

Checklist

nodejs-github-bot commented Jun 14, 2018

bnoordhuis Jun 14, 2018

Choose a reason for hiding this comment

danbev Jun 14, 2018

Choose a reason for hiding this comment

bnoordhuis Jun 14, 2018

Choose a reason for hiding this comment

danbev Jun 14, 2018

Choose a reason for hiding this comment

richardlau left a comment

Choose a reason for hiding this comment

danbev commented Jun 14, 2018 via email

richardlau left a comment

Choose a reason for hiding this comment

danbev commented Jun 14, 2018

refack Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

refack Jun 14, 2018

Choose a reason for hiding this comment

danbev Jun 15, 2018

Choose a reason for hiding this comment

danbev commented Jun 19, 2018

refack left a comment

Choose a reason for hiding this comment

refack commented Jun 21, 2018

danbev commented Jun 24, 2018

refack Jun 14, 2018 •

edited

Loading