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

Made nlohmann_json optional in the tests, exported c++14 requirement #102

Merged
merged 4 commits into from
Aug 25, 2018

Conversation

BeneSim
Copy link
Contributor

@BeneSim BeneSim commented Aug 12, 2018

Prior to this PR xtl did not properly export it's compile feature requirement cxx_std_14. This now propagates to other targets that link against xtl.

When building from testand not the root xtl directory cmake now properly tries to find the nlohmann_json package. In addition if it fails to find nlohmann_json it will not compile test_xoptional.cpp and not link against nlohmann_json.
Furthermore nlohmann_json recently adapt to the more modern namespaced targets nlohmann/json#1048 currently only on master branch. The new CMakeLists.txt will use the correct target i.e. nlohmann_json or nlohmann_json::nlohmann_json.

This also should fix #101 .

@wolfv
Copy link
Member

wolfv commented Aug 12, 2018

nice!

I think that we should #ifdef the tests that require json in test_xoptional because there is only one test that actually requires json.

What do you think?

@BeneSim
Copy link
Contributor Author

BeneSim commented Aug 12, 2018

Sure makes sense, haven't really looked into the test, but will do now :)

@wolfv
Copy link
Member

wolfv commented Aug 12, 2018

Ok that's a little different than the ifdef idea I had in mind. I thought we could just set a compile time define with add_definitions(-DHAVE_NLOHMANN_JSON) and then check that with the regular cpp preprocessor.

@BeneSim
Copy link
Contributor Author

BeneSim commented Aug 12, 2018

I mean right now it uses the cpp preprocessor, we just don't need to pass the definition via command line because configure_file embeds this into the source file. I generally try to avoid adding new command line defines, but the more I think about it the more I like your approach. Your approach is superior when we have multiple tests that want to use the nlohmann_json library. I'll try to amend this PR as soon as possible, then we can decide what would be the best approach ;)

@SylvainCorlay
Copy link
Member

I also think that it would be better to use definitions instead of a .in file here. The reason is that some people may actually be building xtensor tests with other tools (after all, xtensor is header only, some like to vendor it ect).
.in files are preferable for cmake-specific things IMO.

Instead of doing add_definitions, one can use target_set_properties with COMPILE_DEFINITIONS on the test target.

@BeneSim
Copy link
Contributor Author

BeneSim commented Aug 13, 2018

@SylvainCorlay Or just target_compile_definitions ;) But I think we can simply use add_definitions else we would have to distinguish which test cases actually want to use nlohmann_json. I think it won't hurt the other targets if they have a NLOHMANN_JSON_FOUND definition or something similar defined.

I'll amend the PR when I'm back home :)

@SylvainCorlay
Copy link
Member

target_compile_definitions

living and learning :)

@BeneSim
Copy link
Contributor Author

BeneSim commented Aug 13, 2018

Alright added the definition ;)

@JohanMabille JohanMabille merged commit 3b46b4c into xtensor-stack:master Aug 25, 2018
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.

tests fail: cannot find -lnlohmann_json
4 participants