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

Use the canonical #include location for json.hpp #55

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

cebtenzzre
Copy link
Contributor

@cebtenzzre cebtenzzre commented Feb 18, 2025

The two simplest ways to get nlohmann/json are to install it system-wide, or to vendor it via CMake.

System

If you install it system-wide, it will be at /usr/include/nlohmann/json.hpp (on Arch Linux, at least). minja is not compatible with this out-of-the-box, requiring you to add the non-standard #include path of -I/usr/include/nlohmann.

This is confirmed by /usr/share/pkgconfig/nlohmann_json.pc, which does not list /usr/include/nlohmann as a required include directory:

Name: nlohmann_json
Description: JSON for Modern C++
Version: 3.11.3
Cflags: -I/usr/include

CMake

If you provide the json dependency as part of a CMake project like this:

add_subdirectory(deps/json)  # or FetchContent_Declare/FetchContent_MakeAvailable
target_link_libraries(mytarget PRIVATE nlohmann_json::nlohmann_json)

The same problem occurs, requring a manual target_include_directories(mytarget PRIVATE /path/to/nlohmann). See here.


This PR fixes those issues by using the canonical header location. This is consistent with the nlohmann json documentation.

@cebtenzzre
Copy link
Contributor Author

cc @ochafik

@ochafik
Copy link
Collaborator

ochafik commented Feb 18, 2025

Thanks @cebtenzzre , this is also consistent with moving minja down to vendored folder in ggml-org/llama.cpp#11900, I guess the obvious follow up will be to move json.hpp down in llama.cpp soon too.

@ochafik ochafik merged commit dee1b89 into google:main Feb 18, 2025
3 of 11 checks passed
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.

2 participants