fix cmake install directory (for real this time)

[theodelrieu]

• Rename 'develop' folder to 'nlohmann'
• Use <nlohmann/*> headers in sources
• Hack test/CMakeLists.txt to find the correct json.hpp
• Change amalgamate config file

Should fix #928 and #942.

---

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 develop directory, run make amalgamate to create the single-header file src/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.8 (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.

[nlohmann]

Why is the renaming from develop to nlohmann required?

[theodelrieu]

Since the code includes <nlohmann/*.hpp>, there needs to be a nlohmann folder at build time and install time.

[nlohmann]

Sorry to ask so stupid questions... Why should the code include nlohmann/*.hpp?

[theodelrieu]

No worries :)

The relative include directory issue mentioned in #928 is fixed by using "absolute" includes.
Also, users that installed the project with CMake, (prior to PR #700), had to include <nlohmann/json.hpp>.

But now that the source code is split, the compiler doesn't know where to find files included by <nlohmann/json.hpp>.

So we have to correctly install files in include/nlohmann whether users are installing the amalgamated version or not. In addition to that, to support people who include our CMakeLists in their own project, we need to fix the include directories in the CMakeLists.txt itself.

Sorry if it's still confusing :/

[nlohmann]

Hm... I do not like the idea of changing the repository to make CMake happy. Is this possible without?

[theodelrieu]

It is not possible, it is forced by the includes and C++, not CMake.

To make it less confusing, we could (again) rename src to single_header.

And for users that are harcoding the src folder in their projects, simply removing it should work out of the box, if my changes are correct.

[nlohmann]

Hm. So far, the location of the header was always src/json.hpp. Why would we need to change anything now when the header is still at the same spot?

[theodelrieu]

It worked before by accident, because there was only one file. We were not including any other file, so include directories that we defined did not matter much.

With #700, we included split headers relative to the directory where json.hpp was.

However, users who installed the project by running cmake --target install will get a compiler error.

Install dir: /usr/local/include/
After cmake install: /usr/local/include/nlohmann/

User code:

#include <nlohmann/json.hpp>

User command line might look like:

$(CXX) -I/usr/local/include main.cpp

And they will get something like that:

fatal error: 'json_fwd.hpp' file not found
#include "json_fwd.hpp"

They could fix it by adding -I/usr/local/include/nlohmann, but having two include directories for a single library is quite brittle.

By including everything from the root folder (e.g. /usr/local/include in my example), there is no need for an additional -I flag, and more important there will be only one correct way to include the project (as explained in #942).

With that being said

There is one way to fix it without changing the current include method though:

// detail/meta.hpp

#include "../json_fwd.hpp"

which is awful, or by adding a detail/json_fwd.hpp which will then be included by json_fwd.hpp. The second way seems cleaner to me.

PS: I just thought about that after writing everything else...

The latter approach will fix #928, but the questions raised in #942 remain.
IMO it would be great to have a single way to include the project.

[nlohmann]

I also have to think about this in detail. I actually wanted to release 3.1.0 tomorrow...

[theodelrieu]

Ah, I think this is a mandatory issue for 3.1.0 :p

[coveralls]

Coverage remained the same at ?% when pulling 14cd019 on theodelrieu:fix/cmake_install into 9958dde on nlohmann:develop.

[nlohmann]

As far as I can see, this issue only occurs when the CMake flag -DJSON_MultipleHeaders=ON is used. This has not been officially released and we still can change its behavior. Renaming files in the repository could break existing systems, and I do not want to do this just to please CMake.

[nlohmann]

I have no idea how Catch2 is doing it, but there they have an include and a single_include directory. So there may be a way without renaming any directory to the name of the project.

[theodelrieu]

Catch2 only has two headers exposed: catch.hpp and catch_with_main.hpp, they might have chosen to avoid stuttering (e.g. catch/catch.hpp).

I thought about two possible solutions:

Each will prevent breakage for those who simply include "json.hpp", and will install the project with the following structure:

include
|__ json.hpp
\ _ nlohmann
     |__ json.hpp
     |__ detail/

The include/json.hpp only does #include <nlohmann/json.hpp>.

First one

It still needs the develop folder to be renamed in the project tree

That way, we can use #include <nlohmann/*> in the source code, and everything will work.

Second one

If renaming the develop folder to the project name is an issue (I don't see why honestly), we can continue to use ""-quoted includes, with the hack I mentioned earlier to solve #928, and it will also work in every case.

(Unless I missed something of course)

[nlohmann]

The issue is that I do not want to adjust the repository to people's assumptions about how they can use the code in their CMake projects. This reminds me of the discussion that the repository is too large for people to use it as git submodule. So far, everything is fine, and now I see that adjustment of the repo is required to support the JSON_MultipleHeaders feature. Did I miss anything?

[theodelrieu]

I'm not proposing adjustments for CMake projects that include this library. They should use variables/targets defined in the CMakeLists.txt, if they don't that's their problem to fix.

My main issue is about making the cmake install -DJSON_MultipleHeaders=ON work for everyone. That's what the two solutions I mentioned are about.

[nlohmann]

Alright... I had a look at some other repositories.

There seem to be three approaches:

• include (e.g., catch2)
• include/$library (e.g., include/rapidjson)
• $library (e.g., doctest)

I personally like the first approach include best. include/json or include/nlohmann is a bit of noise. json or nlohmann seems odd, because the folder name would not indicate the content.

The only "problem" left is what to do with the multiple-header version...

[nlohmann]

So would it be possible to rename src to include and leave develop as is?

[theodelrieu]

I'll go over all the arguments that have been discussed, in an attempt to be very clear.

Current state (3.0.1)

Since commit b1a2e9a by @robertmrk, there is a CMake install target. using it results in the following:

install_folder
|__ include
|     |__ nlohmann
|        |__ json.hpp
|
|__ lib
      |__ cmake
           |__ nlohmann_json
                |__ nlohmann_jsonConfig.cmake
                |__ nlohmann_jsonConfigVersion.cmake
                |__ nlohmann_jsonTargets.cmake

As you can see, json.hpp is installed inside the nlohmann directory.

Which means that anyone using this installation method must include it like this:

#include <nlohmann/json.hpp>

And add the following include directive: -I/path/to/install_folder/include.

There was no need to have a nlohmann directory in the project repository, since there was only json.hpp.

Develop branch state (future 3.1.0)

With PR #700, the source code is split in several headers, to ease development, and to permit users to only include files they need. The primary use-case being including json_fwd.hpp in header files.

There's two issues that arose with the split headers:

How should they include each other?

There is two ways, #include "detail/meta.hpp" or #include <topleveldir/detail/meta.hpp>.

The second one is much more refactoring and maintenance proof, because the first one treats paths relatively to the current directory.

That means that if detail/conversions/from_json.hpp has to include topleveldir/json_fwd.hpp (the same problem linked in #928), it has to do: `#include "../../json_fwd.hpp".

This is, in my opinion, atrocious. I once had a project like that, and spent two days replacing relative includes with #include <...> style. Refactoring was simply not possible, and you didn't know which files you were including in the end.

So the only way left is to use #include <topleveldir/json_fwd.hpp>.

However, we need to decide the name of topleveldir.

How should we name it then?

I think we don't have a choice, here's why:

Let's say we keep the develop folder but move it inside of a folder named include located at the project root. We then have to #include <develop/detail/value_t.hpp>, and add ${PROJECT_SOURCE_DIR}/include to the target_include_directories($<BUILD_INTERFACE>)

It will build, run, pass tests on Travis etc...

But, where do we install it? The CMake target used to install headers in the $INSTALL_DIR/include/nlohmann folder.

We don't want users to #include <nlohmann/develop/json_fwd.hpp>, that makes no sense.

If we simply put develop/* in nlohmann/*, it will also break because the source code will internally include #include <develop/detail/value_t.hpp>, and there is no develop folder anywhere.

Wrapping up

The only solution I see is to have an include/nlohmann folder (or simply nlohmann) in the project repository.

Frankly, I fail to understand how this can be an issue, the develop folder has not even been released yet.

About the src renaming to include, IMO it would be better to rename it single_include or single_header, to prevent future contributors to modify this file. This has the advantage to make it clear that this header is generated.

[nlohmann]

Thanks for the patience. I really appreciate this.

My problem is that I never fully understood CMake, and now I am "forced" to make changes to the repository to "please" CMake. Even worse (for me), it seems I have to do it to support the (to me) rare use case of someone (1) wanting to use the non-single headers and (2) wants does not want to change the include code for this.

I really need to sit down and better understand CMake until I am comfortable im making the required changes.

As for the naming, I wonder why Catch can work with single_include and include without catch subdirectory.

[przemkovv]

As I see in Catch2, all header files in internal/ never include anything from one-level up folder. However, the mutual includes between internal/*.hpp still exists with "xxx.hpp" naming. Therefore, even after moving everything to the include/catch, it will work.

In nlohmann/json, the headers in detail/ includes file from one-level up folder (json_fwd.hpp assuming that it is added to search path.

The workaround of adding include/nlohmann is also not very universal and can generate some conflicts in case, when an user's project will also have files named e.g. "detail/exceptions.hpp".
The compiler will have to dermine which one should be included (probably based on the order in search path).

[theodelrieu]

No problem :)

now I am "forced" to make changes to the repository to "please" CMake.

Please not that this is not related to CMake, the same problem would arise with Autotools, Meson or any existing build system.

It's compilers we're trying to make happy here.

(2) wants does not want to change the include code for this.

I think @przemkovv explained well why we certainly do not want to have two include directives (-I/path/to/install_dir/include and -I/path/to/install_dir/include/nlohmann).

Also, it's the library's responsability to ensure such compatibility between single and multi-header versions. Indeed, even people who just include json.hpp: #include <json.hpp> would get compiler errors.

For Catch2, they only have one level of header nesting, and don't include files from parent directories.

Note that installing Catch2 with CMake installs everything in a catch folder.

So you have to use #include <catch/catch.hpp> still. The only difference is that they don't need this top-level directory in their repository, since they're including things with #include "internal/file.hpp".

[nlohmann]

We don't want users to #include <nlohmann/develop/json_fwd.hpp>, that makes no sense.

So would moving json_fwd.hpp into the develop folder solve the issue?

[nlohmann]

So we could fix this by renaming:

• develop-> include/nlohmann
• src -> single_include/nlohmann?

[gregmarr]

About the src renaming to include, IMO it would be better to rename it single_include or single_header, to prevent future contributors to modify this file. This has the advantage to make it clear that this header is generated.

Would naming it generated make it even clearer, or would that make it hard to find? Maybe generated_include or combined_include?

[theodelrieu]

@gregmarr Sure, the clearer the better.

@nlohmann

So would moving json_fwd.hpp into the develop folder solve the issue?

It is already in the develop folder, my point was that I do not want to install a develop folder at all.

The fix is to rename:

• develop -> include/nlohmann
• src -> single_header (or a similar name)

I think there is no need to add a nlohmann folder in src/single_header, since the single header has no includes (except the C++ standard library ones).

Also, the single header has most likely been drag-and-dropped as-is in some projects (e.g. at my work, we currently use #include <json.hpp>, we didn't install the library with CMake).

[nlohmann]

I understand the idea of @gregmarr, but I think this is too conceptional.

A final question: if we do not need nlohmann in the src folder, can't we just leave it as is?

So:

develop -> include/nlohmann
src -> src

[theodelrieu]

It would work too, it's just a matter of preference and clarity. It seems weird to me that a generated file is located in a src folder.

But this could easily be changed later on if needed.

[nlohmann]

Good argument!

So let's do this:

develop -> include/nlohmann
src -> single_include

[theodelrieu]

Great, I'll rebase and push tomorrow morning :)

[nlohmann]

Ok. Then I can prepare the release for tomorrow.

[patrikhuber]

Great conversation and agree with everything that has been said, I'm very happy this fix gets in.
Just one question: Is there any reason not to rename the src folder so it is in line with what will be in the include folder? (E.g. to single_header/nlohmann/json.hpp). Meaning: After your proposed changes, you will have people that include the library with <nlohmann/json.hpp> (from the old "develop" folder) and people that include it with <json.hpp> (the single header version). Why this discrepancy? It is first a bit confusing and inconsistent, second, it doesn't allow you to swap between the single-header and "normal" version without changing all the includes, and third, unnecessary. Fourth, I still think json.hpp is quite a generic name so it's not unlikely it could result in name clashes, while nlohmann/json.hpp makes it very clear what json library one is actually including. Just json.hpp could be anything.

[nlohmann]

@patrikhuber I agree. src -> single_header/nlohmann for the sake of consistency.

[theodelrieu]

It's not really necessary since we can still install single_header/json.hpp in a nlohmann directory.

But this might prevent drag and drop the json.hpp file, and encourage users to copy the enclosing nlohmann folder as well.

I'll push in a few minutes.

[theodelrieu]

Here it is, I have installed with/without multiple headers.

I've patched test files to include <nlohmann/json.hpp> as well.

I haven't looked at the benchmarks folder, there might be some changes to make here as well. I don't know if this is mandatory for the release though.

[patrikhuber]

It's not really necessary since we can still install single_header/json.hpp in a nlohmann directory.

But it prevents using the single header from a submodule in a consistent way with #include <nlohmann/json.hpp>. I mean - maybe that's not important - users who use the library from a submodule might as well use the non-single-header version under <nlohmann/json.hpp>.
I'll leave that up to you guys :-)

[theodelrieu]

I finally went with your suggestion, it makes everything easier, even in the CMake :)

[pboettch]

My 2 cents: I'm using json.hpp in my project (and I'd like to use json_fwd.h). I use git-submodules, but not for this library. For this library I'm downloading the .hpp from the release-site.

I wonder how many users are doing like I do, compared to users who do make install having this project as a submodule (or anywhere else) and are thus concerned about the forced directory-hierarchy - except those who develop this library. Sorry for my ignorance in advance ;-)

[nlohmann]

@pboettch So what do you propose?

[pboettch]

What I tried to say is that maybe changing the paths inside this repo (and thus maybe breaking compatibility) is not a big deal, because only a few people are using it directly as compared to the multiple 10k (!) non-contributing/silent users who just use json.hpp as a copy. But this assumption might be completely wrong, I have no data.

In the intro-animation I read "What if JSON would be part of modern C++?" I hear would be part of standard C++ libraries. I'd expect to be able to do:

#include <json_fwd>

in header-files and in source-files

#include <json>

(similarly to iostream and iosfwd)

and this is exactly what I'm doing in my project(s) (*), I place the library's files so that exactly that works and I'm adding .hpp in my include statements.

(*) - I'm not yet using json_fwd.

[theodelrieu]

I'm not sure following the C++ standard include model is a good idea.

The main point in having a top-level directory to include is to avoid clashes. This library is not part of the standard, we cannot assume that json.hpp is reserved to us.

[pboettch]

@theodelrieu I see your point. A more generic approach then would be to prefix with nlohmann/. Then it would be just logic to have the same hierarchy of paths inside this repo and after a make install.

And that was exactly my original motivation when entering this discussion. I think there is no problem with moving files inside the repo to be coherent.

[nlohmann]

Looks good to me.