[cmake] Assume XRootD >= 5.0.0 and migrate to `XRootDConfig.cmake` shipped by XRootD #13752

guitargeek · 2023-09-29T02:54:50Z

Assume that XRootD will always be >= 5.0.0 since version 4 is EOL for two years now and all the ROOT features that need XRootD 4 are deprecated by now.
Migrates finding XRootD to the XRootDConfig.cmake from XRootD itself.

Axel-Naumann · 2023-09-29T06:16:41Z

net/netxng/CMakeLists.txt

@@ -21,7 +21,7 @@ ROOT_STANDARD_LIBRARY_PACKAGE(NetxNG
    src/TNetXNGSystem.cxx
    src/RRawFileNetXNG.cxx
  LIBRARIES
-    Xrootd::Xrootd
+    ${XROOTD_LIBRARIES}


We should try to link only against the client libraries. I bet there's a part in the upstream FindXROOTD.cmake that allows for this?

I think there is. I'll try to use it

It works, the PR is updated

guitargeek · 2023-09-29T08:54:08Z

Hi! Just as an update on testing this PR: I tried to build ROOT with xrootd with xrootd from this branch here, to see if it gets correctly picked up also with this PR:
https://github.com/xrootd/xrootd/tree/v4.12.x

And it works fine without the xproofd flag 👍 But with xproofd=ON, the ROOT build fails. However, that is mostly related to a host of other problems in the CMake and also C++ code of xprooff (it seems like this deprecated component was not built and tested for a long time).

andresailer · 2023-09-29T09:17:12Z

cmake/modules/SearchInstalledSoftware.cmake

+
+  # By the presence of the XrdClient/XrdClientAdmin.hh header, we check if netx can be compiled (XRootD < 5.0.0).
+  set(CMAKE_REQUIRED_INCLUDES ${XROOTD_INCLUDE_DIRS})
+  check_include_file_cxx(XrdClient/XrdClientAdmin.hh _xrd_client_admin_hh)


This just accidentally might find header files in system locations? Can you add a if (xrootd VERSION_LESS 5) around this or something?

cf. #11750 where libraries from different xrootd installations were mixed.

Thank you, that's a great idea!

Ah, now I remember why I didn't do this obvious thing! It's because XRootD only includes the version info in the latest releases:
xrootd/xrootd#2015

Until then, we either keep a more rigurous version check line in our old FindXROOTD.cmake, or we use a less verbose hack like this. Considering that XRootD is already at its end of support, I don't think it's worth to spend much time optimizing this. I think in ROOT we are anyway going to remove soon the components that require XRootD 4.

But maybe @amadio has a better idea!

But the problem really is that you might have more than one xrootd in your paths somehow, so you really shouldn't pick up these header files if you already know that you are not compatible.

Something like?

# if we don't have any version information from the XROOTD cmake, let's just try # to pick up some header files if(NOT XRootD_VERSION_MAJOR OR XRootD_VERSION_MAJOR VERSION_LESS 5) #... endif()

I see, yes that makes sense. So in that case, you can be sure that at least if you have a new xrootd with version variables (which I presume you do in LCG, right?), then the old headers will not be detected.

Yes, we have a fairly recent xrootd (5.6.0)

Nice, and thanks again for your feedback! I think I addressed your issue now, can you check if #11750 is fixed?

Now, if you have xrootd > 5.6.0 installed and you don't use builtin_xrootd, there should be nothing in the cmake code path besides standard find_package() and version checks.

I think for now it might be necessary to keep your own module, but simply require a new version and drop all deprecated code using XRootD 4.x. I will fix XRootDConfig.cmake to properly export a version in the next feature release, then you will be able to call find_package(XRootD 5.7) and finally drop FindXROOTD.cmake from ROOT.

I've created a ticket on XRootD for this: xrootd/xrootd#2094

That's great, thank you!

amadio · 2023-10-27T14:20:44Z

Note that XRootD 5.6.3 has been released, and now exports the version via CMake.

guitargeek · 2023-11-01T16:58:35Z

Awesome @amadio! I will take a look

phsft-bot · 2023-11-02T11:24:35Z

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

guitargeek · 2023-11-02T11:43:45Z

Hi @amadio, the situation also changed a bit because we deprecated xproofd, which still depended on xrootd 4.

I think we should just assume now that XRootD will be at least version 5, since version 4 is EOL for two years already and we have no non-deprecated featured depending on XRootD 4.

@andresailer, would this fix your issues?

@amadio, does that make sense from the XRootD perspective?

@Axel-Naumann, what do you think? This does add some extra step when we want to resurrect the XRootD 4 dependent features, but this would not be easy anyway (if they even work with eos 4 also being EOL)

github-actions · 2023-11-02T13:14:22Z

Test Results

        7 files         7 suites 1d 8h 27m 32s ⏱️
  2 483 tests   2 483 ✔️ 0 💤 0 ❌
16 448 runs 16 448 ✔️ 0 💤 0 ❌

Results for commit 6e7798e.

♻️ This comment has been updated with latest results.

phsft-bot · 2023-11-02T13:16:26Z

Build failed on mac12arm/cxx20.
Running on macphsft26.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

And 9 more

amadio · 2023-11-02T14:04:55Z

@amadio, does that make sense from the XRootD perspective?

Yes, XRootD/EOS 4.x are both EOL at the end of this year. All EOS instances will be on EOS 5.x by then. If you can, depend on XRootD from EPEL, as it's well maintained by @ellert. On Debian/Ubuntu, however, due to their restrictions on updating packages to newer versions, you may want to carry the builtin just in case for the older releases. Depending on when you plan to release ROOT 6.30, XRootD 5.6.3 (current release) or 5.7.0 (upcoming feature release early December) should be used for the builtin, to have the fixed CMake module exporting a version.

Let me know if you have any other questions.

phsft-bot · 2023-11-02T18:00:29Z

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

Axel-Naumann · 2023-11-02T18:57:07Z

xproofd was deprecated in v6.30, we should be good to remove it in master. Does that address the XRootD 4 headaches?

guitargeek · 2023-11-02T19:26:53Z

If you mean with XRootD 4 headaches the problems that show up when old XRootD 4 is still installed on the system, then yes (see the issues linked to this PR). We now use the FindXRootD from XRootD, and not our own, which is better in dealing with those cases. Like this one:
#11750

Edit: ah you were talking about the xproofd deprecation, not necessarily this PR. Yes that was also important in addressing the XRootD headaches, because then we don't need to do the xrootd version check anymore to determine if xproofd can actually be built.

bellenot

LGTM, if it works 😉

Xrootd 4 reached end of life in september 2021: https://xrootd.slac.stanford.edu/ Therefore, we can assume now that xrootd will always be at least that version, which greatly simplifies the configuration. The version checks were used in two places: * To check if `xproofd` can be built. But `xproofd` is removed in ROOT 6.32. * To determine whether to build `netx` and/or `netxng`. Now we just biuld `netxng` whenever XRootD is available. `netxng` requires XRootD > 3.3.5, which should be a given at this point. The old `netx` required XRootD < 5.0.0, which is EOL and should not be used anymore.

Migrates finding XRootD to the `XRootDConfig.cmake` from XRootD itself.

phsft-bot · 2023-11-30T15:58:24Z

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

guitargeek · 2023-11-30T15:58:38Z

Just rebasing on master to see if it actually still works

hahnjo · 2023-12-04T09:33:09Z

This broke the build on all EPEL-based distributions where the headers are in /usr/include/xrootd, but XRootDConfig.cmake reports /usr/include. I believe this is an error in how the rpm is packaged, but we cannot fix it immediately. Our CI was also very clear about this problem by failing on 5 platforms (!), 3x Fedora and 2x Alma Linux, which I would again like to point out must be always checked before merging! I'm reverting the second commit in #14170 to restore our builds and also help the LCG people.

guitargeek added the in:Build System label Sep 29, 2023

guitargeek requested a review from bellenot as a code owner September 29, 2023 02:54

guitargeek force-pushed the xrootd_fixups branch from 9b6f721 to ed9ff80 Compare September 29, 2023 04:17

Axel-Naumann reviewed Sep 29, 2023

View reviewed changes

dpiparo assigned guitargeek Sep 29, 2023

guitargeek mentioned this pull request Sep 29, 2023

Unable to build master with external XROOTD #12631

Closed

guitargeek force-pushed the xrootd_fixups branch from ed9ff80 to 4cbd582 Compare September 29, 2023 09:08

andresailer reviewed Sep 29, 2023

View reviewed changes

guitargeek linked an issue Sep 29, 2023 that may be closed by this pull request

Unable to build master with external XROOTD #12631

Closed

guitargeek changed the title ~~[cmake] Remove FindXROOTD.cmake and work around XRootD version check~~ [cmake] Migrate to XRootDConfig.cmake shipped by XRootD Sep 29, 2023

guitargeek linked an issue Sep 29, 2023 that may be closed by this pull request

FindXROOTD adds libraries to link against irrespective of xrootd version used #11750

Closed

root-project deleted a comment from phsft-bot Sep 29, 2023

guitargeek force-pushed the xrootd_fixups branch 2 times, most recently from 4db3a22 to 2efa0b5 Compare September 29, 2023 13:53

amadio mentioned this pull request Sep 29, 2023

Export project version in CMake modules xrootd/xrootd#2094

Closed

root-project deleted a comment from phsft-bot Nov 2, 2023

root-project deleted a comment from github-actions bot Nov 2, 2023

root-project deleted a comment from phsft-bot Nov 2, 2023

guitargeek force-pushed the xrootd_fixups branch from 2efa0b5 to ec49341 Compare November 2, 2023 11:24

guitargeek changed the title ~~[cmake] Migrate to XRootDConfig.cmake shipped by XRootD~~ [cmake] Assume XRootD >= 5.0.0 and migrate to XRootDConfig.cmake shipped by XRootD Nov 2, 2023

guitargeek force-pushed the xrootd_fixups branch from ec49341 to 0ed3f8a Compare November 2, 2023 18:00

bellenot approved these changes Nov 30, 2023

View reviewed changes

guitargeek added 2 commits November 30, 2023 14:08

[cmake] Migrate to XRootDConfig.cmake shipped by XRootD

6e7798e

Migrates finding XRootD to the `XRootDConfig.cmake` from XRootD itself.

guitargeek force-pushed the xrootd_fixups branch from 0ed3f8a to 6e7798e Compare November 30, 2023 15:58

guitargeek merged commit 2ee691d into root-project:master Dec 1, 2023
11 of 16 checks passed

guitargeek deleted the xrootd_fixups branch December 1, 2023 11:02

hahnjo mentioned this pull request Dec 4, 2023

Revert "[cmake] Migrate to XRootDConfig.cmake shipped by XRootD" #14170

Merged

guitargeek mentioned this pull request Dec 5, 2023

[cmake] Migrate to XRootDConfig.cmake shipped by XRootD #14180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cmake] Assume XRootD >= 5.0.0 and migrate to `XRootDConfig.cmake` shipped by XRootD #13752

[cmake] Assume XRootD >= 5.0.0 and migrate to `XRootDConfig.cmake` shipped by XRootD #13752

guitargeek commented Sep 29, 2023 •

edited

Loading

Axel-Naumann Sep 29, 2023

guitargeek Sep 29, 2023

guitargeek Sep 29, 2023

guitargeek commented Sep 29, 2023

andresailer Sep 29, 2023

guitargeek Sep 29, 2023

guitargeek Sep 29, 2023

andresailer Sep 29, 2023 •

edited

Loading

guitargeek Sep 29, 2023

andresailer Sep 29, 2023

guitargeek Sep 29, 2023

amadio Sep 29, 2023

amadio Sep 29, 2023

guitargeek Sep 29, 2023

amadio commented Oct 27, 2023 •

edited

Loading

guitargeek commented Nov 1, 2023

phsft-bot commented Nov 2, 2023

guitargeek commented Nov 2, 2023

github-actions bot commented Nov 2, 2023 •

edited

Loading

phsft-bot commented Nov 2, 2023

amadio commented Nov 2, 2023

phsft-bot commented Nov 2, 2023

Axel-Naumann commented Nov 2, 2023

guitargeek commented Nov 2, 2023 •

edited

Loading

bellenot left a comment

phsft-bot commented Nov 30, 2023

guitargeek commented Nov 30, 2023

hahnjo commented Dec 4, 2023

[cmake] Assume XRootD >= 5.0.0 and migrate to XRootDConfig.cmake shipped by XRootD #13752

[cmake] Assume XRootD >= 5.0.0 and migrate to XRootDConfig.cmake shipped by XRootD #13752

Conversation

guitargeek commented Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guitargeek commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresailer Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amadio commented Oct 27, 2023 • edited Loading

guitargeek commented Nov 1, 2023

phsft-bot commented Nov 2, 2023

guitargeek commented Nov 2, 2023

github-actions bot commented Nov 2, 2023 • edited Loading

Test Results

phsft-bot commented Nov 2, 2023

Failing tests:

amadio commented Nov 2, 2023

phsft-bot commented Nov 2, 2023

Axel-Naumann commented Nov 2, 2023

guitargeek commented Nov 2, 2023 • edited Loading

bellenot left a comment

Choose a reason for hiding this comment

phsft-bot commented Nov 30, 2023

guitargeek commented Nov 30, 2023

hahnjo commented Dec 4, 2023

[cmake] Assume XRootD >= 5.0.0 and migrate to `XRootDConfig.cmake` shipped by XRootD #13752

[cmake] Assume XRootD >= 5.0.0 and migrate to `XRootDConfig.cmake` shipped by XRootD #13752

guitargeek commented Sep 29, 2023 •

edited

Loading

andresailer Sep 29, 2023 •

edited

Loading

amadio commented Oct 27, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023 •

edited

Loading

guitargeek commented Nov 2, 2023 •

edited

Loading