Check openssl version and add appropriate defines#9924
Check openssl version and add appropriate defines#9924cmcfarlen merged 3 commits intoapache:masterfrom
Conversation
JosiahWI
left a comment
JosiahWI
left a comment
There was a problem hiding this comment.
I like these changes, but there are some minor points that should be addressed. How is boringssl versioned? Is it possible that finding boringssl could still end up with OPENSSL_IS_OPENSSL3 getting set?
CMakeLists.txt
Outdated
| @@ -167,7 +167,14 @@ endif() | |||
| find_package(PCRE) | |||
|
|
|||
| include(FindOpenSSL) | |||
There was a problem hiding this comment.
Since you're cleaning up the OpenSSL checks, can we remove this direct inclusion of FindOpenSSL? find_package should find the right module to include for us.
CMakeLists.txt
Outdated
| check_openssl_is_boringssl(OPENSSL_IS_BORINGSSL "${OPENSSL_INCLUDE_DIR}") | ||
|
|
||
| if(OPENSSL_VERSION VERSION_GREATER_EQUAL "3.0.0") | ||
| set(OPENSSL_IS_OPENSSL3 ON) |
There was a problem hiding this comment.
Could we use TRUE for internal checks? I think it helps distinguish between options that can be toggled, and deterministic checks.
src/tscore/CMakeLists.txt
Outdated
| target_sources(tscore PUBLIC HKDF_boringssl.cc) | ||
| elseif(OPENSSL_IS_OPENSSL3) | ||
| target_sources(tscore PUBLIC HKDF_openssl3.cc) | ||
| else() | ||
| target_sources(tscore PUBLIC HKDF_openssl.cc) |
There was a problem hiding this comment.
Seeing a PUBLIC usage requirement for target sources is like seeing Santa Clause driving his sleigh in December without his coat on. Are we doing something fancy, or should these be PRIVATE?
There was a problem hiding this comment.
I couldn't figure out which was the default for add_library so just punted. I think we need to audit these usage requirements in general.
There was a problem hiding this comment.
We do. I've been fixing a few here and there, but most of it will probably have to be part of the general cleanup. Regardless, we should make sure that new ones are correct.
Part of the general problem there is that the cyclic dependencies result in usage requirements being weird.
|
|
||
| if(OPENSSL_VERSION VERSION_GREATER_EQUAL "3.0.0") | ||
| set(OPENSSL_IS_OPENSSL3 ON) | ||
| add_compile_definitions(OPENSSL_API_COMPAT=10002 OPENSSL_IS_OPENSSL3) |
There was a problem hiding this comment.
./plugins/certifier/CMakeLists.txt:add_definitions(-DOPENSSL_API_COMPAT=10002)
Interesting, this is duplicated in the certifier. That's not necessarily bad because it helps decouple, but I'm wondering whether the one in certifier should also be guarded by a conditional.
There was a problem hiding this comment.
This is how auto tools is setup when openssl3 is found. I'm not sure if its necessary.
There was a problem hiding this comment.
I will remove the definition in certifier plugin. I don't think it needs to be in both. If its a problem, perhaps there is something else going on.
borinssl defines |
3 participants
No description provided.