Skip to content
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.

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

Unable to specify a valid path for llvm installation #703

Open
bwarden opened this issue Mar 5, 2025 · 7 comments
Open

Unable to specify a valid path for llvm installation #703

bwarden opened this issue Mar 5, 2025 · 7 comments

Comments

@bwarden
Copy link

bwarden commented Mar 5, 2025

In Clear Linux, 64-bit libraries are installed under the standard location /usr/lib64. As a result, the system llvm installs files such as /usr/lib64/cmake/llvm/LLVMConfig.cmake.

The following cmake specification hard-codes lib, so we have no way to pass in a valid CA_LLVM_INSTALL_DIR.

# Add our cmake modules directory to the cmake include path including
# LLVM/Clang.
string(REPLACE "\\" "/" CA_LLVM_INSTALL_DIR "${CA_LLVM_INSTALL_DIR}")
if(NOT EXISTS "${CA_LLVM_INSTALL_DIR}/lib/cmake/llvm/LLVMConfig.cmake")
message(FATAL_ERROR
"'${CA_LLVM_INSTALL_DIR}/lib/cmake/llvm/LLVMConfig.cmake' does not exist"
" (search path set with CA_LLVM_INSTALL_DIR)")
endif()
if(NOT EXISTS "${CA_LLVM_INSTALL_DIR}/lib/cmake/clang/ClangTargets.cmake")
message(FATAL_ERROR
"'${CA_LLVM_INSTALL_DIR}/lib/cmake/clang/ClangTargets.cmake' does not exist"
" (search path set with CA_LLVM_INSTALL_DIR)")
endif()
list(APPEND CMAKE_MODULE_PATH
${CA_LLVM_INSTALL_DIR}/lib/cmake/llvm
${CA_LLVM_INSTALL_DIR}/lib/cmake/clang)
set(LLVM_DIR ${CA_LLVM_INSTALL_DIR}/lib/cmake/llvm)

@hvdijk
Copy link
Collaborator

hvdijk commented Mar 5, 2025

Thanks for the report! I see at https://github.com/clearlinux-pkgs/llvm/blob/main/cmake_args that Clear Linux does this by setting LLVM_LIBDIR_SUFFIX=64. A possible fix would therefore be to change ${CA_LLVM_INSTALL_DIR}/lib/... to ${CA_LLVM_INSTALL_DIR}/lib${CA_LLVM_LIBDIR_SUFFIX}/... everywhere so that you could pass CA_LLVM_LIBDIR_SUFFIX=64. Would that be enough for you to be able to build successfully?

@bwarden
Copy link
Author

bwarden commented Mar 5, 2025

Would that be enough for you to be able to build successfully?

Technically yes, although the slightly better approach would be to initialize CA_LLVM_LIBDIR_SUFFIX from LLVM_LIBDIR_SUFFIX if it's not otherwise specified, or even just use LLVM_LIBDIR_SUFFIX instead, since somebody could certainly override that if they really mean something different.

But I think it would be even better to eliminate these explicit checks and just let cmake do its thing and naturally find the needed cmake files, because you're going to end up with all kinds of corner cases like this if you're manually hunting them down. In other words, I think this error check is going to cause false failures more often than cmake itself will fail.

As a distribution packager, I look at potential packages like this: If I drop this in a clean system that already satisfies all the dependencies system-wide, can I build it? As long as they're not hidden, I'm happy to chase down those dependencies and package them directly, so they're available for any other software that needs them too.

@hvdijk
Copy link
Collaborator

hvdijk commented Mar 6, 2025

Technically yes, although the slightly better approach would be to initialize CA_LLVM_LIBDIR_SUFFIX from LLVM_LIBDIR_SUFFIX if it's not otherwise specified, or even just use LLVM_LIBDIR_SUFFIX instead, since somebody could certainly override that if they really mean something different.

I get where you're coming from, it would be nice if we could pick up the LLVM_LIBDIR_SUFFIX that is already set, but remember that we need this before we have included any LLVM CMake file, we need this in order to locate LLVMConfig.cmake. Therefore, we cannot automatically pick up any variable that is already specified inside LLVMConfig.cmake.

But I think it would be even better to eliminate these explicit checks and just let cmake do its thing and naturally find the needed cmake files,

I did have a look at that as well, but just doing it with find_package, while it would simplify the LLVM lookup by only needing LLVM_DIR be set, we then cannot use that same variable for also locating Clang and LLD, we would need the user to specify more variables than is needed now.

We also have a third option, which is to use CA_LLVM_INSTALL_DIR (and CA_LLVM_LIBDIR_SUFFIX or such) to set LLVM_DIR, Clang_DIR, LLD_Dir if it is provided, but then use regular find_package.

@hvdijk
Copy link
Collaborator

hvdijk commented Mar 6, 2025

With #705 I have managed to build on Clear Linux with

swupd bundle-add dev-utils
swupd bundle-add devpkg-SPIRV-Tools
cmake -G Ninja path/to/OCK/sources -DCA_LLVM_INSTALL_DIR=/usr -DCA_LLVM_LIBDIR_SUFFIX=64 -DCA_ENABLE_API=cl

I agree with you that this is not ideal. I will continue work on cleaning this up but am putting this up first to at least unblock you.

@bwarden
Copy link
Author

bwarden commented Mar 6, 2025

I agree with you that this is not ideal. I will continue work on cleaning this up but am putting this up first to at least unblock you.

Thank you! I'll pull the diff and try it once I get clspv built.

@bwarden
Copy link
Author

bwarden commented Mar 6, 2025

I should point out that we do set a handful of top-level variables for cmake, including:
https://github.com/clearlinux/autospec/blob/07a959cc837e5fc315f90364c04b8c2012d72002/autospec/specfiles.py#L470-L473

-DCMAKE_INSTALL_PREFIX=/usr
-DCMAKE_INSTALL_SBINDIR=/usr/bin
-DCMAKE_INSTALL_LIBDIR=/usr/lib64
-DLIB_INSTALL_DIR=/usr/lib64
-DLIB_SUFFIX=64

It would be handy if all the optional variables aren't set, the cmake config falls back to these system-level definitions.

@hvdijk
Copy link
Collaborator

hvdijk commented Mar 6, 2025

What I am working on, but don't have working correctly yet, is to do find_package(LLVM REQUIRED CONFIG HINTS ${CA_LLVM_INSTALL_DIR}), and likewise for Clang and LLD, which does not use any of those variables, but will still pick up a system LLVM without needing to specify additional options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants