Feature/upgrade nlohman json by dan-42 · Pull Request #907 · ruslo/hunter

dan-42 · 2017-07-26T17:23:25Z

Dear @ruslo
This PR pushes nlohmanUjson to V2.1.1, currently in hunter is V1.0.0
As they have improved there cmake, the usage breaks.
as target_link_libraries(main nlohmann-json::nlohmann-json) not needed any more
and find_package calls nlohman_json
also JSON_BuildTests changed to camel case.

so I allowed me to rename every thing to this new naming scheme and I have abonded two very old versions V1.0.0 as they also have only been release candidates

I'm looking forward with exitment for your comments on this, and if its to much "breaking" changes or not.

Thank you

ruslo · 2017-07-27T13:25:44Z


 add_executable(main main.cpp)
-
-target_link_libraries(main


Should we have target_link_libraries(main nlohmann_json::nlohmann_json) somewhere?

good catch, i checked and it is now target_link_libraries(main nlohmann_json)

ruslo · 2017-07-27T13:27:38Z

I'm looking forward with exitment for your comments on this, and if its to much "breaking" changes or not

Looks good, just mention in wiki how to migrate and what version has the last nlohmann-json::nlohmann-json imported target name.

ruslo · 2017-07-27T16:51:50Z

Testing:

dan-42 · 2017-07-27T16:52:26Z

Thx for the review.
I'll updated the wiki as well see, if its not shipped in 0.19.50 I'll change the numbers again :-)

ruslo · 2017-07-27T17:01:22Z

Everything seems to be failing:

/home/travis/build/ingenue/hunter/_testing/Hunter/_Base/e8d66e1/3bce796/20e478a/Build/nlohmann_json/Source/src/json.hpp:5670:36: error: no viable overloaded '='

        result.m_it.array_iterator = m_value.array->insert(

        ~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~

dan-42 · 2017-07-27T17:10:44Z

ohh no sry, my tests worked -,-
I'll look into it and fix it

dan-42 · 2017-07-27T17:46:27Z

Ok I had the old version somehow still in my path and I did not tested the example properly. Also the include changed it's now#include <json.hpp>

now runs on my system

dan@kay ..it/hunter/examples/nlohmann_json/build (git)-[feature/upgrade_nlohman_json] % rm -rf ./*
zsh: sure you want to delete all 6 files in /home/dan/git/hunter/examples/nlohmann_json/build/. [yn]? y
dan@kay ..it/hunter/examples/nlohmann_json/build (git)-[feature/upgrade_nlohman_json] % cmake ..
Including HunterGate: /home/dan/git/hunter/gate/cmake/HunterGate.cmake
-- The C compiler identification is GNU 7.1.1
-- The CXX compiler identification is GNU 7.1.1
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- [hunter] Calculating Config-SHA1
-- [hunter] Calculating Toolchain-SHA1
-- [hunter] HUNTER_ROOT: /home/dan/git/hunter
-- [hunter] [ Hunter-ID: xxxxxxx | Config-ID: b4f8ee8 | Toolchain-ID: 12f1fdb ]
-- [hunter] NLOHMANN_JSON_ROOT: /home/dan/git/hunter/_Base/xxxxxxx/b4f8ee8/12f1fdb/Install (ver.: 2.1.1)
-- Configuring done
-- Generating done
-- Build files have been written to: /home/dan/git/hunter/examples/nlohmann_json/build
dan@kay ..it/hunter/examples/nlohmann_json/build (git)-[feature/upgrade_nlohman_json] % make
Scanning dependencies of target main
[ 50%] Building CXX object CMakeFiles/main.dir/main.cpp.o
[100%] Linking CXX executable main
[100%] Built target main
dan@kay ..it/hunter/examples/nlohmann_json/build (git)-[feature/upgrade_nlohman_json] % ./main 
{"answer":{"everything":42},"happy":true,"list":[1,0,2],"name":"Niels","nothing":null,"object":{"currency":"USD","value":42.99},"pi":3.141}
dan@kay ..it/hunter/examples/nlohmann_json/build (git)-[feature/upgrade_nlohman_json] %

ruslo · 2017-07-27T17:56:04Z

Also the include changed it's now#include <json.hpp>

In this case package should be called json. Where this file installed? <root>/include/nlohmann/json.hpp?

Please squash everything into one commit.

dan-42 · 2017-07-27T18:10:29Z

Ok I can do that, but is that what you want?
the usage of it is still in the c++ namespace nlohmann::json
So another library with the same include name will collide then.
For example the jsoncpp pkg has already #include <json/json.h>

so as an alternative the include path could be altered by hunter? or would that be to intrusiv?

ruslo · 2017-07-27T18:23:33Z

so as an alternative the include path could be altered by hunter? or would that be to intrusiv?

It should be fixed in package itself.

dan-42 · 2017-07-27T18:59:20Z

Ok so for now we postpone this untill I checked it with the maintainers of nlohman_json.
@ruslo many thx for your time and advice.

v1bri · 2017-07-27T19:19:51Z

FYI: #include <nlohmann/json.hpp> works for me.

dan-42 · 2017-07-27T19:24:16Z

Checking with nlohmann about the include name issue here nlohmann/json#668 stay tuned for updates :-)

@v1bri nice I'm not allown by wanting the latest version of this in hunter

v1bri · 2017-07-27T19:44:31Z

@dan-42 Totally, I just started using this useful library myself.

What I meant by my comment is I believe nlohmann/json is installing headers to the appropriate location. Here's my system...

~/develop/hunter/_Base$ find . -name "json.hpp"
./xxxxxxx/a79a7a4/e1266bb/Install/include/nlohmann/json.hpp
./Cellar/425dd7929e5773a442ccdc96bf7f31860d91b80e/425dd79/raw/include/nlohmann/json.hpp

... So I'm wondering if your environment shows the same?

Also, the testing flag is BuildTests in the v2.1.1 release. It was renamed to JSON_BuildTests two weeks ago (nlohmann/json@cd80052) but hasn't been captured in a release yet.

dan-42 · 2017-07-27T20:14:34Z

@v1bri yes, I have seen/been hit by these changes as well.
But one issue is that the Include path by nlohmann_json's cmake is set directly to the json.hpp file.
which is IMHO not relly intended. as you can see in the issue quoted above, nlohmann itself does not yeallt use cmake. I'll first propose a PR for nlohmanns cmake, and the update this PR.
I'll work on this on the weekend. cheers

v1bri · 2017-07-27T22:33:31Z

But one issue is that the Include path by nlohmann_json's cmake is set directly to the json.hpp file.

Hmm I'm not sure where you're seeing this? The include destination folder is set to include/nlohmann and json.hpp is installed to that same folder.

This path is also set in the library's include INTERFACE property. I'm not sure any change is required in the nlohmann/json project. My builds went just fine with #include <nlohmann/json.hpp> and pulling in v2.1.1.

Can you elaborate on what you think needs to change in that project?

ruslo · 2017-07-27T22:53:14Z

Hmm I'm not sure where you're seeing this? The include destination folder is set to include/nlohmann and json.hpp is installed to that same folder.

JSON_INCLUDE_DESTINATION used in two places.

It is correct in install(FILES ...):

https://github.com/nlohmann/json/blob/v2.1.1/CMakeLists.txt#L50-L51

But it's not correct to use it in $<INSTALL_INTERFACE:...> (it should be just include):

https://github.com/nlohmann/json/blob/v2.1.1/CMakeLists.txt#L25

Also instead of INSTALL_INTERFACE you can achieve the same by INCLUDES DESTINATION:

install(
    TARGETS foo
    EXPORT "${targets_export_name}"
    INCLUDES DESTINATION "include"
)

v1bri · 2017-07-27T23:29:31Z

Thanks for the follow-up. CMake generator expressions are still a little opaque to me.

ruslo · 2017-07-30T19:47:31Z

I've released last updates in fork:

9cc93ef

Hunter release:

https://github.com/ruslo/hunter/releases/tag/v0.19.52

dan-42 added 2 commits July 26, 2017 18:46

FEATURE: add support for nlohman_json V2.1.1

f77f005

REFACTOR: adapt new naming scheme with underscore

f07f3f4

ruslo suggested changes Jul 27, 2017

View reviewed changes

FIX nlohmann_json example, missing target_link_libraries

9a8904d

ruslo approved these changes Jul 27, 2017

View reviewed changes

FIX example nlohman_json include changed with V2.1.1

19f13b0

dan-42 mentioned this pull request Jul 29, 2017

REFACTOR: rewrite CMakeLists.txt for better inlcude and reuse nlohmann/json#669

Merged

ruslo closed this Jul 30, 2017

Conversation

dan-42 commented Jul 26, 2017

Uh oh!

ruslo Jul 27, 2017

Choose a reason for hiding this comment

Uh oh!

dan-42 Jul 27, 2017

Choose a reason for hiding this comment

Uh oh!

ruslo commented Jul 27, 2017

Uh oh!

ruslo commented Jul 27, 2017

Uh oh!

dan-42 commented Jul 27, 2017

Uh oh!

ruslo commented Jul 27, 2017

Uh oh!

dan-42 commented Jul 27, 2017

Uh oh!

dan-42 commented Jul 27, 2017

Uh oh!

ruslo commented Jul 27, 2017

Uh oh!

dan-42 commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ruslo commented Jul 27, 2017

Uh oh!

dan-42 commented Jul 27, 2017

Uh oh!

v1bri commented Jul 27, 2017

Uh oh!

dan-42 commented Jul 27, 2017

Uh oh!

v1bri commented Jul 27, 2017

Uh oh!

dan-42 commented Jul 27, 2017

Uh oh!

v1bri commented Jul 27, 2017

Uh oh!

ruslo commented Jul 27, 2017

Uh oh!

v1bri commented Jul 27, 2017

Uh oh!

ruslo commented Jul 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dan-42 commented Jul 27, 2017 •

edited

Loading