Skip to content

Don't install headers of builtins into the global install prefix - #22958

Open
hageboeck wants to merge 3 commits into
root-project:masterfrom
hageboeck:ROOT-8655_DontInstallHeadersOfBuiltins
Open

Don't install headers of builtins into the global install prefix#22958
hageboeck wants to merge 3 commits into
root-project:masterfrom
hageboeck:ROOT-8655_DontInstallHeadersOfBuiltins

Conversation

@hageboeck

Copy link
Copy Markdown
Member

This improves the situation of #8655, by not unconditionally installing the XRootD and vdt headers into /include.

XRootD headers aren't installed at all (all ROOT targets depend on it only privately), but vdt headers are needed when RVec.hxx is used from a ROOT that has vdt enabled. To cover the latter case, a small post-install test was added.

@hageboeck hageboeck self-assigned this Jul 29, 2026
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown

Test Results

    20 files      20 suites   3d 2h 17m 54s ⏱️
 3 882 tests  3 882 ✅ 0 💤 0 ❌
68 622 runs  68 622 ✅ 0 💤 0 ❌

Results for commit 52dd787.

♻️ This comment has been updated with latest results.

@ferdymercury
ferdymercury requested a review from guitargeek July 29, 2026 15:52
@guitargeek

Copy link
Copy Markdown
Contributor

#ifdef R__HAS_VDT
#include <vdt/vdtMath.h>
#endif

#include <ROOT/RVec.hxx>

I was just suggesting to only forward declare vdt in this header 🙂 So ROOT builds with vdt=ON can also be used on systems where vdt is not in the default include path, which is improving the portability situation quite a bit:

So we'd have to update your new post-install test when my PR gets merged.

Maybe a more robust way (independent of my PR getting merged or not) to test what you actually want to cover ("ensure that the VDT headers are found correctly"), is just to check that the #include <vdt/vdtMath.h> header can be found? This can even be done at the post install CMake level with the usual header availability checks, I don't think we need a dedicated compiled test for that.

Or is there an advantage of testing this indirectly via RVec.hxx that I'm missing?

@hageboeck
hageboeck force-pushed the ROOT-8655_DontInstallHeadersOfBuiltins branch from 2b895e4 to 42fc1d7 Compare July 30, 2026 07:16

@vepadulano vepadulano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, two minor comments.

Comment thread test/PostInstall/testRVec.cxx Outdated
Comment thread test/PostInstall/testRVec.cxx
@hageboeck

Copy link
Copy Markdown
Member Author

#ifdef R__HAS_VDT
#include <vdt/vdtMath.h>
#endif

#include <ROOT/RVec.hxx>

I was just suggesting to only forward declare vdt in this header 🙂 So ROOT builds with vdt=ON can also be used on systems where vdt is not in the default include path, which is improving the portability situation quite a bit:

If VDT is not included, trying to call these functions would produce an error. So we are shifting the error on inclusion to an error on call. I think that's fair, but your installation is broken in both cases.
If you want, we can go ahead with the other PR, but I would still like to test that using a VDT function in RVec actually works if ROOT claims that it has VDT.

Maybe a more robust way (independent of my PR getting merged or not) to test what you actually want to cover ("ensure that the VDT headers are found correctly"), is just to check that the #include <vdt/vdtMath.h> header can be found? This can even be done at the post install CMake level with the usual header availability checks, I don't think we need a dedicated compiled test for that.

It's not exactly the same: If CMake finds the header (because you gave it enough search hints), you might still not be able to compile an RVec program, because we need to test that all the -I of the CMake target ROOT::ROOTVecOps are correct. That will be the case if we export things correctly, so I would like to keep compiling a program that includes RVec.hxx and tries to call a VDT function. What this program does is secondary, though, so any call of a vdt function will do.

So should we say:

  1. Finish and merge [RVec] Forward declare VDT functions in RVec.hxx header #22927
  2. Update the test in this PR here.

@guitargeek

Copy link
Copy Markdown
Contributor

So should we say:

1. Finish and merge [[RVec] Forward declare VDT functions in `RVec.hxx` header #22927](https://github.com/root-project/root/pull/22927)

2. Update the test in this PR here.

Agreed with that! Let's continue the discussion in #22927 for now, and then adapt the test in this PR.

In order not to install the vdt headers into /usr/include or similar, a
second find hint is added. This would allow for moving the headers to
<ROOT_INCLUDE_DIR>/ROOT/builtins/vdt.
Currently, vdt headers are installed directly into ROOT's header install
directory, which may clash with a genuine installation of vdt. Here,
it's moved to <HeaderDir>/ROOT/builtins/.

Partial fix of root-project#8655.
One of the only builtins that ROOT depends on publicly is VDT. When
using RVec.h, the VDT headers need to be visible, so this small test
tries to invoke a VDT function on an RVec.
@hageboeck
hageboeck force-pushed the ROOT-8655_DontInstallHeadersOfBuiltins branch from a4f5506 to 52dd787 Compare July 31, 2026 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants