Skip to content
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

Jump to bottom

If external nlohmann::json is not found, then use one from our submodule #865

Merged
merged 8 commits into from
Jun 18, 2021
Merged

If external nlohmann::json is not found, then use one from our submodule #865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
076effa
Add curl via vcpkg to WIndows builds
maxgolov May 11, 2021
0f13e20
Merge branch 'main' of http://github.com/open-telemetry/opentelemetry…
maxgolov May 13, 2021
0540d32
Merge branch 'main' of http://github.com/open-telemetry/opentelemetry…
maxgolov May 19, 2021
c437cef
Merge branch 'main' of http://github.com/open-telemetry/opentelemetry…
maxgolov May 20, 2021
3f37a7a
Merge branch 'main' of http://github.com/open-telemetry/opentelemetry…
maxgolov Jun 4, 2021
157c906
Merge branch 'main' of http://github.com/open-telemetry/opentelemetry…
maxgolov Jun 15, 2021
649e722
Merge branch 'main' of http://github.com/open-telemetry/opentelemetry…
maxgolov Jun 17, 2021
dcbf2b5
CMake fix for using local nlohmann::json library from submodule
maxgolov Jun 17, 2021
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
25 changes: 20 additions & 5 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,24 @@ endif()
# GNUInstallDirs.
include(GNUInstallDirs)

find_package(nlohmann_json QUIET)
if(NOT nlohmann_json_FOUND)
message("Using local nlohmann::json from submodule")
set(JSON_BuildTests
OFF
CACHE INTERNAL "")
set(JSON_Install
OFF
CACHE INTERNAL "")
# This option allows to link nlohmann_json::nlohmann_json target
add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/nlohmann-json)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it fail cmake configure if submodules are not checked out? Just thinking of the scenarios if the user wants to build api and sdk which ideally doesn't have json/curl requirements, we shouldn't let configure fail in those cases.

Copy link
Contributor Author

@maxgolov maxgolov Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is required for most build configurations (certainly required for default build configuration), I can add a check if directory exists to alleviate your concern. If it does not exist, I can trigger a warning recommending to do recursive cloning or installing nlohmann_json package manually? I think in most documents we do recommend cloning recursively. Certainly for Windows we do recommend it here

Copy link
Member

@lalitb lalitb Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes agreed. How about not failing the cmake configure if WITH_API_ONLY is enabled and json is not installed? Though I don't have strong opinion on this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lalitb - Sounds good. I can improve it in one of my other build infra PRs!

# This option allows to add header to include directories
include_directories(
${PROJECT_SOURCE_DIR}/third_party/nlohmann-json/single_include)
else()
message("Using external nlohmann::json")
endif()

if(WITH_OTLP)
set(protobuf_MODULE_COMPATIBLE ON)
find_package(Protobuf)
Expand Down Expand Up @@ -268,13 +286,13 @@ if(WITH_OTLP)
include(cmake/opentelemetry-proto.cmake)
include(CMakeDependentOption)
find_package(CURL)
find_package(nlohmann_json)

cmake_dependent_option(
WITH_OTLP_GRPC "Whether to include the OTLP gRPC exporter in the SDK" ON
"gRPC_FOUND" OFF)
cmake_dependent_option(
WITH_OTLP_HTTP "Whether to include the OTLP http exporter in the SDK" ON
"CURL_FOUND;nlohmann_json_FOUND" OFF)
"CURL_FOUND" OFF)
endif()

list(APPEND CMAKE_PREFIX_PATH "${CMAKE_BINARY_DIR}")
Expand Down Expand Up @@ -329,9 +347,6 @@ if(NOT WITH_API_ONLY)
endif()
endif()

# Add nlohmann/json submodule to include directories
include_directories(third_party/nlohmann-json/single_include)

# Export cmake config and support find_packages(opentelemetry-cpp CONFIG) Write
# config file for find_packages(opentelemetry-cpp CONFIG)
set(INCLUDE_INSTALL_DIR "${CMAKE_INSTALL_INCLUDEDIR}")
Expand Down
2 changes: 0 additions & 2 deletions exporters/etw/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
find_package(nlohmann_json REQUIRED)

add_library(opentelemetry_exporter_etw INTERFACE)

target_include_directories(
Expand Down
7 changes: 2 additions & 5 deletions exporters/otlp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,13 @@ endif()

if(WITH_OTLP_HTTP)
find_package(CURL REQUIRED)
find_package(nlohmann_json REQUIRED)
add_library(opentelemetry_exporter_otlp_http src/otlp_http_exporter.cc)

set_target_properties(opentelemetry_exporter_otlp_http
PROPERTIES EXPORT_NAME otlp_http_exporter)

target_link_libraries(
opentelemetry_exporter_otlp_http
PUBLIC opentelemetry_otlp_recordable http_client_curl
nlohmann_json::nlohmann_json)
target_link_libraries(opentelemetry_exporter_otlp_http
PUBLIC opentelemetry_otlp_recordable http_client_curl)

list(APPEND OPENTELEMETRY_OTLP_TARGETS opentelemetry_exporter_otlp_http)
endif()
Expand Down
11 changes: 1 addition & 10 deletions ext/test/w3c_tracecontext_test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,10 @@ find_package(CURL)
if(NOT CURL_FOUND)
message(WARNING "Skipping example_w3c_tracecontext_test: CURL not found")
else()
find_package(nlohmann_json QUIET)
if(NOT nlohmann_json_FOUND)
# Add library from git submodule to include path
include_directories(
${PROJECT_SOURCE_DIR}/third_party/nlohmann-json/single_include)
else()
# Add header-only library found by CMake
set(NLOHMANN_JSON_LIB nlohmann_json::nlohmann_json)
endif()
add_executable(w3c_tracecontext_test main.cc)
target_link_libraries(
w3c_tracecontext_test
PRIVATE ${CMAKE_THREAD_LIBS_INIT} opentelemetry_trace http_client_curl
opentelemetry_exporter_ostream_span ${CURL_LIBRARIES}
${NLOHMANN_JSON_LIB})
nlohmann_json::nlohmann_json)
endif()