-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add support to use system provided nlohmann_json library if requested #14155
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
base: development
Are you sure you want to change the base?
Add support to use system provided nlohmann_json library if requested #14155
Conversation
Can one of the admins verify this patch? |
|
||
include(CMake/external_json.cmake) | ||
if(BUILD_WITH_SYSTEM_NLOHMANN_JSON) | ||
find_package(nlohmann_json 3.11.3 REQUIRED) |
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.
Did you check that this actually changes what is used?
(The lz4 option is not enough to keep the build from using the vendored headers, #13803 (comment))
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.
I assume you are refering to this statement at
# We cannot directly interface with nlohmann_json (doesn't work on bionic) |
Normally as the nlohmann_json library provides a cmake target required include directories are added out of the box, but after uncommenting
target_link_libraries( ${PROJECT_NAME} PUBLIC nlohmann_json )
I get
CMake Error in CMakeLists.txt:
install(EXPORT "realsense2Targets" ...) includes target "rsutils" which
requires target "nlohmann_json" that is not in any export set.
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.
According to https://stackoverflow.com/questions/5378528/install-export-problem-for-library-with-dependencies#comment134383587_71080574 the reason for this problem is that rsutils is built and installed as a static library, which requires the export of all dependent libraries.
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.
I assume you are refering to this statement at ...
No. I refer to what kind of #include
statements actually refer to nlohmann-json headers. See my linked comment for how devendoring is currently incomplete for lz4.
# We cannot directly interface with nlohmann_json (doesn't work on bionic)
(Maybe that comment refers to an old installation which doesn't have nlohmann-json.)
Normally as the nlohmann_json library provides a cmake target required include directories are added out of the box, but ... I get ...
target "rsutils" which requires target "nlohmann_json" that is not in any export set.
Actually pretty standard situation, and not related to your change. When the exported interface of a build target (rsutils) refers to another build target (nlohmann_json), the other target must be exported to.
Your change introduces a similar problem but with another imported target. You must use find_dependency
or similar in the installed config file when an exported interface refers to an imported target.
Unless you take more control of the exported interface:
- Does it need to be
PUBLIC
link library? This implies downstream use for include dirs, so it introduces a dependency regardless of dynamic or static libs.
PRIVATE
would reduce the exported transitive dependency to a link-only property for static libs. - Does nlohmann-json need to be exposed at all? The package is header-only, and if its headers aren't exposed, it could be a
$<BUILD_ONLY:...>
link lib, not being exported with CMake config.
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.
hat this actually changes what is used?
This change (without the cmake option) has been used to build binaries at https://build.opensuse.org/package/show/hardware/librealsense since 2024-09-30.
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.
Using an external package is a must, as cmake has no internet access to fetch data, at least on remote build systems like build.opensuse.org.
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.
I'm familiar with DEB and RPM packaging on build.opensuse.org. And with CMake and vcpkg. In fact, the vcpkg port is even more aggressive in devendoring ATM.
In this PR, the result of find_package
isn't used. It has no effect on the include dirs. This is a problem when you don't want to use or install nlohmann-json in standard system locations.
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.
Using nlohmann as an external package creates in /usr/lib64/cmake/nlohmann_json/nlohmann_jsonConfig.cmake
add_library(nlohmann_json INTERFACE IMPORTED)
set_target_properties(nlohmann_json PROPERTIES
INTERFACE_LINK_LIBRARIES nlohmann_json::nlohmann_json
Include directories are defined in /usr/lib64/cmake/nlohmann_json/nlohmann_jsonTargets.cmake
add_library(nlohmann_json::nlohmann_json INTERFACE IMPORTED)
set_target_properties(nlohmann_json::nlohmann_json PROPERTIES
INTERFACE_COMPILE_DEFINITIONS "\$<\$<NOT:\$<BOOL:ON>>:JSON_USE_GLOBAL_UDLS=0>;\$<\$<NOT:\$<BOOL:ON>>:JSON_USE_IMPLICIT_CONVERSIONS=0>;\$<\$<BOOL:OFF>: JSON_DISABLE_ENUM_SERIALIZATION=1>;\$<\$<BOOL:OFF>:JSON_DIAGNOSTICS=1>;\$<\$<BOOL:OFF>: JSON_USE_LEGACY_DISCARDED_VALUE_COMPARISON=1>"
INTERFACE_COMPILE_FEATURES "cxx_std_11"
INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include"
)
The package is movable (by IMPORT_PREFIX) and should therefore provide correct include paths when moved to another location. Have I overlooked something?
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.
You overlooked that nothing here makes use of the imported variables and properties.
Nothing is added to the actual include dirs of the build.
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.
When using the embedded nlohman_json library the same target is created, so it would be addable as private library too, but it cannot be added to rsutils because it takes place before the definition of nlohman_json and therefore cannot be found, see
add_subdirectory( "${CMAKE_CURRENT_LIST_DIR}/rsutils" ) |
include(CMake/external_json.cmake) |
After switching the order, nlohmann_json can be added as private library in both cases.
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.
Pull Request Overview
This PR adds CMake configuration support to optionally use the system-provided nlohmann_json library instead of the bundled version. This provides flexibility for distributions and users who prefer to use their system's package manager for dependency management.
- Adds a new CMake option
BUILD_WITH_SYSTEM_NLOHMANN_JSON
to control library source selection - Modifies the build logic to conditionally use
find_package()
for system nlohmann_json or fall back to the existing bundled version - Maintains backward compatibility by defaulting to the current behavior (using bundled library)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
CMake/lrs_options.cmake | Adds the new CMake option definition with default OFF value |
third-party/CMakeLists.txt | Implements conditional logic to use system or bundled nlohmann_json library |
Comments suppressed due to low confidence (1)
CMake/lrs_options.cmake:60
- [nitpick] The option name BUILD_WITH_SYSTEM_NLOHMANN_JSON is inconsistent with the existing naming pattern. Other similar options like USE_EXTERNAL_LZ4 follow the USE_EXTERNAL_* pattern. Consider renaming to USE_EXTERNAL_NLOHMANN_JSON for consistency.
option(BUILD_WITH_SYSTEM_NLOHMANN_JSON "Use system provided nlohmann_json library" OFF)
4af0b56
to
2812f5f
Compare
The use of the library provided by the system is activated with the cmake option -DUSE_EXTERNAL_NLOHMANN_JSON=ON.
2812f5f
to
c3d02a5
Compare
The problem was caused by the target rsutils being added before the library nlohmann_json.
c3d02a5
to
115f12e
Compare
The use of the library provided by the system is activated with the cmake option -DUSE_EXTERNAL_NLOHMANN_JSON=ON.