Skip to content

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Aug 27, 2025

This follows up on guitargeek@e0f8cbc, implementing a more elegant solution
for the problem in #19327, which is now better understood.

The problem was the interaction between my commit guitargeek@c77bd87 in the
6.36 development cycle, the CMAKE_BUILD_WITH_INSTALL_RPATH=TRUE option that
is set by the MacPorts build system, and the gnuinstall=ON option.

In case of gnuinstall=ON, the install tree directories are different
form the build tree directories, in particular the relative path from
the binary to the library directory. The correct RPATH values are
inferred from this relative path. Now, when the user sets
CMAKE_BUILD_WITH_INSTALL_RPATH=TRUE at configuration time, CMake uses
the RPATHs for the install tree also for the build tree, which is not
correct. The build of ROOT will fail, because rootcling doesn't have
the right RPATH, and rootcling is used during the build.

Two mechanisms in the ROOT CMake code were in place to guard against
this problem.

  1. The LD_LIBRARY_PATH is set in the rootcling calls during the
    build, on Linux the build would have succeeded. But on macOS, is was
    forgotten to use DYLD_LIBRARY_PATH, which was not done correctly
    until recently with guitargeek@e0f8cbc.

  2. Another way how ROOT worked around this RPATH consistenty problem was to
    simply set CMAKE_BUILD_WITH_INSTALL_RPATH=FALSE, no matter what the
    user wants. I thought overriding these global CMAKE_* parameters that
    are quite useful for the users is not good practice, so I removed that
    in guitargeek@c77bd87.

So mechanism 1 failed because of macOS, and mechanism 2 failed because I
removed it.

Now that I understood the real problem, this commit implements the
proper solution: override the global CMAKE_BUILD_WITH_INSTALL_RPATH
only for the rootcling target, so that it can be used during the build
successfully.

This means we can also remove the mechanism 1 that set the library path
for rootcling during the build, as the RPATH is now guaranteed to be
correct. Removing that mechanism makes the CMake code more homogeneus
with the Windows version, since setting the library path on Windows is
not necessary.

@guitargeek guitargeek self-assigned this Aug 27, 2025
@guitargeek guitargeek requested a review from bellenot as a code owner August 27, 2025 16:06
@guitargeek guitargeek added in:Build System clean build Ask CI to do non-incremental build on PR labels Aug 27, 2025
@guitargeek guitargeek marked this pull request as draft August 27, 2025 16:06
Copy link

github-actions bot commented Aug 27, 2025

Test Results

    21 files      21 suites   3d 22h 12m 55s ⏱️
 3 645 tests  3 494 ✅  0 💤 151 ❌
74 804 runs  74 630 ✅ 23 💤 151 ❌

For more details on these failures, see this check.

Results for commit 3183c29.

♻️ This comment has been updated with latest results.

@bellenot
Copy link
Member

So on Windows, the DLLs are found in the PATH env variable, like any other executable.

@guitargeek guitargeek force-pushed the win_followup branch 5 times, most recently from 7b4d9aa to e603699 Compare August 28, 2025 16:16
@guitargeek guitargeek marked this pull request as ready for review August 28, 2025 16:29
@guitargeek guitargeek changed the title [CMake] Don't set LD_LIBRARY_PATH on Windows for rootcling [CMake] Use TO_NATIVE_PATH_LIST to build rootcling invocations Aug 28, 2025
@guitargeek guitargeek force-pushed the win_followup branch 2 times, most recently from a63d46e to a0f5da1 Compare August 29, 2025 00:22
@guitargeek guitargeek requested a review from dpiparo as a code owner August 29, 2025 00:27
@guitargeek guitargeek changed the title [CMake] Use TO_NATIVE_PATH_LIST to build rootcling invocations [CMake] Ensure rootcling RPATH for build tree includes library dir Aug 29, 2025
@guitargeek guitargeek changed the title [CMake] Ensure rootcling RPATH for build tree includes library dir [CMake] Ensure executables RPATH in build tree includes library dir Aug 29, 2025
@guitargeek guitargeek force-pushed the win_followup branch 3 times, most recently from 34f7acd to 97a6cf2 Compare August 29, 2025 02:48
This follows up on e0f8cbc, implementing a more elegant solution
for the problem in root-project#19327, which is now better understood.

The problem was the interaction between my commit c77bd87 in the
6.36 development cycle, the `CMAKE_BUILD_WITH_INSTALL_RPATH=TRUE` option that
is set by the MacPorts build system, and the `gnuinstall=ON` option.

In case of `gnuinstall=ON`, the install tree directories are different
form the build tree directories, in particular the relative path from
the binary to the library directory. The correct RPATH values are
inferred from this relative path. Now, when the user sets
`CMAKE_BUILD_WITH_INSTALL_RPATH=TRUE` at configuration time, CMake uses
the RPATHs for the install tree also for the build tree, which is not
correct. The build of ROOT will fail, because `rootcling` doesn't have
the right RPATH, and `rootcling` is used during the build.

Two mechanisms in the ROOT CMake code were in place to guard against
this problem.

 1. The `LD_LIBRARY_PATH` is set in the `rootcling` calls during the
    build, on Linux the build would have succeeded. But on macOS, is was
    forgotten to use `DYLD_LIBRARY_PATH`, which was not done correctly
    until recently with e0f8cbc.

 2. Another way how ROOT worked around this RPATH consistenty problem was to
    simply set `CMAKE_BUILD_WITH_INSTALL_RPATH=FALSE`, no matter what the
    user wants. I thought overriding these global `CMAKE_*` parameters that
    are quite useful for the users is not good practice, so I removed that
    in c77bd87.

So mechanism 1 failed because of macOS, and mechanism 2 failed because I
removed it.

Now that I understood the real problem, this commit implements the
proper solution: override the global `CMAKE_BUILD_WITH_INSTALL_RPATH`
only for the `rootcling` target, so that it can be used during the build
successfully.

This means we can also remove the mechanism 1 that set the library path
for `rootcling` during the build, as the RPATH is now guaranteed to be
correct. Removing that mechanism makes the CMake code more homogeneus
with the Windows version, since setting the library path on Windows is
not necessary.
@guitargeek guitargeek changed the title [CMake] Ensure executables RPATH in build tree includes library dir [CMake] Support builds with CMAKE_BUILD_WITH_INSTALL_RPATH=ON and gnuinstall=ON Aug 29, 2025
Like this, we don't have to hack the RPATH when relocating the XRootD
install to the ROOT install.
@guitargeek guitargeek force-pushed the win_followup branch 2 times, most recently from e3032c2 to a3932b9 Compare August 29, 2025 11:18
This reverts root-project/roottest@f34c844
from 2017. I don't see any problem when running the tests with ninja,
and now we also have a ninja build in the CI to check it.

This greatly speeds up running `roottest` when using ninja.
Copy link
Member

@bellenot bellenot left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@guitargeek guitargeek merged commit c8d6ef5 into root-project:master Aug 29, 2025
40 of 48 checks passed
@guitargeek guitargeek deleted the win_followup branch August 29, 2025 17:16
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.

2 participants