Change the way we ship nlohmann::json
Previously we just bundled json.hpp
and included it locally. This MR does the following:
-
Introduce a flagACTS_USE_EXTERNAL_NLOHMANN_JSON
which defaults to OFF.If it is OFF, cmake will download a version ofnlohmann::json
into the build folder at configure time, and useadd_subdirectory
to import it. Targets using it can just dotarget_link_libraries(ActsJsonPlugin PUBLIC nlohmann_json::nlohmann_json
and then include it like#include <nlohmann/json.hpp>
. At install time, the single header version ofnlohmann::json
will be installed alongside Acts (we want this because it's part of the public API of the JSON plugin.If it is ON, cmake will just callfind_package(nlohmann_json 3.2.0 REQUIRED)
. This will make the same targetnlohmann_json::nlohmann_json
available, so no other changes should be required.
After discussion: revised strategy
-
Introduce a flag
ACTS_USE_BUNDLED_NLOHMANN_JSON
which defaults to ON.- If it is ON, cmake will
add_subdirectory
the bundled version inthirdparty/nlohmann_json
. Targets using it can just dotarget_link_libraries(ActsJsonPlugin PUBLIC nlohmann_json::nlohmann_json
and then include it like#include <nlohmann/json.hpp>
. At install time, the single header version ofnlohmann::json
will be installed alongside Acts (we want this because it's part of the public API of the JSON plugin. - If it is OFF, cmake will just call
find_package(nlohmann_json 3.2.0 REQUIRED)
. This will make the same targetnlohmann_json::nlohmann_json
available, so no other changes should be required.
- If it is ON, cmake will
-
Also I'm doing a bit of cleanup in the JSON plugin, the test wasn't actually built, and the header files had global namespace aliases like
using json = nlohmann::json
which is not ideal. -
Change the minimum cmake version required to
3.11
. That's available in LCG95, and we currently don't support versions before that.
The idea is that in the ATLAS externals build, where another package already "builds" nlohmann::json
we can pick that up instead of rolling our own. This was brought up by @akraszna in atlas/atlasexternals!605 (closed).
@akraszna thoughts? (Or anyone else?)
Edited by Paul Gessinger