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

krasznaa · 2023-12-05T10:28:39Z

This Pull request:

Explicitly set Vdt_ROOT when builtin_vdt was used for the build. This way, a default location would be provided for VDT, which the user could still override if they wanted to.

Changes or fixes:

This fixes the issue reported in #14163. Following up from the changes introduced in #11844.

Note that this fix requires CMake 3.12+, with CMP0074 set to NEW. 🤔 Supporting older CMake versions, or that policy set to OLD would also be possible, but would require a lot more lines of code. (FindVdt.cmake would need to learn about a new hint variable itself.)

Still, if people here feel very strongly about it, it would be possible to go that route as well. 🤔

Checklist:

tested changes locally
updated the docs (if necessary) (N/A)

This PR fixes: #14163

This way, a default location would be provided for VDT, which the user could still override if they wanted to.

phsft-bot · 2023-12-05T10:28:49Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

phsft-bot · 2023-12-05T10:37:33Z

Build failed on windows10/default.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2023-12-05T10:37:31.892Z] LINK : fatal error LNK1104: cannot open file 'C:\build\workspace\root-pullrequests-build\build\bin\libCore.dll' [C:\build\workspace\root-pullrequests-build\build\core\Core.vcxproj]

ferdymercury · 2023-12-05T10:51:35Z

Thanks a lot for the PR!

Side note: to fully fix the linked issue, I think we also need something similar for when nlohmanjson is builtin. Maybe @linev knows more.

olantwin · 2023-12-05T11:00:43Z

Thanks a lot, @krasznaa, for the quick fix.

Note that this fix requires CMake 3.12+, with CMP0074 set to NEW.

Strangely enough, with that policy set it works for me with and without your fix, maybe because we already set $ROOT_ROOT in the build environment.

Do I understand it correctly that your fix covers the case where ROOT is discovered via $ROOT_DIR only?

bellenot

LGTM. Thanks Attila!

ferdymercury · 2023-12-05T11:32:39Z

cmake/scripts/ROOTConfig.cmake.in

@@ -108,6 +108,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")


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

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.

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.

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

😕 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. 🤔

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)

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. 🤔

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).

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

krasznaa · 2023-12-05T13:06:42Z

Thanks a lot, @krasznaa, for the quick fix.

Note that this fix requires CMake 3.12+, with CMP0074 set to NEW.

Strangely enough, with that policy set it works for me with and without your fix, maybe because we already set $ROOT_ROOT in the build environment.

Do I understand it correctly that your fix covers the case where ROOT is discovered via $ROOT_DIR only?

This is a very good point. 🤔 Indeed, this update still requires CMP0074 to be NEW. With that setting, just using ROOT_ROOT to point at the ROOT installation, is already enough to also find VDT.

Still. Since one may use ROOT_DIR, this addition should still make things a bit more robust. But we'll still need to advise people to either request CMake 3.12 as a minimum in their own code, or to explicitly set CMP0074 to NEW. 🤔

olantwin · 2023-12-05T13:16:21Z

Still. Since one may use ROOT_DIR, this addition should still make things a bit more robust. But we'll still need to advise people to either request CMake 3.12 as a minimum in their own code, or to explicitly set CMP0074 to NEW. 🤔

While ROOT already requires CMake >= 3.12, I guess as of 6.30 it requires that any projects depending on ROOT also need to require 3.12 as a minimum (or set the policy explicitly) (regardless of what the CMake version actually is). Not sure whether this is documented somewhere, at least to me it wasn't 100% obvious. A lot of packages don't seem to define a minimum version at all...

krasznaa · 2023-12-05T13:22:27Z

While ROOT already requires CMake >= 3.12, I guess as of 6.30 it requires that any projects depending on ROOT also need to require 3.12 as a minimum (or set the policy explicitly) (regardless of what the CMake version actually is). Not sure whether this is documented somewhere, at least to me it wasn't 100% obvious. A lot of packages don't seem to define a minimum version at all...

It is indeed a requirement for the users. The way that CMake works, even if you use the very latest CMake version, if your code has let's say

cmake_minimum_required(VERSION 3.10)

in it, then CMake will try to behave exactly like version 3.10 did. To have CMP0074 have the "NEW behaviour", either the "last" cmake_minimum_required(...) call had to require CMake 3.12+, or the CMP0074 policy specifically has to be set to NEW. (This way one can pick and choose which policy should behave how, without opting into all of the policies introduced by CMake 3.12.)

Now... One could add cmake_minimum_required(VERSION 3.12) to ROOTConfig.cmake. 🤔 I just fear that that may break the code of a couple of users. Though it might not... Maybe everybody already uses a newer CMake version than that with the latest ROOT versions.

olantwin · 2023-12-05T13:30:27Z

Now... One could add cmake_minimum_required(VERSION 3.12) to ROOTConfig.cmake. 🤔 I just fear that that may break the code of a couple of users. Though it might not... Maybe everybody already uses a newer CMake version than that with the latest ROOT versions.

Maybe if we depend on this behaviour to find VDT, we could set only 0074 in the ROOTConfig.cmake? (can we unset it at the end of the ROOTConfig.cmake?)
While the actual CMake version should probably be high enough, it could be that some packages depend on pre-3.12 behaviour for other policies?

phsft-bot · 2023-12-05T13:46:26Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

krasznaa · 2023-12-05T13:48:44Z

So, I added an attempt for doing this in the least invasive way. 🤔 With this latest setting, the configuration

cmake_minimum_required(VERSION 3.10)
project(ROOTFindTester VERSION 0.0.1 LANGUAGES CXX)

find_package(ROOT 6.30 CONFIG REQUIRED)

succeeds.

[bash][pcadp04]:buildtest > cmake -DROOT_DIR=/mnt/hdd1/krasznaa/projects/root/install/cmake ../findtester/
-- The CXX compiler identification is GNU 11.3.1
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found Vdt: /mnt/hdd1/krasznaa/projects/root/install/include (found version "0.4") 
-- Configuring done
-- Generating done
-- Build files have been written to: /mnt/hdd1/krasznaa/projects/root/buildtest
[bash][pcadp04]:buildtest >

Wile previously it failed.

[bash][pcadp04]:buildtest > cmake -DROOT_DIR=/mnt/hdd1/krasznaa/projects/root/install/cmake ../findtester/
-- The CXX compiler identification is GNU 11.3.1
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Warning (dev) at /usr/share/cmake/Modules/CMakeFindDependencyMacro.cmake:47 (find_package):
  Policy CMP0074 is not set: find_package uses <PackageName>_ROOT variables.
  Run "cmake --help-policy CMP0074" for policy details.  Use the cmake_policy
  command to set the policy and suppress this warning.

  CMake variable Vdt_ROOT is set to:

    /mnt/hdd1/krasznaa/projects/root/install/cmake/..

  For compatibility, CMake is ignoring the variable.
Call Stack (most recent call first):
  /mnt/hdd1/krasznaa/projects/root/install/cmake/ROOTConfig.cmake:140 (find_dependency)
  CMakeLists.txt:5 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Error at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Vdt (missing: VDT_INCLUDE_DIR VDT_LIBRARY)
Call Stack (most recent call first):
  /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:594 (_FPHSA_FAILURE_MESSAGE)
  /mnt/hdd1/krasznaa/projects/root/install/cmake/modules/FindVdt.cmake:63 (find_package_handle_standard_args)
  /usr/share/cmake/Modules/CMakeFindDependencyMacro.cmake:47 (find_package)
  /mnt/hdd1/krasznaa/projects/root/install/cmake/ROOTConfig.cmake:140 (find_dependency)
  CMakeLists.txt:5 (find_package)


-- Configuring incomplete, errors occurred!
See also "/mnt/hdd1/krasznaa/projects/root/buildtest/CMakeFiles/CMakeOutput.log".
[bash][pcadp04]:buildtest >

phsft-bot · 2023-12-05T13:54:56Z

Build failed on windows10/default.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2023-12-05T13:54:54.745Z] LINK : fatal error LNK1104: cannot open file 'C:\build\workspace\root-pullrequests-build\build\bin\libCore.dll' [C:\build\workspace\root-pullrequests-build\build\core\Core.vcxproj]

github-actions · 2023-12-05T18:56:42Z

Test Results

      10 files       10 suites 1d 23h 8m 4s ⏱️
  2 484 tests   2 483 ✔️ 0 💤 1 ❌
23 763 runs 23 762 ✔️ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit cfc9425.

bellenot · 2023-12-06T15:08:42Z

OK I merge this and then we'll see. And for nlohmann_json, please open another issue/PR if there is anything to do about it.

Explicitly set Vdt_ROOT when builtin_vdt was used for the build.

aa91eac

This way, a default location would be provided for VDT, which the user could still override if they wanted to.

krasznaa requested a review from bellenot as a code owner December 5, 2023 10:28

guitargeek assigned bellenot Dec 5, 2023

bellenot approved these changes Dec 5, 2023

View reviewed changes

ferdymercury reviewed Dec 5, 2023

View reviewed changes

Require CMP0074 to be NEW for ROOTConfig.cmake.

cfc9425

ferdymercury mentioned this pull request Dec 6, 2023

CMake build broken with rootproject/root:latest image root-project/root-docker#60

Closed

bellenot merged commit 3fcf2f1 into root-project:master Dec 6, 2023
11 of 16 checks passed

This was referenced Dec 6, 2023

cmake find_package ROOT 6.30 broken: it requires nlohmann-json #14188

Closed

cmake find_package VDT not found in root docker container #14195

Closed

dpiparo added this to the 6.32/00 milestone Dec 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

krasznaa commented Dec 5, 2023 •

edited

phsft-bot commented Dec 5, 2023

phsft-bot commented Dec 5, 2023

ferdymercury commented Dec 5, 2023 •

edited

olantwin commented Dec 5, 2023

bellenot left a comment

ferdymercury Dec 5, 2023

linev Dec 5, 2023

ferdymercury Dec 5, 2023 •

edited

linev Dec 5, 2023

krasznaa Dec 5, 2023

ferdymercury Dec 5, 2023

krasznaa Dec 5, 2023

ferdymercury Dec 5, 2023 •

edited

ferdymercury Dec 5, 2023

krasznaa commented Dec 5, 2023

olantwin commented Dec 5, 2023

krasznaa commented Dec 5, 2023

olantwin commented Dec 5, 2023

phsft-bot commented Dec 5, 2023

krasznaa commented Dec 5, 2023

phsft-bot commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

bellenot commented Dec 6, 2023

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

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

Conversation

krasznaa commented Dec 5, 2023 • edited

This Pull request:

Changes or fixes:

Checklist:

phsft-bot commented Dec 5, 2023

phsft-bot commented Dec 5, 2023

Errors:

ferdymercury commented Dec 5, 2023 • edited

olantwin commented Dec 5, 2023

bellenot left a comment

Choose a reason for hiding this comment

ferdymercury Dec 5, 2023

Choose a reason for hiding this comment

linev Dec 5, 2023

Choose a reason for hiding this comment

ferdymercury Dec 5, 2023 • edited

Choose a reason for hiding this comment

linev Dec 5, 2023

Choose a reason for hiding this comment

krasznaa Dec 5, 2023

Choose a reason for hiding this comment

ferdymercury Dec 5, 2023

Choose a reason for hiding this comment

krasznaa Dec 5, 2023

Choose a reason for hiding this comment

ferdymercury Dec 5, 2023 • edited

Choose a reason for hiding this comment

ferdymercury Dec 5, 2023

Choose a reason for hiding this comment

krasznaa commented Dec 5, 2023

olantwin commented Dec 5, 2023

krasznaa commented Dec 5, 2023

olantwin commented Dec 5, 2023

phsft-bot commented Dec 5, 2023

krasznaa commented Dec 5, 2023

phsft-bot commented Dec 5, 2023

Errors:

github-actions bot commented Dec 5, 2023

Test Results

bellenot commented Dec 6, 2023

krasznaa commented Dec 5, 2023 •

edited

ferdymercury commented Dec 5, 2023 •

edited

ferdymercury Dec 5, 2023 •

edited

ferdymercury Dec 5, 2023 •

edited