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

Add JSON C++ support - Bazel #190

Merged
merged 28 commits into from Jul 21, 2020
Merged

Conversation

jajanet
Copy link
Contributor

@jajanet jajanet commented Jul 17, 2020

PR to import JSON in C++ as an external dependency. Would be useful for zPages/TraceZ and other SDK features for OpenTelemetry. @maxgolov

Reference: nlohmann/json#1606

Copy link
Contributor

@maxgolov maxgolov left a comment

Choose a reason for hiding this comment

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

I assume this is for Bazel only right now. There are two approaches for CMake:

  • vcpkg install nlohmann-json - safe; OR
  • add it as submodule to third_party - I don't think we agreed yet we can use submodules.

@pyohannes
Copy link
Contributor

add it as submodule to third_party - I don't think we agreed yet we can use submodules.

I think this would be necessary to make it work cross-platform. There were some discussions about vendoring strategies with CMake/Bazel, but no final conclusions were made.

There's also the option to just vendor the single include version. It almost feels like a bit of an overkill to add a submodule if all we need is just a single header.

@pyohannes
Copy link
Contributor

This is still marked as a draft. Is it intended to solve all the TODOs mentioned in this PR?

@jajanet
Copy link
Contributor Author

jajanet commented Jul 20, 2020

I think we could leave it as is, and merge. @maxgolov said he can handle adding CMake support.

Alternatively, different features could also add JSON C++ via CMake by adding it manually to their CMake file:

find_package(nlohmann_json 3.2.0 REQUIRED)
...
add_library(foo ...)
...
target_link_libraries(foo PRIVATE nlohmann_json::nlohmann_json)

@pyohannes
Copy link
Contributor

I think we could leave it as is, and merge. @maxgolov said he can handle adding CMake support.

Ok. Can you please write issues for TODOs that remain open after merging this PR?

@jajanet
Copy link
Contributor Author

jajanet commented Jul 20, 2020

Will do, thank you!

@jajanet jajanet changed the title Add JSON C++ support Add JSON C++ support - Bazel Jul 20, 2020
@jajanet jajanet marked this pull request as ready for review July 20, 2020 18:48
@jajanet jajanet requested a review from a team as a code owner July 20, 2020 18:48
@reyang reyang self-requested a review July 20, 2020 21:53
@reyang reyang added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Jul 20, 2020
@reyang reyang merged commit 2c8b0e3 into open-telemetry:master Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants