Improve support for custom OpenSSL selection#5848
Improve support for custom OpenSSL selection#5848masa-koz wants to merge 2 commits intomicrosoft:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5848 +/- ##
==========================================
- Coverage 86.30% 85.01% -1.30%
==========================================
Files 60 60
Lines 18732 18732
==========================================
- Hits 16167 15925 -242
- Misses 2565 2807 +242 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
guhetier
left a comment
guhetier
left a comment
There was a problem hiding this comment.
As a general comment, this start to be a lot of options to handle OpenSSL.
I would be interesting to find a way to keep the complexity from getting out of hands, although I don't have a solution in mind, and it doesn't have to be in this PR.
| option(QUIC_USE_EXTERNAL_OPENSSL "Use external OpenSSL instead of building from submodules" OFF) | ||
| set(QUIC_OPENSSL_INCLUDE_DIR "" CACHE PATH "Path to OpenSSL include directory") | ||
| set(QUIC_OPENSSL_LIB_DIR "" CACHE PATH "Path to OpenSSL library directory") | ||
| set(QUIC_OPENSSL_ROOT_DIR "" CACHE PATH "Path to OpenSSL Root directory") |
There was a problem hiding this comment.
| set(QUIC_OPENSSL_ROOT_DIR "" CACHE PATH "Path to OpenSSL Root directory") | |
| set(QUIC_OPENSSL_ROOT_DIR "" CACHE PATH "Path to OpenSSL root directory") |
| message(FATAL_ERROR "libcrypto not found in ${QUIC_OPENSSL_LIB_DIR}") | ||
| endif() | ||
| find_library(LIB_SSL NAMES ssl libssl PATHS ${QUIC_OPENSSL_LIB_DIR} NO_DEFAULT_PATH) | ||
| if(BUILD_SHARED_LIBS) |
There was a problem hiding this comment.
nit: To simplify this a bit, I'd consider grouping the find_library and checking both at once:
if (BUILD_SHARED_LIBS)
find...
find...
else
find...
find...
endif
if(NOT LIB_SSL OR NOT LIB_CRYPTO) ...
| target_link_libraries(OpenSSLQuic INTERFACE OpenSSL::SSL OpenSSL::Crypto) | ||
| # OpenSSL::SSL and OpenSSL::Crypto imported targets include their dependencies such as zstd. | ||
| # However, these dependencies will be linked in src/platform/CMakeLists.txt, | ||
| # so link only the main OpenSSL targets here to avoid duplication. |
There was a problem hiding this comment.
I'm not sure I understand why the duplication is a problem, doesn't CMake handle it?
| } else { | ||
| if let Ok(openssl_include_dir) = std::env::var("OPENSSL_INCLUDE_DIR") { | ||
| config.define("QUIC_OPENSSL_INCLUDE_DIR", openssl_include_dir); | ||
| } | ||
| if let Ok(openssl_lib_dir) = std::env::var("OPENSSL_LIB_DIR") { | ||
| config.define("QUIC_OPENSSL_LIB_DIR", openssl_lib_dir); | ||
| } |
There was a problem hiding this comment.
nit: Either else if, or enforce the two are present (since CMake will need both or none)
2 participants
Description
This pull request introduces improved support for building with an external OpenSSL installation, primarily by adding a new
openssl_externalfeature and enhancing the build system to handle more flexible OpenSSL configuration. The changes affect CMake configuration, Rust feature flags, and build scripts to better support both static and shared linking, and to allow specifying OpenSSL paths via environment variables.Build system and CMake improvements:
QUIC_OPENSSL_ROOT_DIRto allow specifying the OpenSSL root directory, and updated OpenSSL selection logic to support this variable for both static and shared builds. [1] [2]BUILD_SHARED_LIBSflag, and improved handling of imported OpenSSL targets to avoid duplicate dependency linking.Rust feature and build script updates:
openssl_externalfor building with an external OpenSSL, and updated the build script (scripts/build.rs) to configure CMake accordingly by passing the appropriate variables from environment variables if set. [1] [2]libon Windows andartifactselsewhere, improving compatibility with different platforms.Test and platform compatibility:
src/rs/config.rsto recognize the newopenssl_externalfeature when determining which TLS provider is active on Windows.Testing
No
Documentation
TBD