-
Notifications
You must be signed in to change notification settings - Fork 446
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
If external nlohmann::json is not found, then use one from our submodule #865
Conversation
Codecov Report
@@ Coverage Diff @@
## main #865 +/- ##
==========================================
+ Coverage 95.49% 95.50% +0.02%
==========================================
Files 156 156
Lines 6620 6620
==========================================
+ Hits 6321 6322 +1
+ Misses 299 298 -1
|
OFF | ||
CACHE INTERNAL "") | ||
# This option allows to link nlohmann_json::nlohmann_json target | ||
add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/nlohmann-json) |
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.
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.
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.
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
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.
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 :)
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.
@lalitb - Sounds good. I can improve it in one of my other build infra PRs!
Fixes #843 ( partially 😄 )
Quick recap: despite the fact that we clone
nlohmann::json
library as one of our submodules, we do not attempt to use that by default. Neither in Linux nor in Windows. This is a bit illogical, because for Windows - we expect it to be provided byvcpkg
, on Mac we install it withbrew
, and on Ubuntu we expect it to be installed viaapt
... Despite the fact that we actually have it right there - in the submodule!Changes
The following components depend on / require
nlohmann/json.hpp
:Given that:
I'd like to consolidate the usage of it as follows:
CMakeLists.txt
The library itself is "pay-per-play". It's header only. If someone builds a build combo without it, - fine, we're good, we're just not including it. We require it. But we don't include it. This approach makes the overall SDK build / consumption experience much smoother.. And we no longer need to explain why we have JSON right there in the submodule but it's not found during the build. Making this world quite a bit more sane and reasonable place.
Note that we presently don't install / export / deploy the json.hpp library together with OpenTelemetry SDK headers. The library remains private in most cases. If we ever need to export that as part of OpenTelemetry SDK, i.e. install it to user include directories, then we could set the
JSON_Install
option to ON. This installs our local snapshot of nlohmann/json.hpp.. But in cases where users really feel like their code depends on json.hpp, they might as well install their own version themselves via pkg manager.Similar approach could be implemented for
benchmark
,googletest
andms-gsl
... But for for now I'm tackling this only fornlohmann/json.hpp
.