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

nlohmann json is available as a package, no need to download it #2874

Closed
yurivict opened this issue Jul 14, 2019 · 7 comments · Fixed by #4747
Closed

nlohmann json is available as a package, no need to download it #2874

yurivict opened this issue Jul 14, 2019 · 7 comments · Fixed by #4747
Assignees
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@yurivict
Copy link

yurivict commented Jul 14, 2019

See cmake/DownloadNlohmannJson.cmake

Repology shows that nlohmann-json is available on virtually all distros: https://repology.org/project/nlohmann-json/versions
There is no need to download it.

Downloads aren't allowed in the modern packaging systems due to security concerns, and such downloads only clutter the packaging process.

@ghost
Copy link

ghost commented Jul 14, 2019

It's suggested in #2802 (comment) that we use the same version of nlohmann_json that the customer has installed. In other words, it's planned that we will use find_package to discover it rather than downloading it.

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jul 14, 2019
@coryan coryan added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed triage me I really want to be triaged. labels Jul 15, 2019
@amensel
Copy link

amensel commented Jul 31, 2020

Hoping to draw some attention back to this one, especially in relation to #4094.

GCC 10 has improved improved detection of incomplete types. This revealed a bug in nlohmann::json: nlohmann/json#1920

Unfortunately, versions later than 3.4.0 (the version vendored into google-cloud-cpp) are no longer safe to use alongside 3.4.0. The header guards have changed as has the template for the basic_json type. This creates a nasty little fight for which I still haven't found a clean resolution.

It would be great if nlohmann::json could be provided as a package like other dependencies in e.g. the Protobuf or gRPC builds.

If not that, it would at least be polite to not expose the touched up and customized version that this library uses in its public includes, despite whatever claims are made in the comments of nljson.h about it being "safe".

@coryan
Copy link
Contributor

coryan commented Jul 31, 2020

Thanks for letting us know about the additional problems. I have moved up this issue in our backlog, I do not think we will be able to fix it for the next release (due in less than a week). I am hoping for the 2020-09 release.

@coryan coryan self-assigned this Jul 31, 2020
@amensel
Copy link

amensel commented Jul 31, 2020

Awesome! Thanks for the speedy response.

@coryan
Copy link
Contributor

coryan commented Aug 1, 2020

As you may have noticed we now have a PR that will fix this problem. But note that we have not upgraded to a newer version of nlohmann_json with #4747. We are staying on v3.4.0 to keep the PR manageable. The good news is that it seems to work, some of our builds use vcpkg and they are are using v3.8.0 of the nlohmann_json library, but I would like to run all builds with that version before declaring success.

@amensel
Copy link

amensel commented Aug 1, 2020

Ah, I had missed that; I'll pull it into our build in the interim while we wait for the official release.

Thanks!

@coryan
Copy link
Contributor

coryan commented Aug 1, 2020

No problem, please let us know if you run into any trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
4 participants