test_crypto_engine: depends on non-shared openssl#40965
Closed
minrk wants to merge 1 commit intonodejs:masterfrom
Closed
test_crypto_engine: depends on non-shared openssl#40965minrk wants to merge 1 commit intonodejs:masterfrom
minrk wants to merge 1 commit intonodejs:masterfrom
Conversation
nodejs-github-bot
added
build
minrk
force-pushed
the
test-noshared-openssl
branch
2 times, most recently
from
dc21bf7 to
7ffe941
Compare
so only build it when using non-shared openssl, otherwise (mac at least) will build deps/openssl even when using shared openssl. Fixes: https://github.com/nodejs/node/40958
minrk
force-pushed
the
test-noshared-openssl
branch
from
7ffe941 to
65119a8
Compare
Collaborator
RaisinTen
approved these changes
Nov 27, 2021
RaisinTen
requested changes
Nov 27, 2021
| # TODO(RaisinTen): Enable this to build on other platforms as well. | ||
| ['(OS=="mac" or (OS=="linux" and target_arch=="x64")) and \ | ||
| node_use_openssl=="true"', { | ||
| node_use_openssl=="true" and node_shared_openssl=="false"', { |
Member
There was a problem hiding this comment.
Tests are failing. I think you need to add this conditional:
if (process.config.variables.node_shared_openssl === false)around
node/test/parallel/test-crypto-engine.js
Line 31 in eadfbae
Member
|
closing since a fix for this has already landed in #41830 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test_crypto_engine is written such that it assumes node_shared_openssl is false (
deps/openssl/includeon linux,deps/openssl/openssl.gyp:opensslon mac), so update the condition to only when those are valid expectations. Without this,deps/opensslis built on mac even when building with--shared-openssl(#40958).closes #40958
It's possible the right fix is to update the dependencies to point to shared openssl when true. I don't know how to do that, though, so feel free to close this and someone else can pursue that resolution.
This patch is confirmed to fix the spurious build part of the issue in conda-forge/nodejs-feedstock#221