Skip to content

[CMake] Fixes and improvements for buildng with builtin_clang=OFF #18386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 13, 2025
Merged

[CMake] Fixes and improvements for buildng with builtin_clang=OFF #18386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b20bb1
[CMake] Don't check if `theanos` library is available
guitargeek Apr 13, 2025
7db0415
[CMake] Disable linking against shared LLVM also for builtin_clang=OFF
guitargeek Apr 13, 2025
82b1a49
[CMake] Improve Clang directory deduction in `core/clingutils`
guitargeek Apr 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 7 additions & 19 deletions core/clingutils/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,31 +80,19 @@ foreach(dict ${stldicts})
target_include_directories(${dict}Dict PRIVATE ${CMAKE_SOURCE_DIR}/interpreter/cling/include/cling/cint)
endforeach()

set(CLANG_RESOURCE_DIR_STEM)
if (builtin_clang)
set(CLANG_RESOURCE_DIR_STEM ${CMAKE_BINARY_DIR}/interpreter/llvm-project/llvm/${CMAKE_CFG_INTDIR}/lib/clang)
set(CLANG_RESOURCE_DIR_VERSION ${LLVM_VERSION_MAJOR})
set(CLANG_INSTALL_PREFIX ${CMAKE_BINARY_DIR}/interpreter/llvm-project/llvm/${CMAKE_CFG_INTDIR})
else ()
set(CLANG_RESOURCE_DIR_STEM ${LLVM_LIBRARY_DIR}/clang)
# A user can define a clang version to use, otherwise find it (but will error if more than one version is present)
if (NOT DEFINED CLANG_RESOURCE_DIR_VERSION)
if (NOT EXISTS ${CLANG_RESOURCE_DIR_STEM})
message(FATAL_ERROR "${CLANG_RESOURCE_DIR_STEM} does not exist. Please install clang.")
endif()
# There is no reasonable way to get the version of clang under which is its resource directory.
# For example, lib/clang/5.0.0/include. Deduce it.
file(GLOB CHILDREN RELATIVE ${CLANG_RESOURCE_DIR_STEM} ${CLANG_RESOURCE_DIR_STEM}/*)
list(LENGTH CHILDREN CHILDREN_LENGTH)
if (${CHILDREN_LENGTH} GREATER 1)
message(FATAL_ERROR "Found more than one version of clang. CLANG_RESOURCE_DIR_VERSION contains: '${CHILDREN}'." )
endif()

list(GET CHILDREN 0 CLANG_RESOURCE_DIR_VERSION)
# If no explicit Clang prefix is set, assume it lives inside LLVM prefix.
if(NOT DEFINED CLANG_INSTALL_PREFIX)
set(CLANG_INSTALL_PREFIX ${LLVM_DIR})
Copy link
Member

Choose a reason for hiding this comment

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

LLVM_DIR is the wrong variable, it is only a hint specified by the user. The right approach is to take one of the variables set by LLVMConfig.cmake, see also commit bb4fde0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, thanks! I'll fix this up, going back to the ${LLVM_LIBRARY_DIR} that was used before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR opened: #18407

endif()
endif()

# We only look for the Clang version that matches the LLVM version, because other setups are not supported.
set(CLANG_RESOURCE_DIR_VERSION ${LLVM_VERSION_MAJOR})

set(CLANG_RESOURCE_DIR ${CLANG_RESOURCE_DIR_STEM}/${CLANG_RESOURCE_DIR_VERSION}/include)
set(CLANG_RESOURCE_DIR ${CLANG_INSTALL_PREFIX}/lib/clang/${CLANG_RESOURCE_DIR_VERSION}/include)

#---Deal with clang resource here----------------------------------------------
install(DIRECTORY ${CMAKE_BINARY_DIR}/etc/cling/lib/clang/${CLANG_RESOURCE_DIR_VERSION}/include/
Expand Down
6 changes: 6 additions & 0 deletions interpreter/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,12 @@ if (builtin_clang)
else()
find_package(Clang REQUIRED CONFIG)
message(STATUS "Found Clang ${CLANG_PACKAGE_VERSION} in ${CLANG_CMAKE_DIR}")

# Disable linking against shared LLVM.
# We have to do this after find_package(Clang). Finding Clang internally
# calls find_package(LLVM), which overwrites LLVM_LINK_LLVM_DYLIB to what it
# was set when building LLVM.
set(LLVM_LINK_LLVM_DYLIB FALSE)
Copy link
Member

Choose a reason for hiding this comment

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

Revisiting the code, this is also not ideal: The right solution that we have chosen in Cling is to pass DISABLE_LLVM_LINK_LLVM_DYLIB to not use the shared library even if present. @guitargeek can you please revert this patch and implement the proper solutions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not as easy, as we have discussed in private. Also, the exact same thing is done in the builtin_clang code path:
https://github.com/root-project/root/blob/master/interpreter/CMakeLists.txt#L380

So this PR here only made the configuration code less inconsistent, which is always good IMHO. So I don't see the reason to revert if the supposedly "right" solution is not implemented in our main configuration code path either.

Copy link
Member

Choose a reason for hiding this comment

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

It's not as easy, as we have discussed in private.

I disagree with this summary of our discussions. For me it is clear, and always has been, that the right solution is implemented on the linking side for libCling.

Also, the exact same thing is done in the builtin_clang code path: https://github.com/root-project/root/blob/master/interpreter/CMakeLists.txt#L380

Yes, though it is slightly different because there we are actually building Clang, not just modifying a variable after find_package(Clang). But I agree that it should also be reviewed.

So this PR here only made the configuration code less inconsistent, which is always good IMHO.

(for a bit more consistency in the file, it should have been OFF instead of FALSE...)

So I don't see the reason to revert if the supposedly "right" solution is not implemented in our main configuration code path either.

This PR was proposed on a Sunday afternoon, on the day before locking the branch, and merged within 2 hours, without review from any of the developers recently involved in this area before. I have pointed out one immediate mistake, and I maintain that fiddling with LLVM_LINK_LLVM_DYLIB is not the correct solution. The statement about "main configuration code path" is incorrect since the default, and only supported configuration for production use as far as I'm concerned, is builtin_llvm=ON builtin_clang=ON.

As changes to the build system this late in the cycle can have a significant blast area, I maintain that this PR should be reverted.

Copy link
Contributor Author

@guitargeek guitargeek Apr 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For me it is clear, and always has been, that the right solution is implemented on the linking side for libCling.

You seemed to have no problem approving and merging the precedent for the builtin_clang=ON case: #13420 (review). What's really different now?

What you say seems contradictory to me. In the issue I opened you said that builtin_llvm=OFF && builtin_clang=OFF are maintained on a best effort basis, and now you argue that changes in these configuration paths can have a significant blast area. Why don't you just let me do the best effort then even though you think it's not perfect 🙂 Better than having something that doesn't work. And it's only natural that these things are discovered late in the dev cycle, because that's when we start thinking about how the new release can be built and packaged.

The statement about "main configuration code path" is incorrect since the default, and only supported configuration for production use as far as I'm concerned, is builtin_llvm=ON builtin_clang=ON

You're right, I apologize. I forgot that the precedent was for builtin_llvm=OFF builtin_clang=ON

Copy link
Member

Choose a reason for hiding this comment

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

  1. In that code path we are building Clang, which I assumed is influenced by setting this variable. If I understand your upstream PR correctly, setting it before building Clang actually doesn't make a difference. So maybe it doesn't serve a purpose in the end, to be tested.
  2. The PR added DISABLE_LLVM_LINK_LLVM_DYLIB for linking, which I think is the right solution. The setting of LLVM_LINK_LLVM_DYLIB may have been an oversight among the other changes in the PR.
  3. I delved deeper into interpreter/CMakeLists.txt since then, in particular last December / January when I cleaned it up quite a bit. This required some reverse engineering to understand what is going on, when I also discovered that some things are not working as I thought.
  4. After realizing that one path is not the right solution, I think it is bad practice to approve merging more code. If code is already there, it should be reworked instead of added to.

Beyond that, I don't consider it good style to link a PR from almost two years ago and say "you accepted it back then"...

now you argue that changes in these configuration paths can have a significant blast area

What I argue is that build system changes in general can have a significant blast radius. This PR contains changes on the main configuration code path in core/clingutils/CMakeLists.txt.

Why don't you just let me do the best effort then even though you think it's not perfect

Because "best effort" doesn't mean that I don't care, and it doesn't mean that the use cases can be broken at will.

Better than having something that doesn't work.

YMMV.

And it's only natural that these things are discovered late in the dev cycle, because that's when we start thinking about how the new release can be built and packaged.

Then this should be changed, there will be a next cycle.

Copy link
Member

Choose a reason for hiding this comment

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

I've submitted a revert in #18446

endif()

# Reset the compiler flags after compiling LLVM and Clang
Expand Down
1 change: 0 additions & 1 deletion tmva/pymva/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ set(Libraries Core MathCore TMVA PyMVA ROOTTMVASofie)
# Look for needed python modules
ROOT_FIND_PYTHON_MODULE(torch)
ROOT_FIND_PYTHON_MODULE(keras)
ROOT_FIND_PYTHON_MODULE(theano)
ROOT_FIND_PYTHON_MODULE(tensorflow)
ROOT_FIND_PYTHON_MODULE(sklearn)

Expand Down
Loading