Including nlohmann the badwrong way.

[Lord-Kamina]

I want to use your library in a game. I've added proper code to find and link to it, but I also want to allow for the possibility somebody might want to compile the game and not know how to (or even just not want to) find/install nlohmnann.

So... instead of just dropping an include_directories I decided I'd try embedding your library as a submodule and just including your CMakeLists if a config module cannot be found. I am not entirely sure what nasty repercusions this could have but I figured it was better since it's still using your logic to define the target.

The only problem is, in doing this I ran into a million errors due to includes or subdirectories not existing (being called from your CMakeLists). I tracked this down to CMake populating essentially all variables (and also using it when not otherwise explicitly specified, such as in

add_subdirectory(test)

with my project folder instead of yours. I then realized I could use CMAKE_CURRENT_LIST_DIR instead of CMAKE_CURRENT_SOURCE_DIR and PROJECT_SOURCE_DIR and it will work fine.

Since I'd rather not modify your files (Ideally I'd like to be able to easily update my included version from your releases)... do you think it might be possible to support what I'm doing by comparing CMAKE_CURRENT_LIST_DIR and CMAKE_CURRENT_SOURCE_DIR and using the former if they're not equal?

[nlohmann]

I’m not a CMake expert, so I hope someone else can evaluate this.

[theodelrieu]

I would rather use the nlohmann_jsonConfig.cmake file generated during installation.

You could have a thirdparty/nlohmann folder (containing the installed library), then in your CMakeLists.txt:

set(CMAKE_PREFIX_PATH "${PROJECT_SOURCE_DIR}/thirdparty/nlohmann;${CMAKE_PREFIX_PATH})

find_package(nlohmann_json REQUIRED)

add_executable(my_game main.cpp)
target_link_libraries(my_game nlohmann_json::nlohmann_json)

[Lord-Kamina]

@theodelrieu you mean instead of cloning the repo, installing it to a subfolder in my project? That might work but it'd completely break the submodule logic, wouldn't it?

[theodelrieu]

True, I guess you could use ExternalProject first, then install nlohmann_json and use it via find_package.

[Lord-Kamina]

So... here's what I actually am doing:

find_package(nlohmann_json 3.2.0 QUIET)

if(nlohmann_json_FOUND)
	message(STATUS "Nlohmann JSON Found.")
else()
	message(STATUS "Nlohmann JSON not found; will use included version.")
	include("${CMAKE_SOURCE_DIR}/3rdparty/json/CMakeLists.txt")
endif()

if(nlohmann_json_FOUND)
	target_link_libraries(performous PRIVATE nlohmann_json::nlohmann_json)
	set_target_properties(nlohmann_json::nlohmann_json PROPERTIES INTERFACE_COMPILE_FEATURES ${COMPUTED_CXX_STD})
else()
	target_link_libraries(performous nlohmann_json)
	set_target_properties(nlohmann_json PROPERTIES INTERFACE_COMPILE_FEATURES ${COMPUTED_CXX_STD})
endif()

And it works, but I had to make the following changes to the nlohmann's CmakeLists:

--- CMakeLists.txt	2018-08-18 13:46:15.000000000 -0300
+++ CMakeLists_modd.txt	2018-09-20 11:09:33.000000000 -0300
@@ -26,17 +26,17 @@
   CACHE INTERNAL "")
 set(NLOHMANN_JSON_INCLUDE_INSTALL_DIR       "include")
 set(NLOHMANN_JSON_TARGETS_EXPORT_NAME       "${PROJECT_NAME}Targets")
-set(NLOHMANN_JSON_CMAKE_CONFIG_TEMPLATE     "cmake/config.cmake.in")
+set(NLOHMANN_JSON_CMAKE_CONFIG_TEMPLATE    "${CMAKE_CURRENT_LIST_DIR}/cmake/config.cmake.in")
 set(NLOHMANN_JSON_CMAKE_CONFIG_DIR          "${CMAKE_CURRENT_BINARY_DIR}")
 set(NLOHMANN_JSON_CMAKE_VERSION_CONFIG_FILE "${NLOHMANN_JSON_CMAKE_CONFIG_DIR}/${PROJECT_NAME}ConfigVersion.cmake")
 set(NLOHMANN_JSON_CMAKE_PROJECT_CONFIG_FILE "${NLOHMANN_JSON_CMAKE_CONFIG_DIR}/${PROJECT_NAME}Config.cmake")
 set(NLOHMANN_JSON_CMAKE_PROJECT_TARGETS_FILE "${NLOHMANN_JSON_CMAKE_CONFIG_DIR}/${PROJECT_NAME}Targets.cmake")
 
 if (JSON_MultipleHeaders)
-    set(NLOHMANN_JSON_INCLUDE_BUILD_DIR "${PROJECT_SOURCE_DIR}/include/")
+    set(NLOHMANN_JSON_INCLUDE_BUILD_DIR "${CMAKE_CURRENT_LIST_DIR}/include/")
     message(STATUS "Using the multi-header code from ${NLOHMANN_JSON_INCLUDE_BUILD_DIR}")
 else()
-    set(NLOHMANN_JSON_INCLUDE_BUILD_DIR "${PROJECT_SOURCE_DIR}/single_include/")
+    set(NLOHMANN_JSON_INCLUDE_BUILD_DIR "${CMAKE_CURRENT_LIST_DIR}/single_include/")
     message(STATUS "Using the single-header code from ${NLOHMANN_JSON_INCLUDE_BUILD_DIR}")
 endif()
 
@@ -63,7 +63,7 @@
         ${NLOHMANN_JSON_TARGET_NAME} 
         INTERFACE 
             $<INSTALL_INTERFACE:${NLOHMANN_NATVIS_FILE}>
-            $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/${NLOHMANN_NATVIS_FILE}>  
+            $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/${NLOHMANN_NATVIS_FILE}>  
     )
 endif()
 
@@ -75,7 +75,7 @@
 
 if(BUILD_TESTING AND JSON_BuildTests)
     enable_testing()
-    add_subdirectory(test)
+    add_subdirectory(${CMAKE_CURRENT_LIST_DIR}/test ${CMAKE_CURRENT_BINARY_DIR}/json-tests)
 endif()
 
 ##

Now, from what I understand, the only scenario where CMAKE_CURRENT_SOURCE_DIR is different from CMAKE_CURRENT_LIST_DIR is when people do what I just did. So, you might be able to support it by specifically checking for that and only doing things differently when this is detected.

[OvermindDL1]

If you want, you can use hunter.sh to automatically bring it in as it handles all that for you (you copy in the HunterGate.cmake file into your project, include it, HunterGate(...) the package repository you want, like the default one, then just hunter_add_package(nlohmann_json)' 'find_package(nlohmann_json CONFIG REQUIRED)' 'add_executable(main main.cpp)' 'target_link_libraries(main PUBLIC nlohmann_json::nlohmann_json) as normal).... It doesn't keep up with master, but a PR can update it if needed (or just ask in an issue).

[Lord-Kamina]

@OvermindDL1 I was looking into that yesterday and it sounded like it might be a solution but then I remembered mxe (the way we endorse for building windows executables) explicitly disallows packages to download things when building.

[OvermindDL1]

@Lord-Kamina It can use a local cache as well that you can copy around and/or include. Hunter is extremely flexible (ask in their issue tracker for any questions, hunter does have a few advanced options that aren't documented in the docs but probably should be...).

A quick look at mxe (that was a dangerous google search I discovered, they need a better name...) doesn't show that it would have any issues with cmake downloading things though? (I'm also not really seeing the point of it, everything listed on its feature list is fairly native to cmake either via directly or via it's build system output, cross-compile and all...)

[Lord-Kamina]

@OvermindDL1 in their "Creating Packages" section, it says:

Things not to do:

do not run target executables with Wine, as Wine is not guaranteed to be installed. Instead build the needed tool natively or (if it is too huge to build one more time) add to MXE's dependencies. This policy is forced by setting WINEPREFIX to an empty directory, which breaks Wine;
do not download anything while building, as all files downloaded should be verified by checksums. Instead create a package which installs the needed file. This policy is forced on Linux by LD_PRELOAD trick, breaking network functions.

I have gone into the hunter.sh issue tracker and have been asking about this but it seems they don't even really grasp what I'm trying to do, and it seems I'll most likely have to rely on downloading something at some point if I got with hunter.

The point of mxe is that it provides a simple-to-use package manager that, when properly configured, allows us to produce a working windows installer by essentially running a single command, and it works perfectly for me on mac and for the other devs who use Linux.

[OvermindDL1]

I have gone into the hunter.sh issue tracker and have been asking about this but it seems they don't even really grasp what I'm trying to do, and it seems I'll most likely have to rely on downloading something at some point if I got with hunter.

That's a unique issue! I'm quite curious to read about it, do you have their issue #/url?

Honestly though, I don't see the point in 'not' using wine as you could easily have a docker container around it or something (or just use a system installed version).

[Lord-Kamina]

That's a unique issue! I'm quite curious to read about it, do you have their issue #/url?

https://github.com/ruslo/hunter/issues/1557

[OvermindDL1]

Ah I'm not seeing wine being listed there, just the question about how to not download dependencies (I.E. just include them into the local cache). I'm confused precisely what you are asking about there beyond that as well like ruslo. You can very easily use a pre-existing package by just testing if it's found before calling hunter's acquiring thing as well if that is what was being asked?

[Lord-Kamina]

What I'm asking there is whether it's possible to configure and use hunter such that everything it might need could be included as a submodule in my project, in order to make sure it doesn't download anything at all.

The wine thing I pasted was from mxe, not hunter.

[OvermindDL1]

What I'm asking there is whether it's possible to configure and use hunter such that everything it might need could be included as a submodule in my project, in order to make sure it doesn't download anything at all.

Oh, to use a fully embedded hunter and its cache 'as' a submodule! You should post that in that thread, much more clear. :-)

[nlohmann]

Is there anything to be changed? Can this issue be closed?

[Lord-Kamina]

I'll submit a proposed patch later, I didn't right away because there's not much point if you're not willing to add it.

[Lord-Kamina]

@nlohmann I have refined the changes I had posted previously in the PR above. That PR would allow me (and anybody else who wanted to do so) to include your json library as a submodule using the Cmake include() directive. Somebody correct me if I'm wrong but I think this is better than doing add_subdirectory() because it should process all the relevant sections of your CMakeLists (including target names and such) in addition to just adding the sources.

[Quincunx271]

include() is for including cmake module files, which a CMakeLists.txt is not. Use add_subdirectory().

I looked at the project's CMakeLists.txt, and I don't see anything which is unfriendly to add_subdirectory(). A quick test showed that it indeed works to include nlohmann_json as a subdirectory, no changes needed.

You may want to turn off BUILD_TESTING when you add the subdirectory, though, unless you want to build all of the tests:

project(MyProject)

# ...

set(OLD_BUILD_TESTING "${BUILD_TESTING}")
set(BUILD_TESTING FALSE)
add_subdirectory(external/nlohmann_json)
set(BUILD_TESTING "${OLD_BUILD_TESTING}")

# ...

add_executable(my_executable ...)
target_link_libraries(my_executable PRIVATE nlohmann_json::nlohmann_json)

[Lord-Kamina]

I see. Will try that then.on an unrelated note, I haven't committed it yet but I'm making (it's done, really) a portfile of nlohmann for macports.

…

-- Gregorio Litenstein Goldzweig [image: glit_ind.png] Médico Cirujano - Fono: +56 9 96343643 <+56%209%2096343643> - E-Mail: [email protected] From: Justin Bassett <[email protected]> <[email protected]> Reply: nlohmann/json <[email protected]> <[email protected]> Date: October 1, 2018 at 14:44:20 To: nlohmann/json <[email protected]> <[email protected]> CC: Gregorio Litenstein <[email protected]> <[email protected]>, Mention <[email protected]> <[email protected]> Subject: Re: [nlohmann/json] Including nlohmann the badwrong way. (#1250) include() is for including cmake module files, which a CMakeLists.txt is

not. Use add_subdirectory(). I looked at the project's CMakeLists.txt, and I don't see anything which is unfriendly to add_subdirectory(). A quick test showed that it indeed works to include nlohmann_json as a subdirectory, no changes needed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1250 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGJfLN9vSd3MpbWDzrHv4A9XURu69jumks5uglR0gaJpZM4WxZEw> .

[nlohmann]

@Lord-Kamina Thanks for working on a portfile for MacPorts! Once it's done, it would be great to mention it in the README:

As for this issue and PR #1266, I think we can close both, right?