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

mac: deps/openssl compiled, even with --shared-openssl #40958

Closed
minrk opened this issue Nov 24, 2021 · 2 comments
Closed

mac: deps/openssl compiled, even with --shared-openssl #40958

minrk opened this issue Nov 24, 2021 · 2 comments
Labels
build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency.

Comments

@minrk
Copy link

minrk commented Nov 24, 2021

Version

17.1.0

Platform

Darwin hostname 21.1.0 Darwin Kernel Version 21.1.0: Wed Oct 13 17:33:23 PDT 2021; root:xnu-8019.41.5~1/RELEASE_X86_64 x86_64

Subsystem

node.gyp

What steps will reproduce the bug?

build with

./configure \
    --ninja \
    --prefix=${PREFIX} \
    --without-node-snapshot \
    --shared \
    --shared-libuv \
    --shared-openssl \
    --shared-zlib \
    --with-intl=system-icu 

though I believe only --shared-openssl is relevant.

How often does it reproduce? Is there a required condition?

on anny mac build with --shared-openssl, as far as I can tell.

What is the expected behavior?

deps/openssl is not compiled when --shared-openssl is given.

What do you see instead?

deps/openssl is compiled (and fails!). This should just be wasted build time, compiling openssl that isn't going to be used, but the fact that it happens to fail means it's blocking progress. I don't know if the build failure is relevant, but the fact is it shouldn't be built at all (and isn't on linux with the same input)

Additional information

I believe it is caused by test_crypto_engine (added in #40481) depending on deps/openssl/openssl.gyp:openssl when OS=="mac" and node_use_openssl=="true".

The simplest patch is to change the condition to node_use_openssl=="true" and node_shared_openssl=="false" because the content appears to assume that deps/openssl is used, but I'm not sure if the test should still be built with shared openssl. If so, it should at least properly link/include shared openssl, instead of deps/openssl. That, I don't know how to patch. The linux builds succeed, but the fact that it unconditionally includes from deps/openssl/include suggests it might do something wrong, depending on the version of the shared openssl.

This is seen updating nodejs on conda-forge:

@targos targos added build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency. labels Nov 25, 2021
@minrk
Copy link
Author

minrk commented Nov 25, 2021

FWIW, this patch did fix the mac builds. I've submitted it as #40965 but feel free to close if it should be the more complicated fix to ensure the tests are built with the shared openssl, which I don't know how to do.

@Bo98
Copy link

Bo98 commented Dec 1, 2021

This has now regressed in 16.13.1 as well.

mhdawson added a commit to mhdawson/io.js that referenced this issue Feb 4, 2022
Fixes: nodejs#41633
Fixes: nodejs#40958

- move test-crypto-engine so that dummy engine
  is only built if tests are run

Signed-off-by: Michael Dawson <[email protected]>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
Fixes: nodejs#41633
Fixes: nodejs#40958

- move test-crypto-engine so that dummy engine
  is only built if tests are run

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#41830
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
Fixes: nodejs#41633
Fixes: nodejs#40958

- move test-crypto-engine so that dummy engine
  is only built if tests are run

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#41830
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022
Fixes: nodejs#41633
Fixes: nodejs#40958

- move test-crypto-engine so that dummy engine
  is only built if tests are run

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#41830
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants