8000 build: add crypto check to build targets by danbev · Pull Request #22148 · nodejs/node · GitHub
  • [go: up one dir, main page]
    Skip to content

    build: add crypto check to build targets #22148

    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.

    Sign up for GitHub

    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

    Jump to bottom
    Closed
    danbev wants to merge 4 commits into from
    Closed

    build: add crypto check to build targets #22148

    danbev 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

    danbev added 3 commits August 6, 2018 10:08
    @danbev
    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.
    @danbev
    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.
    @danbev
    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
    Copy link
    Collaborator
    nodejs-github-bot commented Aug 6, 2018

    @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
    8000

    @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

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

    @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.

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

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

    @danbev
    Copy link
    Contributor Author
    danbev commented Aug 8, 2018

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

    mhdawson
    mhdawson approved these changes Aug 8, 2018
    View reviewed changes
    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
    @danbev
    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
    @danbev
    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
    @danbev
    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
    @danbev @targos
    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
    @danbev @targos
    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
    @danbev @targos
    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 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
    Copy link
    Member
    joyeecheung commented Nov 3, 2018

    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 •
    edited
    Loading

    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 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
    @danbev @rvagg
    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
    @danbev @rvagg
    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
    @danbev @rvagg
    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>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @jasnell jasnell jasnell approved these changes

    @lpinca lpinca lpinca approved these changes

    @cjihrig cjihrig cjihrig approved these changes

    @richardlau richardlau richardlau approved these changes

    @mhdawson mhdawson mhdawson approved these changes

    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

    @danbev @nodejs-github-bot @lpinca @joyeecheung @refack @jasnell @cjihrig @richardlau @mhdawson
    0