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

Make nlohmann friendly for inclusion. #1266

Closed
wants to merge 1 commit into from
Closed

Make nlohmann friendly for inclusion. #1266

wants to merge 1 commit into from

Conversation

Lord-Kamina
Copy link
Contributor

[Describe your pull request here. Please read the text below the line, and make sure you follow the checklist.]

New PR for #1250

The one change I haven't tested is the natvis bit because I don't have an MSVC system on hand. Other than that, I tried installing regularly, running the tests and including my way and it worked as expected.

Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

Please don't

  • The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.7 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
  • Specifically, I am aware of compilation problems with Microsoft Visual Studio (there even is an issue label for these kind of bugs). I understand that even in 2016, complete C++11 support isn't there yet. But please also understand that I do not want to drop features or uglify the code just to make Microsoft's sub-standard compiler happy. The past has shown that there are ways to express the functionality such that the code compiles with the most recent MSVC - unfortunately, this is not the main objective of the project.
  • Please refrain from proposing changes that would break JSON conformance. If you propose a conformant extension of JSON to be supported by the library, please motivate this extension.
  • Please do not open pull requests that address multiple issues.

@chuckatkins
Copy link
Contributor

chuckatkins commented Sep 30, 2018

I'm a bit confused as to the problem this is trying to solve. When would PROJECT_SOURCE_DIR not be the same as CMAKE_CURRENT_LIST_DIR in the top level CMake? The usual way to add a third party CMake library to another CMake project is via add_subdirectory, not include(), although it shouldn't matter too much which way you do it.

We (at kitware) are currently using this as a third party library via find_package for external or add_subdirectory for internal, without issue. So what doesn't work that this is trying to fix? It wasn't really clear what doesn't work since you should be abel to do the following as is without changing anything:

CMakeLists.txt

...
add_subdirectory(3rdParty)
...
add_subdirectory(src)
...

3rdParty/CMakeLists.txt

...
find_package(nlohmann_json 3.2.0 QUIET)
if(nlohmann_json_FOUND)
  message(STATUS "Nlohmann JSON found externally.")
else()
  message(STATUS "Nlohmann JSON not found externally; will use included version.")
  add_subdirectory(json)
endif()

3rdParty/json/CMakeLists.txt

# Set any project specific options for nlohmann_json
set(BUILD_TESTING OFF)
add_subdirectory(nlohmann_json)

src/CMakeLists.txt

...
# The nlohmann_json::nlohmann_json target will always be there since via find_package
# the target is already namespaced and if built internally then the
# nlohmann_json::nlohmann_json target is added as an alias to nlohmann_json.
target_link_libraries(performous PRIVATE nlohmann_json::nlohmann_json)
...

3rdParty/json/nlohmann_json is then the source for this project as a git submodule.

This should work as a third party library infrastructure in general with each 3rdParty/foo containing a CMakeLists.txt that sets the foo specific build options and adds the sub-directory for 3rdParty/foo/libfoo which is the project's actual code.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4b1f2c6 on performous:performous into 910a895 on nlohmann:develop.

@Lord-Kamina
Copy link
Contributor Author

@chuckatkins what it's trying to solve is it not working via include(), which is a valid choice AFAIK.

Either way, the way I see it... it's perhaps an unusual way to do things but it's a couple lines of code that doesn't affect "normal" usage and makes the Cmake script more robust.

@chuckatkins
Copy link
Contributor

chuckatkins commented Oct 1, 2018

it's perhaps an unusual way to do things

Using include to add another project carries a slew of implications with it that break other CMake code in the project. It pulls the included file in and processes it in the directory the include is called from rather than changing the working directory, so CMAKE_CURRENT_SOURCE_DIR and CMAKE_CURRENT_BINARY_DIR aren't valid in the included CMakeLists.txt file. Similarly, since the project() command is executed in the included file anything after the include has the PROJECT_* variables set to the value from the included file instead of the current project:

project(foo ...)
# PROJECT_<...> variables set correctly for the foo project
include(3rdParty/json/CMakeLists.txt)
# PROJECT_<...> variables now set to the values from nlohmann_json

There should probably be better documentation in CMake for some of these implications and to suggest not to add additional cmake projects in that way. This is somewhat conveyed in the docs for the project() command:

The top-level CMakeLists.txt file for a project must contain a literal, direct call to the project() command; loading one through the include() command is not sufficient.

Which essentially says that project() being called from an include file doesn't set up the project correctly; so in this case, the nlohman_json sub-project project won't get set up the way it needs to and CMake expects it to. Using add_subdirectory keeps things scoped accordingly.

@nlohmann
Copy link
Owner

nlohmann commented Oct 1, 2018

I have little experience with CMake and trust @chuckatkins in this regard. To my understanding, the PR would add a fix to support a discouraged way of including the library. If this is the case, then I'd rather not have the PR merged, especially as #1260 added a comment https://github.com/nlohmann/json#integration on how to use CMake for the library.

@nlohmann nlohmann closed this Oct 3, 2018
@Lord-Kamina Lord-Kamina deleted the performous branch May 6, 2020 07:58
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.

4 participants