Skip to content

[runtimes] Allow use of external llvm-lit on standalone builds #144347

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

Merged
merged 1 commit into from
Jun 18, 2025

Conversation

pratlucas
Copy link
Contributor

When creating a standalone build of the runtimes sub-project, the
current CMake implementation looks for a lit executable that might
potentially exist in the build tree and unconditionally overrides the
value of LLVM_EXTERNAL_LIT. Due to this, any value passed via
-DLLVM_EXTERNAL_LIT when configuring the CMake project is ignored.
This change adds the ALLOW_EXTERNAL argument to the
get_llvm_lit_path call in the runtimes' CMakeLists.txt, allowing any
value previously set to be considered.

When creating a standalone build of the runtimes sub-project, the
current CMake implementation looks for a lit executable that might
potentially exist in the build tree and unconditionally overrides the
value of `LLVM_EXTERNAL_LIT`. Due to this, any value passed via
`-DLLVM_EXTERNAL_LIT` when configuring the CMake project is ignored.
This change adds the `ALLOW_EXTERNAL` argument to the
`get_llvm_lit_path` call in the runtimes' CMakeLists.txt, allowing any
value previously set to be considered.
@pratlucas pratlucas requested a review from a team as a code owner June 16, 2025 13:07
@llvmbot llvmbot added the cmake Build system in general and CMake in particular label Jun 16, 2025
@pratlucas pratlucas requested review from ldionne and arichardson June 16, 2025 13:10
Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

This looks fine to me, although the setting of LLVM_EXTERNAL_LIT_MISSING_WARNED_ONCE is incorrect if LLVM_EXTERNAL_LIT was already set when entering this file. Probably doesn't matter since people who manually set LLVM_EXTERNAL_LIT probably know what they are doing and if it is set to something invalid, then we just fail a bit later in the configure process instead.

@pratlucas
Copy link
Contributor Author

Thanks for the review!

When ALLOW_EXTERNAL is used, the get_llvm_lit_path function itself will check if the path provided exists and issue the warning, so I believe it's still ok the set LLVM_EXTERNAL_LIT_MISSING_WARNED_ONCE after that call:

if (NOT "${LLVM_EXTERNAL_LIT}" STREQUAL "")
if (EXISTS ${LLVM_EXTERNAL_LIT})
get_filename_component(LIT_FILE_NAME ${LLVM_EXTERNAL_LIT} NAME)
get_filename_component(LIT_BASE_DIR ${LLVM_EXTERNAL_LIT} DIRECTORY)
set(${file_name} ${LIT_FILE_NAME} PARENT_SCOPE)
set(${base_dir} ${LIT_BASE_DIR} PARENT_SCOPE)
return()
elseif (NOT DEFINED CACHE{LLVM_EXTERNAL_LIT_MISSING_WARNED_ONCE})
message(WARNING "LLVM_EXTERNAL_LIT set to ${LLVM_EXTERNAL_LIT}, but the path does not exist.")
set(LLVM_EXTERNAL_LIT_MISSING_WARNED_ONCE YES CACHE INTERNAL "")
endif()
endif()

@arichardson
Copy link
Member

Thanks for the review!

When ALLOW_EXTERNAL is used, the get_llvm_lit_path function itself will check if the path provided exists and issue the warning, so I believe it's still ok the set LLVM_EXTERNAL_LIT_MISSING_WARNED_ONCE after that call:

if (NOT "${LLVM_EXTERNAL_LIT}" STREQUAL "")
if (EXISTS ${LLVM_EXTERNAL_LIT})
get_filename_component(LIT_FILE_NAME ${LLVM_EXTERNAL_LIT} NAME)
get_filename_component(LIT_BASE_DIR ${LLVM_EXTERNAL_LIT} DIRECTORY)
set(${file_name} ${LIT_FILE_NAME} PARENT_SCOPE)
set(${base_dir} ${LIT_BASE_DIR} PARENT_SCOPE)
return()
elseif (NOT DEFINED CACHE{LLVM_EXTERNAL_LIT_MISSING_WARNED_ONCE})
message(WARNING "LLVM_EXTERNAL_LIT set to ${LLVM_EXTERNAL_LIT}, but the path does not exist.")
set(LLVM_EXTERNAL_LIT_MISSING_WARNED_ONCE YES CACHE INTERNAL "")
endif()
endif()

Ah yes, sounds good to me then :)

@pratlucas pratlucas merged commit 6fcdde2 into llvm:main Jun 18, 2025
61 of 64 checks passed
@pratlucas pratlucas deleted the runtimes-external-lit branch June 18, 2025 09:26
fschlimb pushed a commit to fschlimb/llvm-project that referenced this pull request Jun 18, 2025
…144347)

When creating a standalone build of the runtimes sub-project, the
current CMake implementation looks for a lit executable that might
potentially exist in the build tree and unconditionally overrides the
value of `LLVM_EXTERNAL_LIT`. Due to this, any value passed via
`-DLLVM_EXTERNAL_LIT` when configuring the CMake project is ignored.
This change adds the `ALLOW_EXTERNAL` argument to the
`get_llvm_lit_path` call in the runtimes' CMakeLists.txt, allowing any
value previously set to be considered.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Build system in general and CMake in particular
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants