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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
