Skip to content

[llvm] allow building with builtin_llvm=OFF in Ubuntu26#22513

Open
ferdymercury wants to merge 4 commits into
root-project:masterfrom
ferdymercury:patch-20
Open

[llvm] allow building with builtin_llvm=OFF in Ubuntu26#22513
ferdymercury wants to merge 4 commits into
root-project:masterfrom
ferdymercury:patch-20

Conversation

@ferdymercury

@ferdymercury ferdymercury commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

builtin_llvm=OFF option is not tested anywhere in the CI.

find_package(LLVM REQUIRED CONFIG) does not work otherwise, because the Find-script is rather strict, it only returns true if major AND minor versions are the same, meaning that API compatibility is ensured.

Without this change, in Ubuntu26 I get:

couldn't find LLVM, candidate version 20.1.8 does not match required version. Setting a range 20...21 does not help either, it needs exact match of minor and major.

depends on root-project/root-ci-images#131

find_package(LLVM REQUIRED CONFIG) does not work otherwise, because the Find-script is rather strict, it only returns true if major AND minor versions are the same, meaning that API compatibility is ensured.

Without this change, in Ubuntu26 I get:

couldn't find LLVM, candidate version 20.1.8 does not match required version. Setting a range 20...21 does not help either, it needs exact match of minor and major.
@ferdymercury ferdymercury requested a review from guitargeek June 7, 2026 10:11
@dpiparo dpiparo requested review from devajithvs and hahnjo June 7, 2026 10:26
@github-actions

github-actions Bot commented Jun 7, 2026

Copy link
Copy Markdown

Test Results

    22 files      22 suites   3d 15h 25m 27s ⏱️
 3 859 tests  3 848 ✅   0 💤 11 ❌
76 253 runs  76 124 ✅ 118 💤 11 ❌

For more details on these failures, see this check.

Results for commit c766d9e.

♻️ This comment has been updated with latest results.

@dpiparo

dpiparo commented Jun 7, 2026

Copy link
Copy Markdown
Member

this is a good initiative. Once ubu26 failed, I stopped the build. Maybe, for the tests done in this PR, would it make sense to reduce the number of builds to the ones necessary to check the change? Then, when happy, the full battery can be used before the actual merge.

@ferdymercury ferdymercury added the clean build Ask CI to do non-incremental build on PR label Jun 7, 2026
@ferdymercury

Copy link
Copy Markdown
Collaborator Author

Thanks!

I have tested the Ubuntu26 build like this locally by downloading the docker image registry.cern.ch/root-ci/ubuntu2604 and it seems to build fine with -Dminimal=ON -Dbuiltin_llvm=OFF after installing those three missing packages. So, my suggestion would be:

  • first merge packages needed for building with builtin_llvm=OFF root-ci-images#131
  • then restart this build
  • If there are failures in Ubu26, then do what you say: disable all platforms but Ubuntu26 to save CI resources and debug further. Side note: it would be really great to have a Github label "only-ubuntu" or "only-mac" or "only-windows" where this can be controlled more easily/automated than having to modify the .github yml scripts

@dpiparo

dpiparo commented Jun 7, 2026

Copy link
Copy Markdown
Member

So far, we did not implement any mechanism to restrict the number of platforms to keep the system simple and maximise coverage. Once upon at time, this was possible, and it did not help development much: in order to save time not all platforms were tested, and that led to breakage.

Said that, I think that if this works well, it well could, it ought to become a special build. It is a big change and we might not want (yet) to affect the binaries created for ubuntu 26 nor add the builtin_llvm as customisation for the PRs.

@dpiparo

dpiparo commented Jun 7, 2026

Copy link
Copy Markdown
Member

we'll also have to remember that that hypothetical build, will cease to function once LLVM22 is merged, for obvious reasons, and perhaps we want it also in the CI of the 6.40 branch, again for obvious reasons :-)

@ferdymercury

ferdymercury commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator Author

Indeed A special build sounds great! It's just for testing not for changing binaries

endif()
else()
find_package(LLVM REQUIRED CONFIG)
find_package(LLVM ${ROOT_LLVM_VERSION_REQUIRED_MAJOR}.${ROOT_LLVM_VERSION_REQUIRED_MINOR} REQUIRED CONFIG)

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.

I'm not sure about this change: You're saying that the previous line find_package(LLVM REQUIRED CONFIG) doesn't work at all? It is the officially documented way to find LLVM: https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project Then we are manually checking the version further below.

@ferdymercury ferdymercury Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's what I am saying/experiencing on Ubuntu 26 root docker image, it does not work at all. If you check the LLVMConfigVersion.cmake file https://github.com/ferdymercury/root/blob/master/interpreter/llvm-project/llvm/cmake/modules/LLVMConfigVersion.cmake.in, it's checking for strict match of major and minor versions. So found version 20.1.8 does not match requested version unspecified.unspecified
It's weird, but they seem to enforce that strict match to be API compatible.
In any case, the solution of adding/specifying major minor when calling find is not too bad, so that we ensure it matches the current builtin llvm.

@ferdymercury ferdymercury Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Seems to work for me when specifying the right variables: #22544

@ferdymercury ferdymercury marked this pull request as ready for review June 8, 2026 15:53
@devajithvs devajithvs self-assigned this Jun 9, 2026
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants