Skip to content
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

build: add crypto check to build targets #22148

Closed
wants to merge 4 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Aug 6, 2018

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 nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 6, 2018
@danbev
Copy link
Contributor Author

danbev commented Aug 6, 2018

@lpinca
Copy link
Member

lpinca commented Aug 6, 2018

CI is failing.

@danbev
Copy link
Contributor Author

danbev commented Aug 7, 2018

@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
Copy link
Contributor Author

danbev commented Aug 7, 2018

@danbev
Copy link
Contributor Author

danbev commented Aug 8, 2018

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

@danbev
Copy link
Contributor Author

danbev commented Aug 9, 2018

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
Copy link
Contributor Author

danbev commented Aug 10, 2018

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

@danbev
Copy link
Contributor Author

danbev commented Aug 10, 2018

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

@danbev danbev closed this Aug 10, 2018
@danbev danbev deleted the crypto_checks branch August 10, 2018 05:29
danbev added a commit that referenced this pull request Aug 10, 2018
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>
danbev added a commit that referenced this pull request Aug 10, 2018
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>
danbev added a commit that referenced this pull request Aug 10, 2018
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>
targos pushed a commit that referenced this pull request Aug 11, 2018
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>
targos pushed a commit that referenced this pull request Aug 11, 2018
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>
targos pushed a commit that referenced this pull request Aug 11, 2018
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
Copy link
Member

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
Copy link
Contributor

refack commented Nov 3, 2018

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
Copy link
Contributor

refack commented Nov 3, 2018

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
Copy link
Member

joyeecheung commented Nov 3, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants