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

lib: add crypto dependant modules cannotUseCache by danbev · Pull Request #24100 · nodejs/node · GitHub

/ node Public

lib: add crypto dependant modules cannotUseCache - #24100

Closed
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:build_move_crypto_files
ConversationCommits (1)ChecksFiles changed
Closed

lib: add crypto dependant modules cannotUseCache#24100
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:build_move_crypto_files

Conversation

danbev commented Nov 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

This commit adds JavaScript modules that depend on crypto to the cannotUseCache array. This is to avoid having them compiled when node has been configured --without-ssl which currently fails.

With this change the complete testsuite passes again when configured --without-ssl.

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

nodejs-github-bot commented Nov 5, 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/1496/pipeline

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

danbev commented Nov 5, 2018

Copy link
Copy Markdown
Contributor Author

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

danbev force-pushed the build_move_crypto_files branch from dd15296 to 3fc4f3d Compare November 5, 2018 10:28

joyeecheung commented Nov 5, 2018

Copy link
Copy Markdown
Member

This would make the errors different now right? Before it's ERR_NO_CRYPTO, now it's the module not found error?

Also, if just to work around the cache generator - you can just push to the cannotUseCache array in lib/internal/bootstrap/cache.js when process.versions.openssl is falsy, similar to how Intl, trace events and inspector gets skipped - although come to think of it, don't we have no crypto jobs in the CI? @refack

danbev commented Nov 5, 2018

Copy link
Copy Markdown
Contributor Author

This would make the errors different now right?

I'm not sure, if there are no existing test for those error messages then that might be an issue. All test pass locally for me using --without-ssl (if I also include #24096 but that is a different failure than this one).

Also, if just to work around the cache generator

I was thinking that we exclude the crypto object files when configured without-ssl and thought it made sense to also exclude these, but my main goal here is to get the testsuite to pass with --without-ssl so I'll take a look cannotUseCache. Thanks

refack commented Nov 5, 2018

Copy link
Copy Markdown
Contributor

although come to think of it, don't we have no crypto jobs in the CI? @refack

No. We only have --without-intl and --shared-openssl, but we can do have CONFIG_FLAGS so lets see:
https://ci.nodejs.org/job/node-test-commit/22899/parameters/

refack added the crypto Issues and PRs related to the crypto subsystem. label Nov 5, 2018

refack commented Nov 5, 2018

Copy link
Copy Markdown
Contributor

This commit moves the JavaScript source files that have a crypto dependency to be guarded by node_use_openssl

I'm trying to figure out how to make this more maintainable 🤔 ... Maybe have a blacklist instead of a whitelist (like V8 does for no-intl -

node/deps/v8/gypfiles/v8.gyp

Lines 1892 to 1893 in 7e1b178

}, { # v8_enable_i18n_support==0
'sources!': [
)

Or even better (could be in a later PR) parsed the sources and dynamically filter those out in js2c.py.

lib: add crypto dependant modules cannotUseCache
aaac542 
This commit adds JavaScript modules that depend on crypto to
the cannotUseCache array. This is to avoid having them compiled when
node has been configured --without-ssl which currently fails.
danbev force-pushed the build_move_crypto_files branch from 3fc4f3d to aaac542 Compare November 6, 2018 06:51
danbev changed the title build: guard crypto JS files with condition lib: add crypto dependant modules cannotUseCache Nov 6, 2018

danbev commented Nov 6, 2018

Copy link
Copy Markdown
Contributor Author

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

refack added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 6, 2018

danbev commented Nov 8, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 350bef6.

danbev closed this Nov 8, 2018
danbev deleted the build_move_crypto_files branch November 8, 2018 04:40
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 8, 2018
lib: add crypto dependant modules cannotUseCache
350bef6 
This commit adds JavaScript modules that depend on crypto to
the cannotUseCache array. This is to avoid having them compiled when
node has been configured --without-ssl which currently fails.

PR-URL: nodejs#24100
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
lib: add crypto dependant modules cannotUseCache
1225a0a 
This commit adds JavaScript modules that depend on crypto to
the cannotUseCache array. This is to avoid having them compiled when
node has been configured --without-ssl which currently fails.

PR-URL: #24100
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR mentioned this pull request Nov 14, 2018
Merged
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
lib: add crypto dependant modules cannotUseCache
e6ae541 
This commit adds JavaScript modules that depend on crypto to
the cannotUseCache array. This is to avoid having them compiled when
node has been configured --without-ssl which currently fails.

PR-URL: nodejs#24100
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
This was referenced Nov 15, 2018
Closed
Closed
Closed
Closed
Closed
beevelop mentioned this pull request Nov 16, 2018
Closed
codebytere pushed a commit that referenced this pull request Nov 29, 2018
lib: add crypto dependant modules cannotUseCache
4864cb0 
This commit adds JavaScript modules that depend on crypto to
the cannotUseCache array. This is to avoid having them compiled when
node has been configured --without-ssl which currently fails.

PR-URL: #24100
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request Dec 3, 2018
lib: add crypto dependant modules cannotUseCache
4035ca1 
This commit adds JavaScript modules that depend on crypto to
the cannotUseCache array. This is to avoid having them compiled when
node has been configured --without-ssl which currently fails.

PR-URL: #24100
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
codebytere mentioned this pull request Dec 3, 2018
Merged
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

joyeecheung joyeecheung approved these changes
richardlau richardlau approved these changes
+1 more reviewer
refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. build Issues and PRs related to build files or the CI. crypto Issues and PRs related to the crypto subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL