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

should't the cmake install target be to nlohman/json.hpp #668

Closed
dan-42 opened this issue Jul 27, 2017 · 7 comments
Closed

should't the cmake install target be to nlohman/json.hpp #668

dan-42 opened this issue Jul 27, 2017 · 7 comments
Assignees
Labels
kind: enhancement/improvement kind: question solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@dan-42
Copy link
Contributor

dan-42 commented Jul 27, 2017

dear @nlohmann and other developers
first of all many thx for this awesome lib. The usage and simplicity is very cool and I and some former coworkers use it on many places in production and private projects.

So my question: is it on purpose that cmake exposes the json.hpp for include directly?
after all a cmake install installes the file into nlohmann/json.hpp and the usage is also
inside the namespace nlohmann?

so for the sake of consistancy in bigger projects and also to avoid name collisions with other libraries in package managers, it might be better to atleast for cmake install to expose the the include to nlohmann/json.jpp and users of package managers could easaly disdinguish between nlohmann_json, jsoncpp, and so on.

The cause of my request is, that I wanted to update the cmake/hunter package which was still on V1.0.0rc. and integrating nlohman_json with the current CMakeLists.txt would cause an inconsistency with the package name, the include and the namespace the user has to see.

@nlohmann
Copy link
Owner

@dan-42 The Cmake files were mostly created by contributors. I have little knowledge of Cmake and the only value it has for me as maintainer is to be able to use AppVeyor. :)

That said, I unfortunately do not understand the issue you describe, and I would be happy for further information, or better a PR.

Out of curiosity: what kind of project you are using the library for?

@dan-42
Copy link
Contributor Author

dan-42 commented Jul 27, 2017

@nlohmann wow that was a quick answer :-)
My use case/problem is that the cmake package manager hunter
allowes user to write simply:

hunter_add_package(nlohmann_json)
find_package(nlohmann_json CONFIG REQUIRED)
add_executable(main main.cpp)
target_link_libraries(main nlohmann_json)

And In the background the library is downloaded, compiled if needed and added to the include and librarypath. Also very easy to use and only dependancy is cmake, which is already there.

an then user uses it

#include <json.hpp>
int main (int argc, char** argv) {
  //using it here within the namepace nlohmann
nlohmann::json;
}

So but in the example nlohmann is inconsistent with there names, with namespace, include name and cmake package name.

Also when I look into the install folder, cmake installes nlohman like this

Install the project...
-- Install configuration: ""
-- Up-to-date: /home/dan/Downloads/json211/json-2.1.1/install/include/nlohmann/json.hpp
-- Installing: /home/dan/Downloads/json211/json-2.1.1/install/cmake/nlohmann_jsonTargets.cmake
-- Up-to-date: /home/dan/Downloads/json211/json-2.1.1/install/cmake/nlohmann_jsonConfig.cmake
-- Up-to-date: /home/dan/Downloads/json211/json-2.1.1/install/cmake/nlohmann_jsonConfigVersion.cmake

notice the include/nlohmann/json.hpp
But in the source code we use #include <json.hpp> but I would expect #include <nlohmann/json.hpp>

I'm happy to provide a PR, but would first want to know if this is wanted by the maintainer :-)

a friend daminetreg wrote in his library an adapter to parse/generate json from any BOOST_FUSION_ADAPTED_STRUCT, and the it was used as a central engine for complex configuration files in embedded devices. And it was also used in a RESTful/Api for a webinterface as well.

@nlohmann
Copy link
Owner

If you would open a PR, it would also be nice to add 1-2 sentences about that hunter package manager (see https://github.com/nlohmann/json#integration).

@dan-42
Copy link
Contributor Author

dan-42 commented Jul 29, 2017

PR is created #669, I'll add some more information to https://github.com/nlohmann/json#integration,
when you are OK with my proposed changes and then hunter support nlohmann_json again with the latest stable release.

@nlohmann
Copy link
Owner

I merged #669. It would be great if you could add 1-2 sentences on the hunter support.

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Jul 30, 2017
@nlohmann nlohmann added this to the Release 3.0.0 milestone Jul 30, 2017
@dan-42
Copy link
Contributor Author

dan-42 commented Jul 31, 2017

see #671 for the hunter support.

@dan-42 dan-42 closed this as completed Jul 31, 2017
@nlohmann nlohmann self-assigned this Aug 1, 2017
@ringerc
Copy link

ringerc commented Jan 26, 2018

Note that this looks to introduce a compat break from somewhere between 2.0.2 and 2.1.1. Nothing dramatic, but you have to work around it if you want to support both forms. You'll need some kind of ifdef to pick between the headers based on configure/cmake/whatever detection.

Even Fedora 27 only has 2.0.2 so supporting the old path would make sense for most projects. (Unfortunately, it may not be trivial to support both headers in code either.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement/improvement kind: question solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

3 participants