Added support for custom OpenSSL selection#5799
Added support for custom OpenSSL selection#5799toxicteddy00077 wants to merge 1 commit intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5799 +/- ##
==========================================
- Coverage 86.34% 84.78% -1.56%
==========================================
Files 60 60
Lines 18729 18729
==========================================
- Hits 16172 15880 -292
- Misses 2557 2849 +292 ☔ 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.
Thanks for the PR.
I don't think we can use the find_package approach with QuicTLS.
Our longer term goal is to stop depending on QuicTLS completely (once OpenSSL 3.5 is broadly available), so we could also add these new build config work only for openssl, not quictls.
CMakeLists.txt
Outdated
| add_library(OpenSSL INTERFACE) | ||
|
|
||
| include(FetchContent) | ||
| option(QUIC_USE_EXTERNAL_OPENSSL "Use external OpenSSL instead of building from submodules" OFF) |
There was a problem hiding this comment.
Please declare these parameters with the others at the top of the file, so they are easy to find.
| endif() | ||
| target_link_libraries(OpenSSLQuic INTERFACE OpenSSL::SSL OpenSSL::Crypto) | ||
| endif() | ||
| add_library(OpenSSLQuic::OpenSSLQuic ALIAS OpenSSLQuic) |
There was a problem hiding this comment.
Consider using a different name for the alias, and aliasing OpenSSLQuic::OpenSSLQuic to that name too.
I find it somewhat confusing as it is (unless this is a common Cmake pattern, I am not fluent in Cmake)
There was a problem hiding this comment.
Can't say im sure about the naming convention either but for the sake of brevity ive kept it as OpenSSL::MsQuic
|
|
||
| if(QUIC_USE_EXTERNAL_OPENSSL OR QUIC_OPENSSL_INCLUDE_DIR OR QUIC_OPENSSL_LIB_DIR) | ||
| add_library(OpenSSLQuic INTERFACE) | ||
| if(QUIC_OPENSSL_INCLUDE_DIR AND QUIC_OPENSSL_LIB_DIR) |
There was a problem hiding this comment.
If only one of the two is set, we will go through the else path.
We should have an error instead.
CMakeLists.txt
Outdated
| if(QUIC_TLS_LIB STREQUAL "openssl") | ||
| find_package(OpenSSL 3.5.0 REQUIRED) | ||
| else() | ||
| find_package(OpenSSL REQUIRED) |
There was a problem hiding this comment.
When the TLS provider is quictls, we need to link QuicTLS version of libssl statically.
It is a fork of OpenSSL with support for QUIC, OpenSSL prior to 3.5 doesn't expose the required APIs, so dynamically linking to the system libssl won't work.
There was a problem hiding this comment.
Alright ive added a condition for this
toxicteddy00077
force-pushed
the
add-openssl-version-support
branch
from
f64c75a to
12ead5d
Compare
2 participants
Closes #5614
Added support for choosing custom OpenSSL version with three prospective methods:
I mostly built on the suggestion given by the issue author