Skip to content

Add json.hpp to CI and Build Tools#306

Merged
reyang merged 8 commits into
open-telemetry:masterfrom
maxgolov:maxgolov/cmake_fixes
Aug 31, 2020
Merged

Add json.hpp to CI and Build Tools#306
reyang merged 8 commits into
open-telemetry:masterfrom
maxgolov:maxgolov/cmake_fixes

Conversation

@maxgolov
Copy link
Copy Markdown
Contributor

@maxgolov maxgolov commented Aug 28, 2020

Resolves #281

Adding nlohmann/json json.hpp to CMake build.

Practical reason today:

  • zpages implementation requires nlohmann json, but it only added to Bazel build (CMake part was not done)

Future considerations:

  • JSON for Modern C++ is a robust and convenient friendly-licensed library used by many other prominent projects, that allows to express complex objects in intuitive, idiomatic way similar to Javascript's native expresson of JSON objects. It also provides a built-in MsgPack encoder, if we ever require one.

@maxgolov maxgolov requested a review from a team August 28, 2020 10:31
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 28, 2020

Codecov Report

Merging #306 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #306   +/-   ##
=======================================
  Coverage   94.59%   94.59%           
=======================================
  Files         146      146           
  Lines        6629     6629           
=======================================
  Hits         6271     6271           
  Misses        358      358           

Comment thread ci/setup_cmake.sh
sudo \
libcurl4-openssl-dev
libcurl4-openssl-dev \
nlohmann-json-dev \
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Main reason why I need to ignore the errors / skip missing, is because the package name / version is different between distros. I'm trying to keep this setup script universal to work for any recent distro (Ubuntu 16.xx, 18.xx, 20.xx).. Basically, we "install whatever available" json.hpp . Ideally we may want to reconsider, and either use PPA to install latest, OR add specific version we need as git submodule.

@maxgolov maxgolov linked an issue Aug 28, 2020 that may be closed by this pull request
@reyang
Copy link
Copy Markdown
Member

reyang commented Aug 29, 2020

Adding nlohmann json.hpp to CMake build

@maxgolov would you update the description to include "why do we do this"? Thanks.

@maxgolov
Copy link
Copy Markdown
Contributor Author

@maxgolov would you update the description to include "why do we do this"? Thanks.

Added description.

Copy link
Copy Markdown
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@reyang reyang merged commit 965a0aa into open-telemetry:master Aug 31, 2020
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 17, 2025
[CMake] Add missing CMake keyword for target_link_libraries (open-telemetry#3442)
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.

Add CMake Support for the JSON C++ Dependency

2 participants