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

Prioritize ${OPENSSL_ROOT_DIR} over system paths #538

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

diku89
Copy link
Contributor

@diku89 diku89 commented Feb 28, 2023

This commit reorders the locations where CMake will look for openssl, starting with ${OPENSSL_ROOT_DIR} and then proceeding to system install locations.

This allows the user to provide a different Openssl version with -DOPENSSL_ROOT_DIR=<path/to/openssl>

This commit reorders the locations where CMake will look
for openssl, starting with ${OPENSSL_ROOT_DIR} and then
proceeding to system install locations.

This allows the user to provide a different Openssl version
with -DOPENSSL_ROOT_DIR=<path/to/openssl>
@absurdfarce
Copy link
Collaborator

Greetings @diku89 and thanks for the contribution! Apologies for taking so long to get back to you.

Have you signed the Contributor License Agreement for contributions to DataStax open source projects? If not you can find it at https://cla.datastax.com/. Thanks!

@diku89
Copy link
Contributor Author

diku89 commented Mar 2, 2023

Hello. Thanks for the heads up. I just signed it.

@absurdfarce
Copy link
Collaborator

Hey @diku89 , just so I have this right in my own head... the goal here is to make sure that an OpenSSL install supplied via the build param is used in preference over an existing install, correct? That makes sense to me, so I think we're largely good if that's the case. I need to test a few things locally but this seems pretty sane to me.

One thing: the syntax ENV OPENSSL_ROOT_DIR on line 87 seems to refer to specifying an OpenSSL install via an environment variable. If we're preferring an OpenSSL install specified via a build param to a hard-coded value seems like we should prefer one specified via an environment variable as well. So with that in mind it seems reasonable to make the hard-coded value(s) the last thing in this list.

@absurdfarce
Copy link
Collaborator

After some testing I can confirm that this is definitely not behaving as expected. Here's what we have from the current behaviour in base, when passing props via the command-line and via env vars:

  • Base using existing config:
    • -- Found OpenSSL: /usr/local/opt/openssl/lib/libssl.so (found version "1.1.1q")
  • LIBUV_ROOT_DIR=/work/local/libuv-1.44.2/ cmake -DOPENSSL_ROOT_DIR=/work/local/openssl ..
    • -- Found OpenSSL: /usr/local/opt/openssl/lib/libssl.so (found version "1.1.1q")
  • LIBUV_ROOT_DIR=/work/local/libuv-1.44.2/ OPENSSL_ROOT_DIR=/work/local/openssl cmake ..
    • -- Found OpenSSL: /usr/local/opt/openssl/lib/libssl.so (found version "1.1.1q")

Just moving the default /usr/local/opt/openssl config down seems to leave us in a much better spot:

  • Base using existing config:
    • -- Found OpenSSL: /usr/local/opt/openssl/lib/libssl.so (found version "1.1.1q")
  • LIBUV_ROOT_DIR=/work/local/libuv-1.44.2/ cmake -DOPENSSL_ROOT_DIR=/work/local/openssl ..
    • -- Found OpenSSL: /work/local/openssl/lib/libssl.so (found version "1.1.1q")
  • LIBUV_ROOT_DIR=/work/local/libuv-1.44.2/ OPENSSL_ROOT_DIR=/work/local/openssl cmake ..
    • -- Found OpenSSL: /work/local/openssl/lib/libssl.so (found version "1.1.1q")

I'm going to merge this PR as it stands and tweak it slightly after the fact so that both the CLI prop and the env var can override the default.

Huge thanks to @diku89 for the PR... and for your patience!

@absurdfarce absurdfarce merged commit 6ebb472 into datastax:master Sep 15, 2023
@absurdfarce
Copy link
Collaborator

Quick note for the record: follow up commit mentioned above is here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants