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

Out of tree builds and a few other miscellaneous CMake cleanups. #242

Merged
merged 7 commits into from
May 11, 2016
Merged

Out of tree builds and a few other miscellaneous CMake cleanups. #242

merged 7 commits into from
May 11, 2016

Conversation

ChrisKitching
Copy link
Contributor

Addresses the stuff I raised in #241. Hopefully this isn't too invasive for you...

I'll attempt to find some time to do more cleanup in the coming days.

I'm not sure that using a variable for target names really helps
with clarity. Unlike paths, target names aren't really something
you change. In a sense, targets are themselves a sort of variable,
so having a variable to name a variable seems just a bit gnarly.
There exist lots of json libraries, and project/target names must
be globally unique. If someone integrated with this library in a
particularly stupid way, using a generic name like "json" might
cause a problem.
This introduces a clear separation between test data and test
binaries. Test data is moved into test/data, and the test binaries
move into test/src. A new CMake script specific to building the
tests is introduced in /test to slightly clean up the toplevel
one.

As well as tidying things up, this makes the next step trivial...
The resulting install tree, when tests are enabled, looks like this:

```
.
├── cmake
│   ├── nlohmann_jsonConfig.cmake
│   ├── nlohmann_jsonConfigVersion.cmake
│   └── nlohmann_jsonTargets.cmake
├── include
│   └── nlohmann
│       └── json.hpp
└── test
    ├── bin
    │   └── json_unit
    └── data
        ├── json_nlohmann_tests
        │   ├── all_unicode.json
        │   └── bom.json
        ├── json.org
        │   ├── 1.json
        │   ├── ...
        ├── json_roundtrip
        │   ├── roundtrip01.json
        │   ├── roundtrip02.json
        │   └── ...
        ├── json_tests
        │   ├── fail10.json
        │   └── ...
        └── json_testsuite
            └── sample.json
```

It has the property that you can invoke the test binary from the
root of the install tree and the tests work correctly (you no
longer depend on the test binary being run inside the source
tree).

If tests are disabled, the entire `test/` subtree is omitted.
Notice how that yields exactly what you want for using this
library in other projects.

I do not believe I need to update travis due to this change, as the
evil Makefile continues to do in-tree builds. I expect I'll find
out soon enough.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.954% when pulling 527a69b on ChrisKitching:outOfTree into 9ecf83f on nlohmann:develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.954% when pulling 7a58a48 on ChrisKitching:outOfTree into 9ecf83f on nlohmann:develop.

@nlohmann
Copy link
Owner

Hi @ChrisKitching, could you please check why the AppVeyor build is broken?

@nlohmann
Copy link
Owner

(Apart from this, I have not checked the details of the changes)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.954% when pulling 59c484d on ChrisKitching:outOfTree into 9ecf83f on nlohmann:develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.954% when pulling 890ce59 on ChrisKitching:outOfTree into 9ecf83f on nlohmann:develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.954% when pulling f7ead65 on ChrisKitching:outOfTree into 9ecf83f on nlohmann:develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.954% when pulling dd5e8a3 on ChrisKitching:outOfTree into 9ecf83f on nlohmann:develop.

It's sort of gnarly that it's still doing in-tree builds, but I
really, _really_ don't want to get any more friendly with CMake's
Visual Studio generator to work out how to make it stop doing it.

In-tree builds still work, after all, and the goal of this work is
to make out-of-tree builds work as well. Notional horrors like
this will have to wait ;)
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.954% when pulling 1287948 on ChrisKitching:outOfTree into 9ecf83f on nlohmann:develop.

@ChrisKitching
Copy link
Contributor Author

There we go. Builds on Windows are officially my least favourite thing.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.954% when pulling 0e2f0c4 on ChrisKitching:outOfTree into 9ecf83f on nlohmann:develop.

@nlohmann nlohmann merged commit 888d022 into nlohmann:develop May 11, 2016
@nlohmann
Copy link
Owner

Thanks!

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.

3 participants