[clang-tools-extra] Do not use `LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR`

[Abpostelnicu]

Summary

In D58157 LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR is set only when you enable clang-tools-extra but in D96787 has been added by default. This should be fixed and use the old variant with CMAKE_SOURCE_DIR, otherwise we have build failures when you don't specify clang-tools-extra in LLVM_ENABLE_PROJECTS even though it's going to be built.

[kadircet]

I am not an expert on Cmake nor understand what is going on here fully, but here are my 2 cents.

D58157 didn't remove LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR, it just stopped enabling CTE whenever clang was enabled.
All of LLVM_EXTERNAL_$PROJECT variables are set based on the values passed into LLVM_ENABLE_PROJECTS in https://github.com/llvm/llvm-project/blob/main/llvm/CMakeLists.txt#L105.

So in theory this cmake file should only be evaluated when CTE is enabled and LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR should always be set when CTE is enabled.
The correct fix would be to figure out why one of the above statements are not holding, i.e.:

• This cmake file is evaluated/executed despite CTE being off (this shouldn't be the case)
• LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR is not set, even though CTE is part of LLVM_ENABLE_PROJECTS.

[Abpostelnicu]

In D100625#2693971, @kadircet wrote:

I am not an expert on Cmake nor understand what is going on here fully, but here are my 2 cents.

D58157 didn't remove LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR, it just stopped enabling CTE whenever clang was enabled.
All of LLVM_EXTERNAL_$PROJECT variables are set based on the values passed into LLVM_ENABLE_PROJECTS in https://github.com/llvm/llvm-project/blob/main/llvm/CMakeLists.txt#L105.

So in theory this cmake file should only be evaluated when CTE is enabled and LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR should always be set when CTE is enabled.
The correct fix would be to figure out why one of the above statements are not holding, i.e.:

• This cmake file is evaluated/executed despite CTE being off (this shouldn't be the case)
• LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR is not set, even though CTE is part of LLVM_ENABLE_PROJECTS.

If you don't specify LLVM_ENABLE_PROJECTS clang-tools-extra will still be built but without having set LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR.

[kadircet]

If you don't specify LLVM_ENABLE_PROJECTS clang-tools-extra will still be built but without having set LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR.

This feels like the failure of the first case i mentioned, i.e. some cmake file under CTE being executed even though it is disabled. Can you provide some repro steps? For example when I do:

git clone [e-mail]:llvm/llvm-project.git
cd llvm-project && mkdir build_test && cd build_test
cmake -G Ninja ../llvm/ -DCMAKE_BUILD_TYPE="RELEASE"
ninja all

this finishes without any errors. as expected clang-tools-extra/clangd/quality/CompletionModel.cmake doesn't interfere with the build when clang-tools-extra is not mentioned in LLVM_ENABLE_PROJECTS.

---

also there are lots of build bots building different subsets of LLVM in different configurations and none of them have been broken by this change. so it would be great to understand the reason behind the breakage you are experiencing.

[Abpostelnicu]

In D100625#2694014, @kadircet wrote:

If you don't specify LLVM_ENABLE_PROJECTS clang-tools-extra will still be built but without having set LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR.

This feels like the failure of the first case i mentioned, i.e. some cmake file under CTE being executed even though it is disabled. Can you provide some repro steps? For example when I do:

git clone [e-mail]:llvm/llvm-project.git
cd llvm-project && mkdir build_test && cd build_test
cmake -G Ninja ../llvm/ -DCMAKE_BUILD_TYPE="RELEASE"
ninja all

this finishes without any errors. as expected clang-tools-extra/clangd/quality/CompletionModel.cmake doesn't interfere with the build when clang-tools-extra is not mentioned in LLVM_ENABLE_PROJECTS.

---

also there are lots of build bots building different subsets of LLVM in different configurations and none of them have been broken by this change. so it would be great to understand the reason behind the breakage you are experiencing.

On llvm main checkout from the github repo, I'm building it with the following:

cmake -GNinja -DCMAKE_C_COMPILER=/builds/worker/fetches/gcc/bin/gcc -DCMAKE_CXX_COMPILER=/builds/worker/fetches/gcc/bin/g++ -DCMAKE_ASM_COMPILER=/builds/worker/fetches/gcc/bin/gcc -DCMAKE_LINKER=/builds/worker/fetches/binutils/bin/ld -DCMAKE_AR=/builds/worker/fetches/binutils/bin/ar -DCMAKE_C_FLAGS= -DCMAKE_CXX_FLAGS= -DCMAKE_ASM_FLAGS= -DCMAKE_EXE_LINKER_FLAGS=-Wl,-Bsymbolic-functions -fuse-ld=gold -Wl,--gc-sections -Wl,--icf=safe -DCMAKE_SHARED_LINKER_FLAGS=-Wl,-Bsymbolic-functions -fuse-ld=gold -Wl,--gc-sections -Wl,--icf=safe -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/builds/worker/fetches/llvm-project/build/stage1/clang -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_ENABLE_ASSERTIONS=OFF -DPYTHON_EXECUTABLE=/usr/bin/python2.7 -DLLVM_TOOL_LIBCXX_BUILD=ON -DLLVM_ENABLE_BINDINGS=OFF  -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=WebAssembly -DLLVM_BINUTILS_INCDIR=/builds/worker/fetches/gcc/include -DLLVM_ENABLE_LIBXML2=FORCE_ON -DCMAKE_SYSROOT=/builds/worker/fetches/sysroot -DLLVM_LINK_LLVM_DYLIB=ON -DCMAKE_RANLIB=/builds/worker/fetches/binutils/bin/ranlib /builds/worker/fetches/llvm-project/llvm

As you can see I don't specify LLVM_ENABLE_PROJECTS but still looking at the build failure, it seems that clang-tools-extra is being built, or parts of it are:

ninja: error: '/clangd/quality/CompletionModelCodegen.py', needed by 'tools/clang/tools/extra/clangd/CompletionModel.cpp', missing and no known rule to make it

If I specify only clang or clang-tools-extra in LLVM_ENABLE_PROJECTS` this will not happen.

[Abpostelnicu]

In my case, this will never be true but, because I don't specify LLVM_ENABLE_PROJECTS so the set will not occur.

[kadircet]

In D100625#2694191, @Abpostelnicu wrote:

As you can see I don't specify LLVM_ENABLE_PROJECTS but still looking at the build failure, it seems that clang-tools-extra is being built, or parts of it are:

because I don't specify LLVM_ENABLE_PROJECTS so the set will not occur.

but the sample you provided has -DLLVM_ENABLE_PROJECTS=clang;compiler-rt in it, hence it doesn't really reproduce the issue.

[Abpostelnicu]

In D100625#2694236, @kadircet wrote:

In D100625#2694191, @Abpostelnicu wrote:

As you can see I don't specify LLVM_ENABLE_PROJECTS but still looking at the build failure, it seems that clang-tools-extra is being built, or parts of it are:

because I don't specify LLVM_ENABLE_PROJECTS so the set will not occur.

but the sample you provided has -DLLVM_ENABLE_PROJECTS=clang;compiler-rt in it, hence it doesn't really reproduce the issue.

Sorry I've posted the wrong command, If I specify DLLVM_ENABLE_PROJECTS="clang" this will not happen.

[jpalus]

Do you mind taking a look at fix proposed in:

https://bugs.llvm.org/show_bug.cgi?id=49990#c3

[hvdijk]

Apparently Phabricator detected the presence of "revert D100625" in my alternative D101851's description and marked that as reverting this one. Of course, as this hadn't been committed, it cannot have been reverted either. Could you confirm that things work for you now, @Abpostelnicu?

In D100625#2697345, @jpalus wrote:

Do you mind taking a look at fix proposed in:

https://bugs.llvm.org/show_bug.cgi?id=49990#c3

Sorry for not noticing this before, this is pretty much what I came up with too and what's committed now.

[Abpostelnicu]

In D100625#2741385, @hvdijk wrote:

Apparently Phabricator detected the presence of "revert D100625" in my alternative D101851's description and marked that as reverting this one. Of course, as this hadn't been committed, it cannot have been reverted either. Could you confirm that things work for you now, @Abpostelnicu?

In D100625#2697345, @jpalus wrote:

Do you mind taking a look at fix proposed in:

https://bugs.llvm.org/show_bug.cgi?id=49990#c3

Sorry for not noticing this before, this is pretty much what I came up with too and what's committed now.

It works just fine, thank you.

clang-tools-extra/clangd/quality/CompletionModel.cmake

-# Run the Completion Model Codegenerator on the model present in the 
+# Run the Completion Model Codegenerator on the model present in the
 # ${model} directory.
-# Produces a pair of files called ${filename}.h and  ${filename}.cpp in the 
+# Produces a pair of files called ${filename}.h and  ${filename}.cpp in the
 # ${CMAKE_CURRENT_BINARY_DIR}. The generated header
 # will define a C++ class called ${cpp_class} - which may be a
 # namespace-qualified class name.
 function(gen_decision_forest model filename cpp_class)
-  set(model_compiler ${LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR}/clangd/quality/CompletionModelCodegen.py)
+  set(model_compiler ${CMAKE_SOURCE_DIR}/../clang-tools-extra/clangd/quality/CompletionModelCodegen.py)
 
   set(output_dir ${CMAKE_CURRENT_BINARY_DIR})
   set(header_file ${output_dir}/${filename}.h)
   set(cpp_file ${output_dir}/${filename}.cpp)
 
   add_custom_command(OUTPUT ${header_file} ${cpp_file}
     COMMAND "${Python3_EXECUTABLE}" ${model_compiler}
       --model ${model}