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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

VDT Finding Fix, master branch (2023.12.05.) #14178

Merged
merged 2 commits into from Dec 6, 2023
Merged

VDT Finding Fix, master branch (2023.12.05.) #14178

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa91eac
Explicitly set Vdt_ROOT when builtin_vdt was used for the build.
krasznaa Dec 5, 2023
cfc9425
Require CMP0074 to be NEW for ROOTConfig.cmake.
krasznaa Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions cmake/scripts/ROOTConfig.cmake.in
View file
Open in desktop
Expand Up @@ -28,6 +28,11 @@
#
# ===========================================================================

# Set any policies required by this module. Note that there is no check for
# whether a given policy exists or not. If it is required, it is required...
cmake_policy(PUSH)
cmake_policy(SET CMP0074 NEW)

#----------------------------------------------------------------------------
# DEBUG : print out the variables passed via find_package arguments
#
Expand Down Expand Up @@ -108,6 +113,9 @@ if(ROOT_minuit2_omp_FOUND)
find_dependency(Threads)
endif()
if(ROOT_vdt_FOUND AND NOT TARGET VDT::VDT)
if(ROOT_builtin_vdt_FOUND)
set(Vdt_ROOT "${_thisdir}/.." CACHE PATH "Location of (the builtin) VDT")
Copy link
Collaborator

Choose a reason for hiding this comment

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

on line 102, we could add sth along this line: else() set (nlohmann_json... "${_thisdir}/.." CACHE PATH "Location of (the builtin) nlohmann-json")

Copy link
Member

Choose a reason for hiding this comment

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

I do not think that we need special rule here for builtin nlohmann/json.hpp.
Only when external nlohmann_json is used, it linked as PUBLIC dependency and need to be there when
correspondent library linked by user application. This is how it defined in REve:

if(builtin_nlohmannjson)
   target_include_directories(ROOTEve PRIVATE ${CMAKE_SOURCE_DIR}/builtins)
else()
   target_link_libraries(ROOTEve PUBLIC nlohmann_json::nlohmann_json)
endif()

So to say - builtin version should not be seen by user cmake.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @linev, but find_package (ROOT) fails (using ubu22 binary release) because it does not find where nlohmann json is, even if I do not need nlohman_json at all in my software.

Copy link
Member

Choose a reason for hiding this comment

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

I just want to understand that kind of changes you want to have for nlohmann_json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

馃槙 Have a look at line 100-102. The logic is actually the opposite for nlohmann_json. CMake only calls find_dependency(nlohmann_json...) if it's not built-in. For a built-in nlohmann_json the CMake setup does nothing. As it expects that the normal ROOT include path would make the nlohmann/json.hpp file visible as well. 馃

But this also means that apparently the binary downloaded by @ferdymercury does not have nlohmann_json included in it. If it did, ROOT would not be looking for it.

Long story short, I didn't think that we needed to do anything with the JSON dependency. Also because that's not something that would've changed recently. I only modified how VDT is handled in 6.30. The JSON dependency did not change wrt. 6.28 as far as I can see. 馃

Copy link
Collaborator

Choose a reason for hiding this comment

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

For adding some info, here a 'diff' of old 6.28 and new 3.30 ROOT versions wrt nlohmann:

grep -r nlohmann_json /tmp/oldroot/*
/tmp/oldroot/cmake/ROOTConfig.cmake:  find_dependency(nlohmann_json 3.10.5)
/tmp/oldroot/include/nlohmann/json.hpp:#define NLOHMANN_JSON_TO(v1) nlohmann_json_j[#v1] = nlohmann_json_t.v1;
/tmp/oldroot/include/nlohmann/json.hpp:#define NLOHMANN_JSON_FROM(v1) nlohmann_json_j.at(#v1).get_to(nlohmann_json_t.v1);
/tmp/oldroot/include/nlohmann/json.hpp:    friend void to_json(nlohmann::json& nlohmann_json_j, const Type& nlohmann_json_t) { NLOHMANN_JSON_EXPAND(NLOHMANN_JSON_PASTE(NLOHMANN_JSON_TO, __VA_ARGS__)) } \
/tmp/oldroot/include/nlohmann/json.hpp:    friend void from_json(const nlohmann::json& nlohmann_json_j, Type& nlohmann_json_t) { NLOHMANN_JSON_EXPAND(NLOHMANN_JSON_PASTE(NLOHMANN_JSON_FROM, __VA_ARGS__)) }
/tmp/oldroot/include/nlohmann/json.hpp:    inline void to_json(nlohmann::json& nlohmann_json_j, const Type& nlohmann_json_t) { NLOHMANN_JSON_EXPAND(NLOHMANN_JSON_PASTE(NLOHMANN_JSON_TO, __VA_ARGS__)) } \
/tmp/oldroot/include/nlohmann/json.hpp:    inline void from_json(const nlohmann::json& nlohmann_json_j, Type& nlohmann_json_t) { NLOHMANN_JSON_EXPAND(NLOHMANN_JSON_PASTE(NLOHMANN_JSON_FROM, __VA_ARGS__)) }
grep: /tmp/oldroot/lib/ROOTDataFrame.pcm: binary file matches
/tmp/oldroot/README/ReleaseNotes/v628/index.md:* [[#11236](https://github.com/root-project/root/issues/11236)] - build failure because of `nlohmann_json`

grep -r nlohmann_json /tmp/newroot/*
/tmp/newroot/cmake/ROOTConfig-targets.cmake:  INTERFACE_LINK_LIBRARIES "ROOT::Core;ROOT::Geom;ROOT::Physics;ROOT::EG;ROOT::TreePlayer;ROOT::RCsg;ROOT::ROOTWebDisplay;nlohmann_json::nlohmann_json"
/tmp/newroot/cmake/ROOTConfig-targets.cmake:  INTERFACE_LINK_LIBRARIES "nlohmann_json::nlohmann_json"
/tmp/newroot/cmake/ROOTConfig.cmake:  find_dependency(nlohmann_json 3.10.5)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I myself would re-write the JSON dependency handling in the same way as VDT is handled now... But that we should do in a separate PR.

And yes, it very much seems that in the 6.30 binary builtin_nlohmannjson was turned off for some reason. 馃

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just want to understand that kind of changes you want to have for nlohmann_json?

Hmmm, I would like to use find_package(ROOT) with binary release 6.30.02 without forcing me to install nlohmann_json externally. (Or if I do need to install externally, then mention that in the prerequisites website).

Copy link
Collaborator

Choose a reason for hiding this comment

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

And yes, it very much seems that in the 6.30 binary builtin_nlohmannjson was turned off for some reason. 馃

Oh, I see. The same happened with builtin_afterimage. If this was done on purpose, then we should add nlohmann_json to the prerequisites, as I did with afterimage: https://github.com/root-project/web/pull/936/files

endif()
find_dependency(Vdt)
endif()

Expand Down Expand Up @@ -166,3 +174,6 @@ foreach(_remaining ${ROOT_FIND_COMPONENTS})
endif()
unset(ROOT_FIND_REQUIRED_${_remaining})
endforeach()

# Clear out any policy settings made for this module.
cmake_policy(POP)