build: add crypto check to build targets #22148

danbev · 2018-08-06T08:58:11Z

Currently when configured without-ssl the build will fail when trying
to run the tools/doc/node_modules, and .docbuildstamp make targets:

internal/util.js:97
    throw new ERR_NO_CRYPTO();
    ^
Error [ERR_NO_CRYPTO]: Node.js is not compiled with OpenSSL crypto support
    at assertCrypto (internal/util.js:97:11)
    at crypto.js:31:1
    ...
    at Object.<anonymous>
       (/node/deps/npm/node_modules/uuid/lib/rng.js:4:14)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    ...
make[1]: *** [tools/doc/node_modules] Error 1

This commit adds crypto check to these targets to allow the build to
pass.

Checklist

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

Currently when configured without-ssl the build will fail when trying to run the tools/doc/node_modules, and .docbuildstamp make targets: internal/util.js:97 throw new ERR_NO_CRYPTO(); ^ Error [ERR_NO_CRYPTO]: Node.js is not compiled with OpenSSL crypto support at assertCrypto (internal/util.js:97:11) at crypto.js:31:1 ... at Object.<anonymous> (/node/deps/npm/node_modules/uuid/lib/rng.js:4:14) at Module._compile (internal/modules/cjs/loader.js:689:30) ... make[1]: *** [tools/doc/node_modules] Error 1 This commit adds crypto check to these targets to allow the build to pass.

Currently when configured without-ssl test-heapdump-http2.js will fail with a missing crypto message. This commit moves the require of http2 to after the crypto check.

Currently when configured without-ssl test-request-arguments.js will fail with a missing crypto message. This commit moves the require of https to after the crypto check.

nodejs-github-bot · 2018-08-06T08:58:12Z

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

danbev · 2018-08-06T08:59:08Z

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

lpinca · 2018-08-06T19:26:05Z

CI is failing.

danbev · 2018-08-07T09:12:14Z

@lpinca Sorry, I noticed this yesterday but did not have time to investigate further. Looking into it now though. Thanks.

This commit moves the declaration of node_use_openssl so that it comes before the first usage as it needs to be available to the ifeq conditions.

danbev · 2018-08-07T10:31:37Z

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

danbev · 2018-08-08T11:51:17Z

Re-run CI: https://ci.nodejs.org/job/node-test-pull-request/16272/

mhdawson

LGTM if CI is green

danbev · 2018-08-09T03:46:19Z

node-test-commit-custom-suites (test-worker) failure looks unrelated

console output:

07:52:05  > git fetch --no-tags --progress git@github.com:nodejs/node.git +refs/heads/*:refs/remotes/origin/* +refs/pull/22148/head:refs/remotes/origin/_jenkins_local_branch # timeout=20
07:52:06 ERROR: Error fetching remote repo 'origin'
07:52:06 hudson.plugins.git.GitException: Failed to fetch from git@github.com:nodejs/node.git
07:52:06 	at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:889)
07:52:06 	at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1146)
07:52:06 	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1177)
07:52:06 	at hudson.scm.SCM.checkout(SCM.java:504)
07:52:06 	at hudson.model.AbstractProject.checkout(AbstractProject.java:1208)
07:52:06 	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:574)
07:52:06 	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
07:52:06 	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:499)
07:52:06 	at hudson.model.Run.execute(Run.java:1798)
07:52:06 	at hudson.matrix.MatrixBuild.run(MatrixBuild.java:323)
07:52:06 	at hudson.model.ResourceController.execute(ResourceController.java:97)
07:52:06 	at hudson.model.Executor.run(Executor.java:429)
07:52:06 Caused by: hudson.plugins.git.GitException: Command "git fetch --no-tags --progress git@github.com:nodejs/node.git +refs/heads/*:refs/remotes/origin/* +refs/pull/22148/head:refs/remotes/origin/_jenkins_local_branch" returned status code 1:

I've been out for a good month remember seeing an email from @Trott about that we should no longer merge a PR if there are CI failures. Could anyone confirm that this is the case and I should hold off on merging until I get a green CI run?

danbev · 2018-08-10T03:49:05Z

Re-running node-test-commit that failed:
https://ci.nodejs.org/job/node-test-commit-custom-suites/604/

danbev · 2018-08-10T05:29:21Z

Landed in 77da6d9, 94840fd, and 346f2a7.

Currently when configured without-ssl the build will fail when trying to run the tools/doc/node_modules, and .docbuildstamp make targets: internal/util.js:97 throw new ERR_NO_CRYPTO(); ^ Error [ERR_NO_CRYPTO]: Node.js is not compiled with OpenSSL crypto support at assertCrypto (internal/util.js:97:11) at crypto.js:31:1 ... at Object.<anonymous> (/node/deps/npm/node_modules/uuid/lib/rng.js:4:14) at Module._compile (internal/modules/cjs/loader.js:689:30) ... make[1]: *** [tools/doc/node_modules] Error 1 This commit adds crypto check to these targets to allow the build to pass. PR-URL: #22148 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

Currently when configured without-ssl test-heapdump-http2.js will fail with a missing crypto message. This commit moves the require of http2 to after the crypto check. PR-URL: #22148 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

Currently when configured without-ssl test-request-arguments.js will fail with a missing crypto message. This commit moves the require of https to after the crypto check. PR-URL: #22148 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

Currently when configured without-ssl the build will fail when trying to run the tools/doc/node_modules, and .docbuildstamp make targets: internal/util.js:97 throw new ERR_NO_CRYPTO(); ^ Error [ERR_NO_CRYPTO]: Node.js is not compiled with OpenSSL crypto support at assertCrypto (internal/util.js:97:11) at crypto.js:31:1 ... at Object.<anonymous> (/node/deps/npm/node_modules/uuid/lib/rng.js:4:14) at Module._compile (internal/modules/cjs/loader.js:689:30) ... make[1]: *** [tools/doc/node_modules] Error 1 This commit adds crypto check to these targets to allow the build to pass. PR-URL: #22148 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

Currently when configured without-ssl test-heapdump-http2.js will fail with a missing crypto message. This commit moves the require of http2 to after the crypto check. PR-URL: #22148 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

Currently when configured without-ssl test-request-arguments.js will fail with a missing crypto message. This commit moves the require of https to after the crypto check. PR-URL: #22148 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

joyeecheung · 2018-11-03T05:09:54Z

This makes our current make process depends on the state of out/BUILDTYPE/node - if the previous build is bad, one has to remove out/BUILDTYPE/node first to rebuild the whole thing.

Why is the doc tools requiring crypto in the first place? That should be avoidable in doc tools scripts right?

refack · 2018-11-03T18:32:52Z

Why is the doc tools requiring crypto in the first place?

Because it does npm install.

I think the change should have been

node_use_openssl = $(shell $(NODE), "-p", "process.versions.openssl != undefined")

since we want to test the built node binary...
At least for the test/addons/.docbuildstamp target.

The other target tools/doc/node_modules I think sould fail, not skipped.

refack · 2018-11-03T18:34:58Z

P.S. @joyeecheung I have a draft of Makefile changes that split it to targets that lead to a node binary, and targets that require a node binary to exist. I think until we do something like that we will keep hitting such dependencies cycles.

joyeecheung · 2018-11-03T20:59:09Z

I don't think tools/doc/node_modules should be PHONY target and gets run always - that's probably the biggest source of interruption here.

EDIT: ah, that has been removed now

danbev added 3 commits August 6, 2018 10:08

test: move require of http2 to after crypto check

d625223

Currently when configured without-ssl test-heapdump-http2.js will fail with a missing crypto message. This commit moves the require of http2 to after the crypto check.

test: move require of https to after crypto check

f1784a4

Currently when configured without-ssl test-request-arguments.js will fail with a missing crypto message. This commit moves the require of https to after the crypto check.

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

jasnell approved these changes Aug 6, 2018

View reviewed changes

lpinca approved these changes Aug 6, 2018

View reviewed changes

squash: move declaration of node_use_openssl

beca02d

This commit moves the declaration of node_use_openssl so that it comes before the first usage as it needs to be available to the ifeq conditions.

cjihrig approved these changes Aug 7, 2018

View reviewed changes

mhdawson approved these changes Aug 8, 2018

View reviewed changes

richardlau approved these changes Aug 9, 2018

View reviewed changes

danbev closed this Aug 10, 2018

danbev deleted the crypto_checks branch August 10, 2018 05:29

rvagg mentioned this pull request Aug 13, 2018

Release proposal: v10.9.0 #22295

Merged

joyeecheung mentioned this pull request Nov 6, 2018

build: only try to run node when it's needed by the target #24115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add crypto check to build targets #22148

build: add crypto check to build targets #22148

danbev commented Aug 6, 2018

nodejs-github-bot commented Aug 6, 2018

danbev commented Aug 6, 2018

lpinca commented Aug 6, 2018

danbev commented Aug 7, 2018

danbev commented Aug 7, 2018

danbev commented Aug 8, 2018

mhdawson left a comment

danbev commented Aug 9, 2018

danbev commented Aug 10, 2018

danbev commented Aug 10, 2018

joyeecheung commented Nov 3, 2018

refack commented Nov 3, 2018

refack commented Nov 3, 2018

joyeecheung commented Nov 3, 2018 •

edited

build: add crypto check to build targets #22148

build: add crypto check to build targets #22148

Conversation

danbev commented Aug 6, 2018

Checklist

nodejs-github-bot commented Aug 6, 2018

danbev commented Aug 6, 2018

lpinca commented Aug 6, 2018

danbev commented Aug 7, 2018

danbev commented Aug 7, 2018

danbev commented Aug 8, 2018

mhdawson left a comment

Choose a reason for hiding this comment

danbev commented Aug 9, 2018

danbev commented Aug 10, 2018

danbev commented Aug 10, 2018

joyeecheung commented Nov 3, 2018

refack commented Nov 3, 2018

refack commented Nov 3, 2018

joyeecheung commented Nov 3, 2018 • edited

joyeecheung commented Nov 3, 2018 •

edited