Page MenuHomePhabricator

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

Authored by Abpostelnicu on Fri, Apr 16, 1:09 AM.

Details

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.

Diff Detail

Event Timeline

Abpostelnicu created this revision.Fri, Apr 16, 1:09 AM
Abpostelnicu requested review of this revision.Fri, Apr 16, 1:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Apr 16, 1:09 AM

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.

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.

Abpostelnicu edited the summary of this revision. (Show Details)Fri, Apr 16, 1:54 AM
kadircet added a comment.EditedFri, Apr 16, 2:07 AM

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 git@github.com: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 added a comment.EditedFri, Apr 16, 2:13 AM

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 git@github.com: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.

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

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.

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 added a subscriber: jpalus.Sun, Apr 18, 9:59 AM

Do you mind taking a look at fix proposed in:

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

hvdijk added a subscriber: hvdijk.Thu, May 6, 12:46 AM

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?

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 abandoned this revision.Thu, May 6, 1:51 AM

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?

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.