[PATCH] D138258: clang/cmake: Fix incorrectly disabling tests when LLVM_EXTERNAL_LIT is used
Konrad Wilhelm Kleine via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 18 01:58:49 PST 2022
kwk requested changes to this revision.
kwk added a comment.
This revision now requires changes to proceed.
As much as I would like this to be fixed. I vote against this patch because in `lld/CMakeLists.txt` there's an almost (if not entirely) identical piece of code <https://github.com/llvm/llvm-project/blob/fcd5098a03dadcd11d4cc8b7155a4c07581999de/lld/CMakeLists.txt#L62> that screams to be outsourced into a `/cmake/Modules/FindLit.cmake` (to be created). I'll have a look at this and see if I can come up with a patch for this. Afterall `/cmake` is the central place to distribute share CMake code between subprojects, right @phosek (didn't you create `/cmake` in the first place?
Alternatively, we can copy the code from `lld/CMakeLists.txt` and outsource it afterwards so that the shared code becomes more obvious.
If there're any objections regarding outsourcing to `FindLit.cmake`, then I would only ask to copy the code from `lld/CMakeLists.txt`. There the `find_program` part sits below the if block.
================
Comment at: clang/CMakeLists.txt:97
# Seek installed Lit.
- find_program(LLVM_LIT
- NAMES llvm-lit lit.py lit
- PATHS "${LLVM_MAIN_SRC_DIR}/utils/lit"
- DOC "Path to lit.py")
+ if (NOT LLVM_EXTERNAL_LIT)
+ find_program(LLVM_EXTERNAL_LIT
----------------
mgorny wrote:
> I don't think you need to do this if you rename the var, `find_program()` doesn't seem to do any searching when the var is already set.
>
> I've tested with a dummy CMakeLists:
>
> ```
> set(FOO /bin/true)
> find_program(FOO NAMES foo)
> message(FATAL_ERROR ${FOO})
> ```
>
> gives `/bin/true` for me.
> I don't think you need to do this if you rename the var, `find_program()` doesn't seem to do any searching when the var is already set.
This might be true but it is counter intuitive to assume that `find_program` does nothing. So for readability I'd keep the `if (NOT LLVM_EXTERNAL_LIT)`. I know it is not needed but with the `if` it won't look like `find_program(LLVM_EXTERNAL_LIT` is the only/central place to set the variable `LLVM_EXTERNAL_LIT`. With the guard around it, it becomes more obvious that this is more of a fallback.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138258/new/
https://reviews.llvm.org/D138258
More information about the cfe-commits
mailing list