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

build: add crypto check to build targets by danbev · Pull Request #22148 · nodejs/node · GitHub

/ node Public

build: add crypto check to build targets - #22148

Closed
danbev wants to merge 4 commits into
nodejs:masterfrom
danbev:crypto_checks
ConversationCommits (4)ChecksFiles changed
Closed

build: add crypto check to build targets#22148
danbev wants to merge 4 commits into
nodejs:masterfrom
danbev:crypto_checks

Conversation

danbev commented Aug 6, 2018

Copy link
Copy Markdown
Contributor

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

danbev added 3 commits August 6, 2018 10:08
build: add crypto check to build targets
2a10c51 
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.
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 commented Aug 6, 2018

Copy link
Copy Markdown
Collaborator

@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

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

danbev commented Aug 6, 2018

Copy link
Copy Markdown
Contributor Author

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

lpinca commented Aug 6, 2018

Copy link
Copy Markdown
Member

CI is failing.

danbev commented Aug 7, 2018

Copy link
Copy Markdown
Contributor Author

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

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.

danbev commented Aug 7, 2018

Copy link
Copy Markdown
Contributor Author

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

danbev commented Aug 8, 2018

Copy link
Copy Markdown
Contributor Author

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

mhdawson approved these changes Aug 8, 2018
View reviewed changes

mhdawson 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

LGTM if CI is green

danbev commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author
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 commented Aug 10, 2018

Copy link
Copy Markdown
Contributor Author

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

danbev commented Aug 10, 2018

Copy link
Copy Markdown
Contributor Author

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

danbev closed this Aug 10, 2018
danbev deleted the crypto_checks branch August 10, 2018 05:29
danbev added a commit that referenced this pull request Aug 10, 2018
build: add crypto check to build targets
77da6d9 
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
test: move require of http2 to after crypto check
94840fd 
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
test: move require of https to after crypto check
346f2a7 
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
build: add crypto check to build targets
d338d41 
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
test: move require of http2 to after crypto check
fcc150d 
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
test: move require of https to after crypto check
3baeb82 
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>
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

joyeecheung commented Nov 3, 2018

Copy link
Copy Markdown
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 commented Nov 3, 2018

Copy link
Copy Markdown
Contributor

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 commented Nov 3, 2018

Copy link
Copy Markdown
Contributor

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 commented Nov 3, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

joyeecheung mentioned this pull request Nov 6, 2018
Closed
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
build: add crypto check to build targets
c42ff4e 
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: nodejs/node#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>
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
test: move require of http2 to after crypto check
a6ab19a 
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: nodejs/node#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>
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
test: move require of https to after crypto check
b1cbbbc 
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: nodejs/node#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>
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

jasnell jasnell approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
richardlau richardlau approved these changes
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL