jorgen / json_struct

json_struct is a single header only C++ library for parsing JSON directly to C++ structs and vice versa
Other
422 stars 57 forks source link

GNUInstallDirs is required for CMAKE_INSTALL_DATAROOTDIR #29

Closed retifrav closed 2 years ago

retifrav commented 2 years ago

You have commented out #include(GNUInstallDirs) in the root CMakeLists.txt, but it actually is required for CMAKE_INSTALL_DATAROOTDIR, otherwise it defaults to /, at least it does so in my builds (on Windows, GNU/Linux and Mac OS):

$ mkdir build && cd $_
$ cmake -DCMAKE_INSTALL_PREFIX="../install" -DJSON_STRUCT_OPT_BUILD_BENCHMARKS=0 -DJSON_STRUCT_OPT_BUILD_EXAMPLES=0 -DJSON_STRUCT_OPT_BUILD_TESTS=0 ..
$ cmake --build . --target install --config Release
-- Installing: D:/code/json-struct/install/lib/cmake/json_struct/json_structConfigVersion.cmake
-- Installing: D:/code/json-struct/install/lib/cmake/json_struct/json_structConfig.cmake
-- Installing: D:/code/json-struct/install/./include
-- Installing: D:/code/json-struct/install/./include/json_struct.h
-- Installing: D:/code/json-struct/install/./include/json_struct_diff.h
-- Installing: /json_struct/package.xml

And if I uncomment include(GNUInstallDirs), then CMAKE_INSTALL_DATAROOTDIR gets a proper value and installation goes correctly:

...
-- Installing: D:/code/json-struct/install/share/json_struct/package.xml

So, I am not sure why you commented it out, maybe there was a good reason, but I decided to let you know about this, just in case.

While we are at it, perhaps you'd want to do something with . destination for installing include. It certainly works as it is, but doesn't look entirely correct (D:/code/json-struct/install/./include/json_struct.h).

jorgen commented 2 years ago

Thank you. I'm a bit unavailable for a few days, but will fix this once I'm home.

jorgen commented 2 years ago

It was @jrmelsha that introduced this, I don't know why it was commented out, but I have removed the # in the 667a78fbd76c4dedc1fe0bc6c83ea00c3697dbe3. I also fixed the . in the install rule, but I moved json_struct.h into a sub folder so its now under D:/code/json-struct/install/include/json_struct/json_struct.h

I have updated all the tests and examples to use '#include <json_struct/json_struct.h>' This is "less intrusive" I think, and maybe in the future I will split the json_struct.h file into several files.

jrmelsha commented 2 years ago

Good to uncomment that! I need to do a PR from my fork when I get a chance.. Sorry about that!

retifrav commented 2 years ago

moved json_struct.h into a sub folder so its now under D:/code/json-struct/install/include/json_struct/json_struct.h

I think this is a good idea, I prefer this approach too.