-
Notifications
You must be signed in to change notification settings - Fork 846
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
Implement build configuration header for SRT_ENABLE_AEAD_API_PREVIEW. #2696
Implement build configuration header for SRT_ENABLE_AEAD_API_PREVIEW. #2696
Conversation
This is a pattern that can be used to add build configuration options that effect the library ABI. Currently some of the them like ENABLE_AEAD_API_PREVIEW have to be set by the library user in order to get the right ABI/API for the library. As indicated by #2610 this is error prone and dangerous. If this PR is accepted, then going forward any ABI affecting configuration options used to build the SRT library can be specified here so that they are set correctly for any subsequent user of the library. |
I dont know what other configuration options are needed at this point. ENABLE_BONDING looks suspicious in srt.h. |
Looks like this would require raising the minimum CMake version to 3.0. |
Oh. Did I use something that is from CMake 3.0 or later? I thought it was only features that were already being used. For instance to generate the version.h file. |
Nope, my bad. I thought |
Why can't this be done the same way as for all other such options - by adding -D to the "definitions" in SRT in cmakefile? |
set(SRT_ENABLE_AEAD_API_PREVIEW ${ENABLE_AEAD_API_PREVIEW}) | ||
configure_file("srtcore/srt_config.h.in" "srt_config.h" @ONLY) | ||
list(APPEND HEADERS_srt "${CMAKE_CURRENT_BINARY_DIR}/srt_config.h") | ||
|
||
add_library(srt_virtual OBJECT ${SOURCES_srt} ${SOURCES_srt_extra} ${HEADERS_srt} ${SOURCES_haicrypt} ${SOURCES_common}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set(SRT_ENABLE_AEAD_API_PREVIEW ${ENABLE_AEAD_API_PREVIEW}) | |
configure_file("srtcore/srt_config.h.in" "srt_config.h" @ONLY) | |
list(APPEND HEADERS_srt "${CMAKE_CURRENT_BINARY_DIR}/srt_config.h") | |
add_library(srt_virtual OBJECT ${SOURCES_srt} ${SOURCES_srt_extra} ${HEADERS_srt} ${SOURCES_haicrypt} ${SOURCES_common}) | |
add_library(srt_virtual OBJECT ${SOURCES_srt} ${SOURCES_srt_extra} ${HEADERS_srt} ${SOURCES_haicrypt} ${SOURCES_common}) | |
set_if(SRT_ENABLE_AEAD_API_PREVIEW ENABLE_AEAD_API_PREVIEW) | |
target_compile_definitions(srt_virtual PRIVATE ENABLE_AEAD_API_PREVIEW=${SRT_ENABLE_AEAD_API_PREVIEW}) |
@@ -255,7 +255,7 @@ const SocketOption srt_options [] { | |||
{ "bindtodevice", 0, SRTO_BINDTODEVICE, SocketOption::PRE, SocketOption::STRING, nullptr}, | |||
#endif | |||
{ "retransmitalgo", 0, SRTO_RETRANSMITALGO, SocketOption::PRE, SocketOption::INT, nullptr } | |||
#ifdef ENABLE_AEAD_API_PREVIEW | |||
#if defined(SRT_ENABLE_AEAD_API_PREVIEW) && (SRT_ENABLE_AEAD_API_PREVIEW == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#if defined(SRT_ENABLE_AEAD_API_PREVIEW) && (SRT_ENABLE_AEAD_API_PREVIEW == 1) | |
#if SRT_ENABLE_AEAD_API_PREVIEW |
Because you need to know how the library was originally compiled. so this information should be stored in generated header files that reflect how the library was built. Otherwise you have no way of knowing if SRT_ENABLE_API_PREVIEW and other configurations that are used to effect the how the library was built and can in fact effect the ABI of the library such that you have to set them correctly when using the library or risk the code using it break. This information needs to be in generated header files. and nowhere else. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2696 +/- ##
==========================================
+ Coverage 66.73% 67.37% +0.64%
==========================================
Files 99 99
Lines 20149 20149
==========================================
+ Hits 13447 13576 +129
+ Misses 6702 6573 -129 ☔ View full report in Codecov by Sentry. |
Implement solution for #2610 FR request.