This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Determine early on which projects are enabled
ClosedPublic

Authored by zturner on Sep 8 2017, 12:01 PM.

Details

Summary

Some projects need to add conditional dependencies on other projects. compiler-rt is already doing this, and I attempted to add this to debuginfo-tests when I ran into the ordering problem, that you can't conditionally add a dependency unless that dependency's CMakeLists.txt has already been run (which would allow you to say if (TARGET foo).

The solution to this seems to be to determine very early on the entire set of projects which is enabled. This is complicated by the fact that there are multiple ways to enable projects, and different tree layouts (e.g. mono-repo, out of -tree, external, etc). This patch attempts to centralize all of this into one place, and then updates compiler-rt to demonstrate as a proof of concept how this can simplify code.

Diff Detail

Event Timeline

zturner created this revision.Sep 8 2017, 12:01 PM
beanz added inline comments.Sep 8 2017, 1:33 PM
llvm/CMakeLists.txt
838

In general the goal is to move away from the <llvm>/projects directory in favor of <llvm>/runtimes or <llvm>/tools in the future. At the very least you'll need to support <llvm>/runtimes here too.

zturner updated this revision to Diff 114426.Sep 8 2017, 1:43 PM
zturner added a reviewer: eugenis.

Updated to look in runtimes. Also +eugenis since I'm touching compiler-rt CMake, even though this is supposed to be NFC. Note that with this change, compiler-rt could in theory just delete its custom variables and use LLVM_PROJECT_LLD_ENABLED everywhere instead of COMPILER_RT_HAS_LLD etc. But that is a bit more invasive change which I don't want to make.

eugenis edited edge metadata.Sep 8 2017, 1:55 PM

compiler-rt changes look great!

llvm/cmake/modules/AddLLVM.cmake
929

The code in llvm/projects treats any subdirectory with CMakeLists.txt as a project. Could we do the same here, instead of listing all (or some?) known projects?

932

Might be a good idea to warn if a project is found in two places at once.

zturner updated this revision to Diff 114436.Sep 8 2017, 2:55 PM

Updated to look for all projects in the runtimes and projects folder.

Also worth mentioning is that projects/CMakeLists.txt seems to have some hackish code that appears to be a workaround for the fact the feature I'm adding in this patch doesn't exist. Specifically, it first adds a bunch of known projects that don't depend on each other in a random order. Then it adds a couple of specific projects that do depend on each other in the right order so that they can use if (TARGET foo) to check for existence.

After this patch, the entire set of valid projects that are present in the tree can probably be added in any order, as long as each CMakeLists.txt is updated to use the proper LLVM_PROJECT_foo_ENABLED variable, rather than an if (TARGET foo) check.

This revision was automatically updated to reflect the committed changes.
pdouglas added a subscriber: pdouglas.EditedSep 13 2017, 2:45 AM

Hi, since this change we're seeing a Cmake error (see below) when we run it with -DLLVM_TOOL_LLD_BUILD=OFF but with tools/lld checked out. If either that argument isn't passed or the lld directory isn't present, it runs fine.

-- Configuring done
CMake Error at cmake/modules/AddLLVM.cmake:1241 (add_dependencies):
  The dependency target "lld" of target "check-all" does not exist.
Call Stack (most recent call first):
  CMakeLists.txt:933 (add_lit_target)


CMake Error at cmake/modules/AddLLVM.cmake:1241 (add_dependencies):
  The dependency target "lld" of target "check-cfi" does not exist.
Call Stack (most recent call first):
  cmake/modules/AddLLVM.cmake:1262 (add_lit_target)
  projects/compiler-rt/test/cfi/CMakeLists.txt:80 (add_lit_testsuite)


CMake Error at cmake/modules/AddLLVM.cmake:1241 (add_dependencies):
  The dependency target "lld" of target "check-cfi-and-supported" does not
  exist.
Call Stack (most recent call first):
  projects/compiler-rt/test/cfi/CMakeLists.txt:84 (add_lit_target)


CMake Error at cmake/modules/AddLLVM.cmake:1241 (add_dependencies):
  The dependency target "lld" of target "check-asan" does not exist.
Call Stack (most recent call first):
  cmake/modules/AddLLVM.cmake:1262 (add_lit_target)
  projects/compiler-rt/test/asan/CMakeLists.txt:161 (add_lit_testsuite)


CMake Error at cmake/modules/AddLLVM.cmake:1241 (add_dependencies):
  The dependency target "lld" of target "check-asan-dynamic" does not exist.
Call Stack (most recent call first):
  cmake/modules/AddLLVM.cmake:1262 (add_lit_target)
  projects/compiler-rt/test/asan/CMakeLists.txt:167 (add_lit_testsuite)