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

Find nlohmann_json dependency in cmake config file when proj is confi… #2763

Closed
wants to merge 2 commits into from
Closed

Conversation

dirkvdb
Copy link

@dirkvdb dirkvdb commented Jul 3, 2021

@rouault
Copy link
Member

rouault commented Jul 10, 2021

It doesn't feel right to have nlohmann/json requirement exposed to external users of PROJ, as it is purely an internal implementation detail

@hobu
Copy link
Contributor

hobu commented Jul 10, 2021

I concur with @rouault.

I guess I haven't been tracking so closely, but did we ever advertise that we would work with an external nlohmann? The established pattern is for projects that use it to vendor it, namespace it, and then not leak it into their external APIs. This is because many other libraries, PDAL in my case, are also using nlohmann, and the opportunity for misery increases substantially if libraries it is using start doing putting their own vendored copies in their public APIs.

@rouault
Copy link
Member

rouault commented Jul 10, 2021

I guess I haven't been tracking so closely, but did we ever advertise that we would work with an external nlohmann?

I'm culprit for that. This has been added in 8.1 as an option. If using external nlohmann, then the official namespace is used (and that way folks combining different libraries that use it can save some binary space, and this is more friendly of distributions w.r.t security issues). If using internal nlohmann, then the proj_nlohmann namespace is used to avoid potential version clash.
I'm not sure why cmake insists about having to require nlohmann when using the library. It probably lacks hints that this is a header only dependency and that the nlohmann headers aren't exposed in our own headers

@rouault
Copy link
Member

rouault commented Jul 14, 2021

Superseded per #2780

@rouault rouault closed this Jul 14, 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.

CMake targets broken in 8.1.0 when using external nlohmann_json
3 participants