Skip to content

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Aug 28, 2025

This makes the CMake code more robust.

Right now, we use some install(CODE "execute_process(COMMAND ln -f ...
solution on unix to install rootcint and genreflex. This does not
work in all cases, either because of the usage of \$ENV{DESTDIR} when
DESTDIR is not set, or because hard links are not allowed.

Always copying rootcling - already in the build tree - would avoid
that problem, but by copying we risk sidestepping the CMake mechanisms
to set the RPath correctly when installing the copies, which are not
actual targets.

To make makes things simpler and more robust, this commit suggests to
build the rootcing and genreflex executables as separate targets
from the same source. The cost is very little cost in memory
(rootcling is only 31K, so copying two times only increases the size
of ROOTs bin directory by 1.5 %) and little in compile time (the extra
compile time is less than a second, not noticable in parallel builds).

@guitargeek guitargeek self-assigned this Aug 28, 2025
@guitargeek guitargeek requested a review from bellenot as a code owner August 28, 2025 12:26
@guitargeek guitargeek added in:Build System clean build Ask CI to do non-incremental build on PR labels Aug 28, 2025
@guitargeek guitargeek changed the title [CMake] Always create rootcint and genreflex as copies of rootcling [CMake] Build rootcint and genreflex as separate targets Aug 28, 2025
This makes the CMake code more robust.

Right now, we use some `install(CODE "execute_process(COMMAND ln -f ...`
solution on unix to install `rootcint` and `genreflex`. This does not
work in all cases, either because of the usage of `\$ENV{DESTDIR}` when
`DESTDIR` is not set, or because hard links are not allowed.

Always copying `rootcling` - already in the build tree - would avoid
that problem, but by copying we risk sidestepping the CMake mechanisms
to set the RPath correctly when installing the copies, which are not
actual targets.

To make makes things simpler and more robust, this commit suggests to
build the `rootcing` and `genreflex` executables as separate targets
from the same source. The cost is very little cost in memory
(`rootcling` is only 31K, so copying two times only increases the size
of ROOTs `bin` directory by 1.5 %) and little in compile time (the extra
compile time is less than a second, not noticable in parallel builds).
@guitargeek
Copy link
Contributor Author

To hide whitespace changes for easier review:

ROOT_EXECUTABLE(rootls src/rootls.cxx LIBRARIES RIO Tree Core Rint ROOTNTuple)
# To inherit the dependencies from rootcling
add_dependencies(genreflex rootcling)
add_dependencies(rootcint rootcint)
Copy link
Member

Choose a reason for hiding this comment

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

Is this maybe?

Suggested change
add_dependencies(rootcint rootcint)
add_dependencies(rootcint rootcling)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh... Yes thanks!

@pcanal
Copy link
Member

pcanal commented Aug 28, 2025

The cost is ... little cost ...

Side note: We do not support static library builds but we have had request for it, this would (silently) make them [avoidably] (even) bigger.

@pcanal
Copy link
Member

pcanal commented Aug 28, 2025

when DESTDIR is not set

I assume this is solvable by ... setting it :) (and/or handling the case more explicitly).

So the bigger hurdle is:

or because hard links are not allowed.

Where/when are hard links not allowed?

@guitargeek
Copy link
Contributor Author

Also Python wheels (I can look up the source again when on the computer)

Copy link

Test Results

    21 files      21 suites   3d 18h 26m 46s ⏱️
 3 641 tests  3 488 ✅  0 💤 153 ❌
74 720 runs  74 544 ✅ 23 💤 153 ❌

For more details on these failures, see this check.

Results for commit 0da49b0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean build Ask CI to do non-incremental build on PR in:Build System
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants