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.

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

Fix nlohmann_json package dependency #1017

Merged
merged 6 commits into from
Oct 19, 2021

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Oct 18, 2021

Fixes # 981

  • The nlohmann_json package should be required only when the component using it are part of the build, else be ignored.
  • Fail with an appropriate error if this package is missing, based on whether the build is initiated from opentelemetry-cpp git repo or release tarball.
  • Make zpage and test for validating w3c trace-context optional in build, as they both use nlohmann_json.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb lalitb requested a review from a team October 18, 2021 16:23
@codecov
Copy link

codecov bot commented Oct 18, 2021

Codecov Report

Merging #1017 (915cbbc) into main (bfab71a) will decrease coverage by 0.58%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1017      +/-   ##
==========================================
- Coverage   95.41%   94.83%   -0.57%     
==========================================
  Files         161      151      -10     
  Lines        6833     5972     -861     
==========================================
- Hits         6519     5663     -856     
+ Misses        314      309       -5     
Impacted Files Coverage Δ
sdk/src/trace/span.cc 81.71% <0.00%> (-9.75%) ⬇️
...include/opentelemetry/sdk/trace/multi_recordable.h 88.00% <0.00%> (-6.00%) ⬇️
...include/opentelemetry/sdk/common/circular_buffer.h 97.92% <0.00%> (-2.08%) ⬇️
ext/test/zpages/tracez_data_aggregator_test.cc
...de/opentelemetry/ext/zpages/threadsafe_span_data.h
ext/test/zpages/tracez_processor_test.cc
ext/include/opentelemetry/ext/zpages/tracez_data.h
ext/src/zpages/tracez_processor.cc
...nclude/opentelemetry/ext/zpages/tracez_processor.h
ext/src/zpages/tracez_data_aggregator.cc
... and 3 more

# This option allows to add header to include directories
include_directories(
${PROJECT_SOURCE_DIR}/third_party/nlohmann-json/single_include)
if(EXISTS ${PROJECT_SOURCE_DIR}/.git)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the repo is cloned without submodule, will this work?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have now modified the second if condition to make it more explicit.

Error message if cloned without submodules,

  nlohmann_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:

  git submodule update --init --recursive

Error message if using release tar-ball:

CMake Error at CMakeLists.txt:290 (find_package):
  By not providing "Findnlohmann_json.cmake" in CMAKE_MODULE_PATH this
  project has asked CMake to find a package configuration file provided by
  "nlohmann_json", but CMake did not find one.

  Could not find a package configuration file provided by "nlohmann_json"
  with any of the following names:

    nlohmann_jsonConfig.cmake
    nlohmann_json-config.cmake

  Add the installation prefix of "nlohmann_json" to CMAKE_PREFIX_PATH or set
  "nlohmann_json_DIR" to a directory containing one of the above files.  If
  "nlohmann_json" provides a separate development package or SDK, be sure it
  has been installed.

@ThomsonTan ThomsonTan merged commit 55a6517 into open-telemetry:main Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants