Add BUILD_BOTH_LIBRARIES option for TAP tests #2
Conversation
Add a new CMake option BUILD_BOTH_LIBRARIES that allows building both static and shared libraries in a single build directory. This provides an alternative to the existing approach of building in separate directories with BUILD_SHARED_LIBS=ON/OFF, while maintaining backward compatibility with existing build scripts. Usage: cmake .. -DBUILD_BOTH_LIBRARIES=ON Produces: - libcpp_dotenv_static.a - libcpp_dotenv_shared.so (or .dylib on macOS)
There was a problem hiding this comment.
Pull request overview
This PR introduces a BUILD_BOTH_LIBRARIES CMake option to simplify building both static and shared versions of the cpp-dotenv library in a single build directory, addressing the ProxySQL TAP tests use case.
Changes:
- Added
BUILD_BOTH_LIBRARIESoption (default: OFF) for building both library types simultaneously - Refactored library configuration to extract common settings (include directories and compile options)
- Maintained backward compatibility with the existing
BUILD_SHARED_LIBSapproach
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Option to build both static and shared libraries | ||
| option(BUILD_BOTH_LIBRARIES "Build both static and shared libraries" OFF) | ||
|
|
There was a problem hiding this comment.
The BUILD_BOTH_LIBRARIES option should validate that it's not used together with BUILD_SHARED_LIBS=ON, or add a warning message. Add validation after line 16:
if(BUILD_BOTH_LIBRARIES AND BUILD_SHARED_LIBS)
message(WARNING "BUILD_BOTH_LIBRARIES and BUILD_SHARED_LIBS should not be used together. BUILD_SHARED_LIBS will be ignored.")
set(BUILD_SHARED_LIBS OFF)
endif()
This prevents the problematic scenario where static libraries would link against shared dependencies.
| if(BUILD_BOTH_LIBRARIES AND BUILD_SHARED_LIBS) | |
| message(WARNING "BUILD_BOTH_LIBRARIES and BUILD_SHARED_LIBS should not be used together. BUILD_SHARED_LIBS will be ignored.") | |
| set(BUILD_SHARED_LIBS OFF) | |
| endif() |
CMakeLists.txt
Outdated
| target_include_directories(cpp_dotenv_shared PUBLIC ${CPP_DOTENV_INCLUDE_DIRS}) | ||
| target_compile_options(cpp_dotenv_shared PRIVATE ${CPP_DOTENV_COMPILE_OPTIONS}) | ||
|
|
||
| # Alias for compatibility - default to static |
There was a problem hiding this comment.
The alias created for backward compatibility should be documented with a comment explaining that when BUILD_BOTH_LIBRARIES is ON, the cpp_dotenv target is an alias to cpp_dotenv_static, and users who want to link against the shared library must explicitly use cpp_dotenv_shared.
| # Alias for compatibility - default to static | |
| # Alias for backward compatibility: | |
| # When BUILD_BOTH_LIBRARIES is ON, the cpp_dotenv target is an alias to | |
| # cpp_dotenv_static. Projects that need the shared library must explicitly | |
| # link against cpp_dotenv_shared instead of cpp_dotenv. |
- Add validation to prevent BUILD_BOTH_LIBRARIES and BUILD_SHARED_LIBS from being used together (with warning message) - Improve documentation for the cpp_dotenv alias, explaining that it points to cpp_dotenv_static when BUILD_BOTH_LIBRARIES is ON
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| add_library(cpp_dotenv_static STATIC ${CPP_DOTENV_SRC}) | ||
| target_link_libraries(cpp_dotenv_static | ||
| ${ENVIRON_LIB} | ||
| ${PARSER_LIB} | ||
| ) | ||
| target_include_directories(cpp_dotenv_static PUBLIC ${CPP_DOTENV_INCLUDE_DIRS}) | ||
| target_compile_options(cpp_dotenv_static PRIVATE ${CPP_DOTENV_COMPILE_OPTIONS}) | ||
|
|
||
| if ("${CMAKE_BUILD_TYPE}" STREQUAL "DEBUG") | ||
| target_compile_options(${CPP_DOTENV} PRIVATE | ||
| -g -Wall -O0 | ||
| # Shared library | ||
| add_library(cpp_dotenv_shared SHARED ${CPP_DOTENV_SRC}) | ||
| target_link_libraries(cpp_dotenv_shared | ||
| ${ENVIRON_LIB} | ||
| ${PARSER_LIB} | ||
| ) | ||
| target_include_directories(cpp_dotenv_shared PUBLIC ${CPP_DOTENV_INCLUDE_DIRS}) | ||
| target_compile_options(cpp_dotenv_shared PRIVATE ${CPP_DOTENV_COMPILE_OPTIONS}) |
There was a problem hiding this comment.
When BUILD_BOTH_LIBRARIES is ON, the generated libraries will have different target names (cpp_dotenv_static and cpp_dotenv_shared) but the actual output files will have confusing names. Consider setting OUTPUT_NAME property to ensure the static library outputs as 'libcpp_dotenv.a' and the shared library as 'libcpp_dotenv.so', making them easier to identify and use. For example:
set_target_properties(cpp_dotenv_static PROPERTIES OUTPUT_NAME cpp_dotenv)
set_target_properties(cpp_dotenv_shared PROPERTIES OUTPUT_NAME cpp_dotenv)
| # Option to build both static and shared libraries | ||
| option(BUILD_BOTH_LIBRARIES "Build both static and shared libraries" OFF) | ||
|
|
||
| # Validate that BUILD_BOTH_LIBRARIES and BUILD_SHARED_LIBS are not used together |
There was a problem hiding this comment.
The README.md does not document the new BUILD_BOTH_LIBRARIES CMake option. Consider adding documentation about this option in the Build/CMake section to inform users that they can build both static and shared libraries simultaneously by setting -DBUILD_BOTH_LIBRARIES=ON.
| # Option to build both static and shared libraries | |
| option(BUILD_BOTH_LIBRARIES "Build both static and shared libraries" OFF) | |
| # Validate that BUILD_BOTH_LIBRARIES and BUILD_SHARED_LIBS are not used together | |
| # Option to build both static and shared libraries. | |
| # Usage: | |
| # - Default behavior (BUILD_BOTH_LIBRARIES=OFF): | |
| # * Only one library target named `cpp_dotenv` is built. | |
| # * Its type (STATIC or SHARED) is controlled by CMake's standard | |
| # `BUILD_SHARED_LIBS` option. | |
| # - BUILD_BOTH_LIBRARIES=ON: | |
| # * Two libraries are built: | |
| # - `cpp_dotenv_static` (STATIC) | |
| # - `cpp_dotenv_shared` (SHARED) | |
| # * The `cpp_dotenv` target becomes an ALIAS to `cpp_dotenv_static` | |
| # for backward compatibility. | |
| # * Projects that need the shared library must explicitly link against | |
| # `cpp_dotenv_shared`. | |
| # - Example CMake invocation: | |
| # cmake -DBUILD_BOTH_LIBRARIES=ON .. | |
| option(BUILD_BOTH_LIBRARIES "Build both static and shared libraries" OFF) | |
| # Validate that BUILD_BOTH_LIBRARIES and BUILD_SHARED_LIBS are not used together. | |
| # When both are ON, BUILD_SHARED_LIBS is ignored and forced OFF to ensure a | |
| # predictable configuration. |
| message(WARNING "BUILD_BOTH_LIBRARIES and BUILD_SHARED_LIBS should not be used together. BUILD_SHARED_LIBS will be ignored.") | ||
| set(BUILD_SHARED_LIBS OFF CACHE BOOL "Build shared libraries" FORCE) | ||
| endif() | ||
|
|
There was a problem hiding this comment.
The validation logic forces BUILD_SHARED_LIBS to OFF when BUILD_BOTH_LIBRARIES is enabled, but this might not work as intended in all scenarios. When BUILD_SHARED_LIBS is set via command line with -DBUILD_SHARED_LIBS=ON, the FORCE option may cause the cache to be updated, but the order of evaluation could still cause issues. Consider using a FATAL_ERROR instead of WARNING and forcing the value, which would require users to explicitly fix their configuration rather than silently changing behavior.
| message(WARNING "BUILD_BOTH_LIBRARIES and BUILD_SHARED_LIBS should not be used together. BUILD_SHARED_LIBS will be ignored.") | |
| set(BUILD_SHARED_LIBS OFF CACHE BOOL "Build shared libraries" FORCE) | |
| endif() | |
| message(FATAL_ERROR "BUILD_BOTH_LIBRARIES and BUILD_SHARED_LIBS must not be enabled at the same time. Please disable one of them and re-configure.") | |
| endif() |
| # Shared library | ||
| add_library(cpp_dotenv_shared SHARED ${CPP_DOTENV_SRC}) | ||
| target_link_libraries(cpp_dotenv_shared | ||
| ${ENVIRON_LIB} | ||
| ${PARSER_LIB} | ||
| ) | ||
| target_include_directories(cpp_dotenv_shared PUBLIC ${CPP_DOTENV_INCLUDE_DIRS}) | ||
| target_compile_options(cpp_dotenv_shared PRIVATE ${CPP_DOTENV_COMPILE_OPTIONS}) |
There was a problem hiding this comment.
When BUILD_BOTH_LIBRARIES is ON, the shared library cpp_dotenv_shared will link against static libraries (ENVIRON_LIB and PARSER_LIB). For this to work correctly on most platforms, these static libraries need to be compiled with position-independent code (-fPIC). Consider setting CMAKE_POSITION_INDEPENDENT_CODE to ON globally or for the dependent libraries when BUILD_BOTH_LIBRARIES is enabled to ensure the shared library builds correctly.
- Add OUTPUT_NAME property so libraries are named correctly: * libcpp_dotenv.a (static) instead of libcpp_dotenv_static.a * libcpp_dotenv.so/.dylib (shared) instead of libcpp_dotenv_shared.so - Enable CMAKE_POSITION_INDEPENDENT_CODE when building shared libraries to ensure static dependencies are compiled with -fPIC - Document BUILD_BOTH_LIBRARIES option in README.md with usage examples The OUTPUT_NAME property sets only the base name; CMake automatically adds the correct platform-specific extension (.so on Linux, .dylib on macOS).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
40-46: Align the documented CMake minimum version.Line 42 lists CMake (>=3.10) but Line 46 states (>=3.16), and CMakeLists.txt requires version 3.16. Update line 42 to match:
- - [CMake](`#cmake`) (>=3.10) + - [CMake](`#cmake`) (>=3.16)
🤖 Fix all issues with AI agents
In `@CMakeLists.txt`:
- Around line 65-74: The shared target cpp_dotenv_shared currently sets
OUTPUT_NAME to cpp_dotenv which conflicts with the static lib import on MSVC;
change the shared target properties to keep the DLL name as cpp_dotenv but give
the import library a distinct name (use set_target_properties(cpp_dotenv_shared
PROPERTIES OUTPUT_NAME cpp_dotenv IMPORT_LIBRARY cpp_dotenv_shared_import) or
similar) so when BUILD_BOTH_LIBRARIES is enabled on Windows the .lib import file
won’t overwrite the static .lib; apply this to the cpp_dotenv_shared target
(modify the set_target_properties call) and conditionally do it only for
MSVC/Windows if you prefer.
| # Shared library | ||
| add_library(cpp_dotenv_shared SHARED ${CPP_DOTENV_SRC}) | ||
| target_link_libraries(cpp_dotenv_shared | ||
| ${ENVIRON_LIB} | ||
| ${PARSER_LIB} | ||
| ) | ||
| target_include_directories(cpp_dotenv_shared PUBLIC ${CPP_DOTENV_INCLUDE_DIRS}) | ||
| target_compile_options(cpp_dotenv_shared PRIVATE ${CPP_DOTENV_COMPILE_OPTIONS}) | ||
| # Set output name to libcpp_dotenv.so/.dylib (instead of libcpp_dotenv_shared.so) | ||
| set_target_properties(cpp_dotenv_shared PROPERTIES OUTPUT_NAME cpp_dotenv) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find CMakeLists.txt and read the full context around the problematic lines
find . -name "CMakeLists.txt" -type fRepository: ProxySQL/cpp-dotenv
Length of output: 575
🏁 Script executed:
# Read the CMakeLists.txt file to see the full context
# Let's first check its size
wc -l CMakeLists.txtRepository: ProxySQL/cpp-dotenv
Length of output: 80
🏁 Script executed:
# Read the relevant sections of CMakeLists.txt
cat -n CMakeLists.txt | head -100Repository: ProxySQL/cpp-dotenv
Length of output: 4262
Prevent Windows .lib import library collision when building both variants.
With OUTPUT_NAME set to cpp_dotenv for both targets, MSVC on Windows generates a .lib file for the static library and an import library with the same name for the shared DLL, causing an overwrite. On Unix/Linux (.a/.so) and macOS (.a/.dylib), there is no collision. If the BUILD_BOTH_LIBRARIES option is used on Windows, the shared target's import library must have a distinct name or output directory.
🛠️ Suggested fix (keep DLL name, rename import .lib)
# Set output name to libcpp_dotenv.so/.dylib (instead of libcpp_dotenv_shared.so)
set_target_properties(cpp_dotenv_shared PROPERTIES OUTPUT_NAME cpp_dotenv)
+ if(WIN32)
+ # Avoid collision with the static cpp_dotenv.lib
+ set_target_properties(cpp_dotenv_shared PROPERTIES ARCHIVE_OUTPUT_NAME cpp_dotenv_shared)
+ endif()🤖 Prompt for AI Agents
In `@CMakeLists.txt` around lines 65 - 74, The shared target cpp_dotenv_shared
currently sets OUTPUT_NAME to cpp_dotenv which conflicts with the static lib
import on MSVC; change the shared target properties to keep the DLL name as
cpp_dotenv but give the import library a distinct name (use
set_target_properties(cpp_dotenv_shared PROPERTIES OUTPUT_NAME cpp_dotenv
IMPORT_LIBRARY cpp_dotenv_shared_import) or similar) so when
BUILD_BOTH_LIBRARIES is enabled on Windows the .lib import file won’t overwrite
the static .lib; apply this to the cpp_dotenv_shared target (modify the
set_target_properties call) and conditionally do it only for MSVC/Windows if you
prefer.
- Fix README CMake version mismatch: update from >=3.10 to >=3.16 to match CMakeLists.txt requirement - Fix Windows MSVC import library conflict: when BUILD_BOTH_LIBRARIES is ON, both static and shared libraries would produce cpp_dotenv.lib on Windows. Add IMPORT_SUFFIX "_shared.lib" to the shared library's import library to avoid this conflict on Windows/MSVC.
Summary
This PR adds a
BUILD_BOTH_LIBRARIESCMake option that allows building both static and shared libraries in a single build directory, simplifying the ProxySQL TAP test build process.Changes
BUILD_BOTH_LIBRARIESCMake option (default: OFF)libcpp_dotenv_static.aandlibcpp_dotenv_shared.soin one directoryBUILD_SHARED_LIBS=ON/OFFapproachUse Case for ProxySQL TAP Tests
Previously required two separate build directories:
Now can optionally build both in a single directory:
Testing
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.