Link OpenSSL statically to static driver lib#450
Draft
wprzytula wants to merge 4 commits into
Draft
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts the build and pkg-config metadata so the Rust wrapper’s static library can link against OpenSSL statically, and consumers can obtain the necessary link flags when using the static driver artifact.
Changes:
- Pass OpenSSL-related environment variables into the
cargo_build()invocations (includingOPENSSL_STATIC=1for the staticlib build) and serialize shared/static builds to avoid cargo cache thrash. - Extend the CMake
cargo_build()helper to accept additional environment variables (ENV ...). - Update the static pkg-config file to use
Requires.private: openssland append platform “native” system libs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
scylla-rust-wrapper/scylla-cpp-driver_static.pc.in |
Adjusts pkg-config dependency handling for OpenSSL and adds a placeholder for native/system libs. |
scylla-rust-wrapper/CMakeLists.txt |
Passes OpenSSL env vars into cargo builds; adds native/system libs to the generated static .pc; serializes shared+static cargo builds. |
cmake/Dependencies.cmake |
Derives an OPENSSL_LIB_DIR to pass through to openssl-sys. |
cmake/CMakeCargo.cmake |
Adds support for passing arbitrary environment variables into the cargo invocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| endif() | ||
|
|
||
| # Derive OPENSSL_LIB_DIR for passing to openssl-sys via environment. | ||
| get_filename_component(OPENSSL_LIB_DIR "${OPENSSL_SSL_LIBRARY}" DIRECTORY) |
Comment on lines
+208
to
+212
| set(native_static_libs "-lm -lpthread -ldl -framework Security -framework CoreFoundation") | ||
| elseif(WIN32) | ||
| set(native_static_libs "-lws2_32 -lbcrypt -luserenv -lntdll") | ||
| else() | ||
| set(native_static_libs "-lm -lpthread -ldl -lrt") |
| Version: @version@ | ||
| Requires: openssl | ||
| Libs: -L${libdir} -lscylla-cpp-driver_static | ||
| Requires.private: openssl |
wprzytula
force-pushed
the
openssl-static-link
branch
from
9eaa602 to
14158dc
Compare
Allow callers to pass additional environment variables to the cargo process. This will be used to pass OPENSSL_STATIC, OPENSSL_LIB_DIR, and OPENSSL_INCLUDE_DIR for controlling how openssl-sys locates and links OpenSSL.
Derive OPENSSL_LIB_DIR from the library path found by CMake's find_package(OpenSSL) and pass both OPENSSL_LIB_DIR and OPENSSL_INCLUDE_DIR to cargo_build() via the ENV parameter. This ensures openssl-sys finds the same OpenSSL installation that CMake found, rather than searching independently. No functional change — openssl-sys already dynamically links OpenSSL by default.
Change Requires to Requires.private in the static library's .pc file. Requires.private means pkg-config only exposes the dependency's flags when the consumer passes --static. Without --static, the dependency is ignored entirely. This is the correct semantics because: - OpenSSL is an internal implementation detail, not exposed in the public API headers. Consumers don't need OpenSSL's -I flags to compile against the driver. - Anyone linking the static .a should be using --static, which pulls in Requires.private deps. So static consumers still get -lssl. - If a consumer mistakenly uses the static .pc without --static (e.g. linking the .a directly), Requires would needlessly add OpenSSL flags that may conflict with their own OpenSSL usage. This matches the convention used by OpenSSL's own libssl.pc, which declares Requires.private: libcrypto rather than Requires:.
Pass OPENSSL_STATIC=1 to the staticlib cargo_build() invocation so openssl-sys links OpenSSL statically into libscylla-cpp-driver_static.a. This makes the static archive truly self-contained — users only need to link system libraries and OpenSSL's pkg-config dependencies. Since the cdylib and staticlib builds share CARGO_TARGET_DIR but pass different OPENSSL_STATIC values, they must not run concurrently — openssl-sys declares rerun-if-env-changed for that variable, and parallel invocations would thrash its build script cache. Serialize by making the staticlib target depend on the cdylib target when both are enabled. Append platform-specific system libraries to the static .pc file's Libs: line via @native_static_libs@. These are needed by the Rust runtime (pthreads, libdl, libm, etc.) and must be provided by the final linker when linking the static archive into an executable. Add libssl-dev to the list of build dependencies, since the static build needs static OpenSSL libraries, which are not provided by libssl1.1.
wprzytula
force-pushed
the
openssl-static-link
branch
from
14158dc to
23a7539
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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
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.
Pre-review checklist
Makefilein{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.Fixes:annotations to PR description.