This is an archive of the discontinued LLVM Phabricator instance.

[standalone-build] outsource, simplify and unify repetitive CMake code
Needs ReviewPublic

Authored by kwk on Jan 5 2023, 6:18 AM.

Details

Summary

See also this topic on discourse: https://discourse.llvm.org/t/an-opinionated-way-to-outsource-simplify-and-unify-repetitive-cmake-code-in-standalone-build-mode/67508

Rationale

This is an opinionated change for the standalone build mode CMake code.

The rationale behind this change is to unify the parts of standalone
builds that historically had to be kept in each and every project. With
the advent of the top-level cmake directory inside the LLVM source
tree, it is now possible to bundle the CMake instructions into a file,
aka cmake/Modules/StandaloneBuildHelpers.cmake, and include that file
in each project that wants to build in standalone-mode.

Historically the standalone build mode is used mostly by Linux
distributions. Certainly not every LLVM contributor cares about Linux
distributions. To reduce the frictions it makes even more sense to have
a unified place where to keep the specialities of building in standalone
mode.

Affected projects (so far)

This change brings the unified standalone build mode to the clang and
lld project.

Assumptions

One radical assumption for this change is that in order to build clang
or ldd in standalone mode, you have to first build the llvm subproject
and *install* it into any location. You can assist the build process to
find LLVM using find_package(LLVM) by specifying
-DCMAKE_PREFIX_PATH=${LLVM_INSTALL_DIR}/lib/cmake/llvm in the cmake
configuration process.
You have to build the `llvm subproject with utilies included and
installed
(-DLLVM_INCLUDE_UTILS:BOOL=ON and -DLLVM_INSTALL_UTILS:BOOL=ON. But
I'm sure that this is done most of the time anyways, no?

Don't build as you go: No more cross-project dependencies on LLVM utilties

Another assumption is that in standalone build mode it makes no sense to
build clang and try to build an LLVM utility binary like FileCheck if
that is missing. This only adds noise to the cmake files and creates an
indirect dependency on the LLVM utilities directory which doesn't exist
in the the clang source tarball. Therefore we go and search for

Don't silently turn off tests

Before this change, we would silently turn off tests if a binary like
FileCheck, count or not was missing. This is not only dangerous
but IMHO not helpful. If someone asks for tests by passing
-DLLVM_INCLUDE_TESTS=On we should error out at configure time because
we cannot fulfil this request when a binary is missing. This is exactly
what this tests does. If you want to check if an LLVM utility binary
exists and what the path to it is, you can call
get_llvm_utility_binary_path("FileCheck" "FileCheck_EXE") and it will
get the import location for the target, aka the path to the binary that
was found when we did find_package(LLVM).

NOTE: You can take a look at this small example project which shows you how importing of an installed project works: https://github.com/kwk/cmake-export-binary-example/blob/2b429fccef97eb93c1717ceddb27bbe0022339d2/project-b/CMakeLists.txt#L7-L10

Require external LIT in standalone mode

We also think that in standalone mode you always want to use an external
lit and not build it as you go. That's why the find_external_lit macro
checks if LLVM_EXTERNAL_LIT is set and the path exists. If one of
these conditions doesn't hold true, we error out.

TODO:

( ) make sure the correct binaries of FileCheck and count and not
get substituted in lit test files.
( ) get feedback on this change or just opinions
( ) extend usage to other projects like mlir, libomp and so on.
( ) more encapsulation in cmake/Modules/StandaloneBuildHelpers.cmake

Diff Detail

Event Timeline

kwk created this revision.Jan 5 2023, 6:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2023, 6:18 AM
kwk requested review of this revision.Jan 5 2023, 6:18 AM
kwk edited the summary of this revision. (Show Details)Jan 5 2023, 7:07 AM
phosek added inline comments.Jan 6 2023, 7:57 PM
clang/CMakeLists.txt
35

It's somewhat unusual to quote output variable names in our CMake files, I'd prefer to follow existing conventions for consistency.

cmake/Modules/StandaloneBuildHelpers.cmake
13–20

I'd omit this, I don't think it's necessary to restrict the usage of this module.

26–31

Do you know why is this needed or this just copy pasted from Clang? I'd consider dropping this altogether (ideally in a separate change).

46–47

This shouldn't be needed since you've already had to insert the module path manually.

62

We use 2 spaces for indentation in our CMake files.

83–110

Is there any reason to keep these two macros separate? Could we combine them?

lld/CMakeLists.txt
57

Where are these variables being used?

kwk updated this revision to Diff 487750.Jan 10 2023, 4:37 AM
kwk marked 2 inline comments as done.
  • Don't quote output variables
  • Use 2 space indention in CMake files
  • Make out_var optional for get_llvm_utility_binary_path
  • Rename get_llvm_utility_binary_path -> require_llvm_utility_binary_path
kwk added a comment.Jan 10 2023, 4:37 AM

Thank you @phosek for your review! I really appreciate it. I've addressed all of your comments. Could you give it another review?

clang/CMakeLists.txt
35

I obey to the consistency.

cmake/Modules/StandaloneBuildHelpers.cmake
13–20

I think restricting the usage makes it clear for what it is. And right now this CMake file does stuff just when you include it which not a good thing in terms of isolation. If you don't mind I'd like to keep this restriction and only remove it before this patch lands in a better shape. Okay?

26–31

Honestly, I started to move the CMake code from clang over to this file piece by piece once I found that the exact or similar formatted code existed in other projects like lld. But you caught me red-handed :).

This is the patch that introduced this particular section: e6d79ec0ebac350355c76d8132e9f1cce62e0cac. It is from 2013.

I'm not so long involved in the llvm project that I can give a solid answer to this but hasn't llvm-config been deprecated for building LLVM itself? If that is the case, I'm happy to remove this section from the clang/CMakeLists.txt in the first place in a separate patch.

46–47

I don't see what the one has to do with the other immediately. Maybe I'm a fool. The CMAKE_INCLUDE_CURRENT_DIR is for including C/C++ files and not CMake files so it doesn't matter what I have in my module path, no?. Here's a little example project I created to proof to myself that my assumption is really true: https://github.com/kwk/cmake-example-CMAKE_INCLUDE_CURRENT_DIR . Just run make once you've clone it. Then comment out this line to see the effect: include(EnableCurrentIncludeDir).

62

I've fixed the indention to use 2 spaces.

83–110

Yes, to both of your questions :) . Yes, I find it easier to read the files that include this file and use the functions and macros when they are separated. And yes, we could combine them to setup_external_lit or something alike. But for the course of this patch I'd like to keep them separated and do the merge later if you don't mind.

lld/CMakeLists.txt
57

They were not being used, yet. I forgot to make the second parameter of get_llvm_utility_binary_path an optional one. Now it is optional and you can omit the second parameter. I've also renamed the function to require_llvm_utility_binary_path to convey the importance that the utility exists through the name of the function.

Also, I'd like to refer to the TODO in this patch's description:

make sure the correct binaries of FileCheck and count and not

get substituted in lit test files.

I'm quite positive that lit will use the correct binaries because we've imported the targets but still I wanna be sure. Having said that, you should not need the path except for LLVM_TABLEGEN_EXE. That variable existed before and I worked my function around it to fit this usecase.

kwk updated this revision to Diff 487826.Jan 10 2023, 8:57 AM
  • Make require_llvm_utility_binary_path usable from a build-tree setup
kwk updated this revision to Diff 488582.Jan 12 2023, 3:15 AM
  • both, build tree and install method support find_package by setting different LLVM_CMAKE_DIR