ArthurSonzogni / nlohmann_json_cmake_fetchcontent

A lightweight Release-tracking repository for nlohmann/json. Suitable for CMake fetchcontent. Automatically upgraded every weeks.
MIT License
79 stars 25 forks source link

Last update is broken if using MSVC #11

Closed NicolasIRAGNE closed 2 years ago

NicolasIRAGNE commented 2 years ago

Linking to json gives the following error:

[cmake] CMake Error at CMakeLists.txt:299 (add_library):
[cmake]   Cannot find source file:
[cmake] 
[cmake]     C:/Users/nicor/source/repos/TestProject/build/_deps/json-src/nlohmann_json.natvis
[cmake] 
[cmake] 

This is due to this bit from the upstream CMakeLists.txt:

## add debug view definition file for msvc (natvis)
if (MSVC)
    set(NLOHMANN_ADD_NATVIS TRUE)
    set(NLOHMANN_NATVIS_FILE "nlohmann_json.natvis")
    target_sources(
        ${NLOHMANN_JSON_TARGET_NAME}
        INTERFACE
            $<INSTALL_INTERFACE:${NLOHMANN_NATVIS_FILE}>
            $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/${NLOHMANN_NATVIS_FILE}>
    )
endif()

The INTERFACE keyword causes the file to be added to the sources of any target that links to nlohmann_json::nlohmann_json.

ArthurSonzogni commented 2 years ago

Thank you!

I rewritten the master branch history (keep the old in old-branch-3) and let the bot reland v3.10.5 after your patch. Could you please check if this is working on Windows?

NicolasIRAGNE commented 2 years ago

This makes me reconsider the decision of using the upstream CMakeLists with the current update.py script.

What if we took this problem the other way around? Instead of copying the files we think are relevant, how about just deleting the irrelevant ones that are responsible for the size?

image

As we see here, the 3 directories responsible for the size are irrelevant for this repo and could simply be removed while keeping everything else. This would allow compatibility for most options and reduce the risk of something breaking on upstream update. @ArthurSonzogni what do you think?

ArthurSonzogni commented 2 years ago

@ArthurSonzogni what do you think?

The nice thing with the previous update.py where we controlled the CMakeLists.txt was that it was fully autonomous with no risk of being broken by upstream. Now, we might be broken, but it should be rare enough.

Your idea sounds like a good one probably. Maybe we should wait a bit with the current solution to see how long it can last and try it the next time this is broken?

NicolasIRAGNE commented 2 years ago

Thank you!

I rewritten the master branch history (keep the old in old-branch-3) and let the bot reland v3.10.5 after your patch. Could you please check if this is working on Windows?

Looks good!

ZADNE commented 1 year ago

This issue is back in master. It was delivered in v3.10.5 to be specific.

ChristianReese commented 1 year ago

This issue is back in master. It was delivered in v3.10.5 to be specific.

I second this - still observing these natvis errors when using 3.10.5 or above.

NicolasIRAGNE commented 1 year ago

FYI I think this repo is not as useful anymore because a similar mechanism is now officially supported by the original repo.

From the Readme:

Embedded (FetchContent)

Since CMake v3.11, FetchContent can be used to automatically download a release as a dependency at configure time. Example:

include(FetchContent)
FetchContent_Declare(json URL https://github.com/nlohmann/json/releases/download/v3.11.2/json.tar.xz)
FetchContent_MakeAvailable(json)
target_link_libraries(foo PRIVATE nlohmann_json::nlohmann_json)

Note: It is recommended to use the URL approach described above which is supported as of version 3.10.0. See https://json.nlohmann.me/integration/cmake/#fetchcontent for more information.