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

lib: add crypto dependant modules cannotUseCache #24100

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Nov 5, 2018

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

danbev commented Nov 5, 2018

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

danbev commented Nov 5, 2018

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

refack commented Nov 5, 2018

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 refack added the crypto Issues and PRs related to the crypto subsystem. label Nov 5, 2018
@refack
Copy link
Contributor

refack commented Nov 5, 2018

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.

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 danbev changed the title build: guard crypto JS files with condition lib: add crypto dependant modules cannotUseCache Nov 6, 2018
@danbev
Copy link
Contributor Author

danbev commented Nov 6, 2018

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 6, 2018
@danbev
Copy link
Contributor Author

danbev commented Nov 8, 2018

Landed in 350bef6.

@danbev danbev closed this Nov 8, 2018
@danbev 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
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
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>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
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>
codebytere pushed a commit that referenced this pull request Nov 29, 2018
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
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 codebytere mentioned this pull request Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
Development

Successfully merging this pull request may close these issues.

6 participants