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 nlohmann_json is missing in customer's build system, then install it from submodule #870

Merged
merged 11 commits into from
Jun 22, 2021
Merged

If nlohmann_json is missing in customer's build system, then install it from submodule #870

Changes from all commits
Commits
Show all changes
11 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:/open-telemetry/opentelemetry…
maxgolov May 13, 2021
0540d32
Merge branch 'main' of http:/open-telemetry/opentelemetry…
maxgolov May 19, 2021
c437cef
Merge branch 'main' of http:/open-telemetry/opentelemetry…
maxgolov May 20, 2021
3f37a7a
Merge branch 'main' of http:/open-telemetry/opentelemetry…
maxgolov Jun 4, 2021
157c906
Merge branch 'main' of http:/open-telemetry/opentelemetry…
maxgolov Jun 15, 2021
649e722
Merge branch 'main' of http:/open-telemetry/opentelemetry…
maxgolov Jun 17, 2021
c337452
Merge branch 'main' of http:/open-telemetry/opentelemetry…
maxgolov Jun 18, 2021
50d5e1f
Fix for nlohmann json dependency for Windows builds
maxgolov Jun 18, 2021
28a5c02
Smarter logic for nlohmann json
maxgolov Jun 21, 2021
8c8c960
Do not require nlohmann_json for API-only build
maxgolov Jun 21, 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
43 changes: 27 additions & 16 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,22 +228,33 @@ 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)
# 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")
if(NOT WITH_API_ONLY)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lidavidm - I hope that should fix the build for API-only build.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can confirm this fixes the API-only build, thanks!

It would be nice if we could still build the rest of the library that doesn't require json without this dependency, though (e.g. the SDK), though I can understand if that would get complicated.

Copy link
Contributor Author

@maxgolov maxgolov Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@lidavidm - it happens that in the majority of cases (80% scenarios I'd say), we do actually require nlohmann/json.hpp, e.g. for ETW on Windows, for zPages (internal monitor of SDK statistics), for Zipkin exporter, as well as for OTLP-HTTP exporter. Plus for all tests, and generally for examples too.

Since we got you covered well - the case when the build is done only for API and nothing else, json is now optional for that scenario.. I think it's gonna be too much of a hassle to make json optional for the rest. As you noticed, it gets complicated - given that most components require it. And we give two alternatives - either consume json library from user-provided deps. Or use the one we cloned from recursive submodules, both options now work cross-plat.

We can probably consider further improvements in a follow-up PR? But as low priority. Since all immediate scenarios have been addressed.

Copy link
Member

Choose a reason for hiding this comment

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

I agree on having further cleanup in a follow-up PR. I think the most common usage scenario would be instrumentation libraries devs using otel-cpp api, and application developers using otel-gRPC exporter. And both these scenarios don't depend on JSON and so shouldn't fail. Though I do see the hassle involved here to work it cross-platform.

Thanks for fixing the API only build :)

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 - My worry is that OTLP-HTTP exporter may also be common. This one does require JSON. Same as Zipkin. Also I think the zPages project is kinda common across all languages. The idea is we use zPages for debugging OpenTelemetry in-proc.. that build flavor of final SDK dll / shared object bit, with zPages in it, would require json.hpp

While I agree we may not need it for OTLP-gRPC only build without zPages, in most other configurations we end up needing json.hpp anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

I totally understand. I think the biggest thing is just that the release tarballs don't include the submodules, but that's a Github issue.

# nlohmann_json package is required for most SDK build configurations
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
ON
CACHE INTERNAL "")
if(EXISTS ${PROJECT_SOURCE_DIR}/third_party/nlohmann-json)
# This option allows to link nlohmann_json::nlohmann_json target
add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/nlohmann-json)
# This option allows to add header to include directories
include_directories(
${PROJECT_SOURCE_DIR}/third_party/nlohmann-json/single_include)
else()
message(
FATAL_ERROR
"\nnlohmann_json package was not found. Please either provide it manually or clone with submodules. "
"To initialize, fetch and checkout any nested submodules, you can use the following command:\n"
"git submodule update --init --recursive")
endif()
else()
message("Using external nlohmann::json")
endif()
maxgolov marked this conversation as resolved.
Show resolved Hide resolved
endif()

if(WITH_OTLP)
Expand Down