Skip to content

[nlohmann] download instead of bundle #21916

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.

Sign up for GitHub

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

Open
ferdymercury wants to merge 3 commits into
base: master
Choose a base branch
from
+59 −25,737
Open

[nlohmann] download instead of bundle #21916

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6324db4
[nlohmann] download instead of bundle
ferdymercury Apr 15, 2026
cbcb7c0
[ci] enable builtin nlohmann in alma10 clang ninja
ferdymercury Apr 15, 2026
b3f84a1
[nlohmann] move to LCG link
ferdymercury Apr 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/root-ci-config/buildconfig/alma10-clang_ninja.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ builtin_gl2ps=ON
builtin_jpeg=ON
builtin_lz4=ON
builtin_lzma=ON
builtin_nlohmann=ON
builtin_png=ON
builtin_vdt=ON
builtin_zlib=ON
Expand Down
97 changes: 58 additions & 39 deletions builtins/nlohmann/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,58 @@
# Install nlohmann/json.hpp and json_fwd.hpp
# This file will define the target
# nlohmann_json
# and the alias
# nlohmann_json::nlohmann_json
# with proper #defines and includes. Use the alias target with the full prefix for access to JSON.

# The installed files are only used when ACLiC or ROOT macros will include REve headers,
# they are not used for ROOT compilation, as this happens directly from the source directory.

# extract version from existing header file
file(STRINGS "json.hpp" JSON_H REGEX "^#define NLOHMANN_JSON_VERSION_[A-Z]+[ ]+[0-9]+.*$")
string(REGEX REPLACE ".+NLOHMANN_JSON_VERSION_MAJOR[ ]+([0-9]+).*$" "\\1" JSON_VERSION_MAJOR "${JSON_H}")
string(REGEX REPLACE ".+NLOHMANN_JSON_VERSION_MINOR[ ]+([0-9]+).*$" "\\1" JSON_VERSION_MINOR "${JSON_H}")
string(REGEX REPLACE ".+NLOHMANN_JSON_VERSION_PATCH[ ]+([0-9]+).*$" "\\1" JSON_VERSION_PATCH "${JSON_H}")
set(nlohmann_json_VERSION "${JSON_VERSION_MAJOR}.${JSON_VERSION_MINOR}.${JSON_VERSION_PATCH}" PARENT_SCOPE)
unset(JSON_H)

add_custom_command(
OUTPUT ${CMAKE_BINARY_DIR}/include/nlohmann/json.hpp
COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_SOURCE_DIR}/builtins/nlohmann/json.hpp ${CMAKE_BINARY_DIR}/include/nlohmann/json.hpp
DEPENDS ${CMAKE_SOURCE_DIR}/builtins/nlohmann/json.hpp)
add_custom_target(builtin_nlohmann_json_incl DEPENDS ${CMAKE_BINARY_DIR}/include/nlohmann/json.hpp)
set_property(GLOBAL APPEND PROPERTY ROOT_BUILTIN_TARGETS builtin_nlohmann_json_incl)

install(FILES ${CMAKE_SOURCE_DIR}/builtins/nlohmann/json.hpp DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/nlohmann/)

# Create a target and all its configs:
add_library(nlohmann_json INTERFACE)

target_include_directories(nlohmann_json INTERFACE $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/builtins> $<INSTALL_INTERFACE:include>)
target_compile_definitions(nlohmann_json INTERFACE NLOHMANN_JSON_PROVIDES_FWD_HPP)

install(TARGETS nlohmann_json
EXPORT ROOTExports)
set_property(GLOBAL APPEND PROPERTY ROOT_EXPORTED_TARGETS nlohmann_json)

# Alias, so can use it as drop-in replacement for system nlohmann_json.
add_library(nlohmann_json::nlohmann_json ALIAS nlohmann_json)
# Copyright (C) 1995-2026, Rene Brun and Fons Rademakers.
# All rights reserved.
#
# For the licensing terms see $ROOTSYS/LICENSE.
# For the list of contributors see $ROOTSYS/README/CREDITS.

# **PLEASE UPDATE ALSO THE FOLLOWING LINE WHEN UPDATING THE VERSION**
# 11 Nov 2025, https://github.com/nlohmann/json/releases/tag/v3.12.0
set(ROOT_NLOHMANN_VERSION 3.12.0)
Copy link
Copy Markdown
Contributor

@silverweed silverweed Apr 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it: what's the advantage of "unvendoring" this library if we're still pinning its version and have to manually update it? I get it for most other cases, but here we're only getting rid of one file..

Copy link
Copy Markdown
Collaborator Author

@ferdymercury ferdymercury Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm some reasons:

  • main one: if there is a security vulnerability found, you can update (or put down) the LCG package and you fix it for all at the same time, without having to force users to git pull a new code.
  • that single header file is 1MB big, so it's many files in one actually (saving 10 files x 100kb or 1 file x 1000kb looks the same to me, and in both cases you need to have/maintain a separate CMakelists building it rather than a call to ExternalProject). If now in 3.13 nlohmann changes eg the indentation or apply clang format or whatever the whole 1MB file will have to be written and you get extra noise in the ROOT git history. So to me, it seems cleaner to just change the version and the hash than to update
  • reducing AI-bot spam surface like the ones we got recently for libgif, etc.
  • enabling in the future to set -DJSON_MultipleHeaders=ON so that one can be more granular about what parts of JSON to include instead of just a huge 1MB big header. Most probably the effect on build time is negligible, but still, it looks cleaner to me if one could be more granular.
  • it's clearer the version you are using and you do not need the ugly hack of parsing the header file to retrieve the version.

I agree with you that there is not any real advantage or that none are very convincing. But I'd still propose this PR to be consistent with the rest of the builtins, for symmetry. In my opinion, there is only clang/llvm and afterimage who need to be burnt-in due to clear reasons, the other should be real externals.

Copy link
Copy Markdown
Member

@linev linev Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • main one: if there is a security vulnerability found, you can update (or put down) the LCG package and you fix it for all at the same time, without having to force users to git pull a new code.

Does not work. We put hash sum into cmake file and cannot manipulate download package.

Copy link
Copy Markdown
Collaborator Author

@ferdymercury ferdymercury Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. But at least it would be an instant way of communication to all the user-building base.

Copy link
Copy Markdown
Member

@linev linev Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. But at least it would be an instant way of communication to all the user-building base.

The only way will be deletion of compromised packages from LCG servers.
But we should foresee such situation in current cmake files - to clearly signal this to users

Copy link
Copy Markdown
Member

@linev linev Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically having cmake ask for the latest version through a well-known URL or something like that

Local hash guarantee - to large extend - that nobody will try to manipulate download procedure.
Otherwise we open door for man-in-the-middle attacks.

Copy link
Copy Markdown
Contributor

@silverweed silverweed Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise we open door for man-in-the-middle attacks.

ExternalProject_Add supports TLS: couldn't we rather use that?

Copy link
Copy Markdown
Member

@linev linev Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExternalProject_Add supports TLS: couldn't we rather use that?

But you propose to make some extra functionality on top of this.

Means you will request latest version of some package from the server and at this point you should fully trust reply obtained via network.

Copy link
Copy Markdown
Contributor

@silverweed silverweed Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm wrong, but if the first TLS request to get the hash from the server succeeds (which, with certificates, should "guarantee" no MITM), then all other requests to that server will be secure, no?
Though I can see it's a more complicated setup and maybe not worth the gain

Copy link
Copy Markdown
Member

@dpiparo dpiparo Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to object to the discussion above. Maybe just that ROOT is moving since a while towards depending on packages on the system where it's built, e.g. installed via a package manager. Seen from that perspective, the old "built-in" mechanism (de facto a distribution channel) becomes nothing more than a fall-back in case nothing else can be done for a particular package on which (some functionality of) ROOT depends on.

set(ROOT_NLOHMANN_HASH "4b92eb0c06d10683f7447ce9406cb97cd4b453be18d7279320f7b2f025c10187")
set(ROOT_NLOHMANN_PREFIX ${CMAKE_BINARY_DIR}/builtins/NLOHMANN-prefix)

unset(nlohmann_json_DIR)
unset(nlohmann_json_DIR CACHE)

if(WIN32 AND NOT CMAKE_GENERATOR MATCHES Ninja)
if(winrtdebug)
set(ROOT_NLOHMANN_BUILD_COMMAND_FLAGS "--config Debug")
else()
set(ROOT_NLOHMANN_BUILD_COMMAND_FLAGS "--config $<IF:$<CONFIG:Debug,RelWithDebInfo>,RelWithDebInfo,Release>")
endif()
endif()

ExternalProject_Add(
BUILTIN_NLOHMANN
PREFIX ${ROOT_NLOHMANN_PREFIX}
# URL ${lcgpackages}/nlohmann-${ROOT_NLOHMANN_VERSION}.tgz
URL https://github.com/nlohmann/json/archive/refs/tags/v3.12.0.tar.gz # TODO move to LCG
URL_HASH SHA256=${ROOT_NLOHMANN_HASH}
Copy link
Copy Markdown
Member

@hageboeck hageboeck Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about directly taking the header from nlohmann's release?

URL https://github.com/nlohmann/json/releases/download/v3.12.0/json.hpp
URL_HASH SHA256=aaf127c04cb31c406e5b04a63f1ae89369fccde6d8fa7cdda1ed4f32dfc5de63

This way, one could stop after the download step.

Copy link
Copy Markdown
Collaborator Author

@ferdymercury ferdymercury Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does LCG also accept header files? If I remember correctly downloading from GitHub sometimes is less reliable than LCG for pulling / timeouts.
In general, I think it's better to be consistent with tar packages, plus this single header is huge, 1 MB, so it's better to download something compressed :)
Plus this would prevent in the future easy switch to enabling in the future to set -DJSON_MultipleHeaders=ON so that one can be more granular

Copy link
Copy Markdown
Member

@dpiparo dpiparo Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is always possible to get packages from several sources, however, that led in the past to unstable contexts, where the failure of one of the sources made ROOT not compilable.

CMAKE_ARGS -G ${CMAKE_GENERATOR}
-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
-DCMAKE_INSTALL_PREFIX=<INSTALL_DIR>
-DBUILD_TESTING=OFF
-DJSON_32bitTest=OFF
-DJSON_BuildTests=OFF
-DJSON_MultipleHeaders=OFF
BUILD_COMMAND ${CMAKE_COMMAND} --build . ${ROOT_NLOHMANN_BUILD_COMMAND_FLAGS}
INSTALL_COMMAND ${CMAKE_COMMAND} --build . ${ROOT_NLOHMANN_BUILD_COMMAND_FLAGS} --target install
LOG_DOWNLOAD 1
LOG_CONFIGURE 1
LOG_BUILD 1
LOG_INSTALL 1
LOG_OUTPUT_ON_FAILURE 1
TIMEOUT 600)

file(MAKE_DIRECTORY ${ROOT_NLOHMANN_PREFIX}/include)
add_library(nlohmann_json::nlohmann_json IMPORTED INTERFACE GLOBAL)
add_dependencies(nlohmann_json::nlohmann_json BUILTIN_NLOHMANN)
set_target_properties(nlohmann_json::nlohmann_json PROPERTIES
INTERFACE_COMPILE_FEATURES "cxx_std_11"
INTERFACE_INCLUDE_DIRECTORIES ${ROOT_NLOHMANN_PREFIX}/include)
target_compile_definitions(nlohmann_json::nlohmann_json INTERFACE NLOHMANN_JSON_PROVIDES_FWD_HPP)

# Set the canonical output of find_package according to
# https://cmake.org/cmake/help/latest/manual/cmake-developer.7.html#standard-variable-names
set(nlohmann_json_INCLUDE_DIRS ${ROOT_NLOHMANN_PREFIX}/include PARENT_SCOPE)
set(nlohmann_json_FOUND TRUE PARENT_SCOPE)
set(nlohmann_json_VERSION ${ROOT_NLOHMANN_VERSION} PARENT_SCOPE)
21 changes: 0 additions & 21 deletions builtins/nlohmann/LICENSE.MIT
View file
Open in desktop

This file was deleted.

Loading
Loading