Revert PR #18386

[hahnjo]

This reverts commit 02b0dc1. It was partially incorrect and should not use LLVM_DIR, which is only a hint specified by the user.

This reverts commit 3ef1193. The taken approach is not the right direction, this should be handled on the linking side of libCling as discussed post-merge in the pull request: #18386

[github-actions]

Test Results

    17 files      17 suites   3d 16h 43m 43s ⏱️
 2 742 tests  2 742 ✅ 0 💤 0 ❌
45 218 runs  45 218 ✅ 0 💤 0 ❌

Results for commit b5322e6.

[dpiparo]

Let's integrate this, and then we can discuss with calm in May how to proceed: we have all the elements on the table.

[guitargeek]

For when we discuss this in May: please give an alternative then how I can build with builtin_clang=off, using LLVM from the packager and without changing the ROOT code. The saved build time was so convenient that I don't want to go back....

Also, what was wrong with: #18407

[hahnjo]

@pcanal while I appreciate you editing the PR summary and adding more details (which were already available as commit messages), please don't put statements that I did not make.

[hahnjo]

For when we discuss this in May: please give an alternative then how I can build with builtin_clang=off, using LLVM from the packager and without changing the ROOT code. The saved build time was so convenient that I don't want to go back....

This works today, if you build without LLVM_LINK_LLVM_DYLIB.

Also, what was wrong with: #18407

If you want others to review a PR, you should add them as reviewers... Fundamentally, the additional PR doesn't address the issue that we are changing the build system just before branching.

[guitargeek]

This works today, if you build without LLVM_LINK_LLVM_DYLIB

I don't want to rebuild LLVM, I want to use the LLVM from my Linux distro which is built with LLVM_LINK_LLVM_DYLIB, which should be possible which I'm pretty sure we have discussed. And setting LLVM_LINK_LLVM_DYLIB in the patched Clang or ROOT doesn't help because find_package(LLVM) overwrites it for no good reason. See:
llvm/llvm-project#135570

[pcanal]

which were already available as commit messages

Some reviewer do not have a commit focused workflow and those cases it is much more convenient to also express the change in the PR in the summary even if it is as simple as a copy/pasting of the commit message.

In my opinion, ideally the the summary should also include also a higher level view, in particular for reverts, a general statement of purpose for the reversion and upcoming plan is helpful (i.e. it is useful to know difference between not needed, needs small revision, needs large redesign, needs discussion or just wait and resubmit later as is)

please don't put statements that I did not make.

Absolutely; Thank you for correcting the text.
I thought I was paraphrasing something I understood you meant in related comments
But I agree, it would have been much better for me to add those are separate comments.

[guitargeek]

My fix that was reverted was also needed because of the added libInterOp, which is new in 6.36.

See:
#17698 (comment)

I think this revert has left the release branch in a worse state.

[guitargeek]

Hi! I have now decided so simply put the fix for this LLVM_LINK_LLVM_DYLIB behavior into my build of the patched Clang:
https://github.com/guitargeek/dotfiles/blob/master/nixos/pkgs/clang-root/Fix-find_package-LLVM-overwriting-LLVM_LINK_LLVM_DYLIB.patch

Therefore, I don't need it anymore in ROOT and I don't need to fight this battle anymore 😆

What I still need in ROOT in any case is some support for Clang builds that are outside the LLVM install tree. I have a new PR that suggests to support this in a more minimal way than my previous PR that was reverted:
#18460