Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: development
Choose a base branch
from
Open

Add support to use system provided nlohmann_json library if requested #14155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26f0d8f
Add support to use system provided nlohmann_json library if requested
rhabacker Jul 22, 2025
115f12e
Fix bug that linking to nlohmann_json library was not possible
rhabacker Jul 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMake/lrs_options.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,4 @@ option(ENABLE_SECURITY_FLAGS "Enable additional compiler security flags to enhan
option(USE_EXTERNAL_LZ4 "Use externally build LZ4 library instead of building and using the in this repo provided version" OFF)
option(BUILD_ASAN "Enable AddressSanitizer" OFF)
mark_as_advanced(BUILD_ASAN)
option(USE_EXTERNAL_NLOHMANN_JSON "Use external build nlohmann_json library instead of building and using the in this repo provided version" OFF)
10 changes: 7 additions & 3 deletions third-party/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
string(REPLACE ${PROJECT_SOURCE_DIR}/ "" _rel_path ${CMAKE_CURRENT_LIST_DIR})

pop_security_flags() # remove security flags for third party, as we cannot guarantee their security enforcment

add_subdirectory( "${CMAKE_CURRENT_LIST_DIR}/rsutils" )
if(USE_EXTERNAL_NLOHMANN_JSON)
find_package(nlohmann_json 3.11.3 REQUIRED)
Copy link

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))

Copy link
Author

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

librealsense/third-party/rsutils/CMakeLists.txt

Line 7 in 6dcf714

# 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.

Copy link
Author

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.

Copy link

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.

Copy link
Author

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.

Copy link
Author

@rhabacker rhabacker Jul 23, 2025 •
edited
Loading

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.

Copy link

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.

Copy link
Author

@rhabacker rhabacker Jul 23, 2025 •
edited
Loading

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?

Copy link

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.

Copy link
Author

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

librealsense/third-party/CMakeLists.txt

Line 4 in 6dcf714

add_subdirectory( "${CMAKE_CURRENT_LIST_DIR}/rsutils" )
and

librealsense/third-party/CMakeLists.txt

Line 8 in 6dcf714

include(CMake/external_json.cmake)
.

After switching the order, nlohmann_json can be added as private library in both cases.

else()
include(CMake/external_json.cmake)
endif()

pop_security_flags() # remove security flags for third party, as we cannot guarantee their security enforcment
add_subdirectory( "${CMAKE_CURRENT_LIST_DIR}/rsutils" )

include(CMake/external_json.cmake)
# Add additional include directories to allow file to include rosbag headers
include(${_rel_path}/realsense-file/config.cmake)

Expand Down
4 changes: 1 addition & 3 deletions third-party/rsutils/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,13 @@ cmake_minimum_required(VERSION 3.8.0) # source_group(TREE)
project( rsutils )

add_library( ${PROJECT_NAME} STATIC "" )
# We cannot directly interface with nlohmann_json (doesn't work on bionic)
#target_link_libraries( ${PROJECT_NAME} PUBLIC nlohmann_json )
target_link_libraries( ${PROJECT_NAME} PRIVATE nlohmann_json )
target_compile_features( ${PROJECT_NAME} PUBLIC cxx_std_14 )
set_target_properties( ${PROJECT_NAME} PROPERTIES FOLDER Library )

target_include_directories( ${PROJECT_NAME}
PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/third-party/json/include>
)

# Headers -----------------------------------------------------------------------------------
Expand Down
Loading