This is an archive of the discontinued LLVM Phabricator instance.

Harmonize cmake_policy() across standalone builds of all projects
ClosedPublic

Authored by mgorny on Oct 23 2022, 9:35 PM.

Details

Summary

Move cmake_policy() settings from llvm/CMakeLists.txt into a shared
cmake/modules/CMakePolicy.cmake. Include it from all relevant
projects that support standalone builds, in order to ensure that
the policies are consistently set whether they are built in-tree
or stand-alone.

Diff Detail

Event Timeline

mgorny created this revision.Oct 23 2022, 9:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2022, 9:35 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
mgorny requested review of this revision.Oct 23 2022, 9:35 PM
mgorny updated this revision to Diff 470108.Oct 24 2022, 3:55 AM
mgorny retitled this revision from [lld] Copy cmake_policy() to silence CMake warnings in standalone builds to Harmonize cmake_policy() across standalone builds of all projects.
mgorny edited the summary of this revision. (Show Details)
mgorny added reviewers: mehdi_amini, labath.
mgorny added subscribers: cfe-commits, lldb-commits.

Replace with one patch for all the projects.

clang/CMakeLists.txt
6

Seems error prone. Does cmake have an include system for including fragments from elsewhere? If so, that seems like a more concise and less error prone approach.

https://cmake.org/cmake/help/latest/command/include.html

mgorny added inline comments.Oct 24 2022, 10:03 AM
clang/CMakeLists.txt
6

I suppose that makes sense. I think that at this point all these components require the top-level cmake directory from monorepo anyway.

mgorny updated this revision to Diff 470225.Oct 24 2022, 11:11 AM
mgorny edited the summary of this revision. (Show Details)

Use a shared policy file via include() instead.

Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2022, 11:11 AM
mgorny marked an inline comment as done.Oct 24 2022, 11:11 AM
nickdesaulniers accepted this revision.Oct 24 2022, 11:48 AM
This revision is now accepted and ready to land.Oct 24 2022, 11:48 AM
MaskRay accepted this revision.Oct 24 2022, 12:11 PM

Thanks. Since this is not urgent, I'll let it wait for others to chime in a while more.

This revision was landed with ongoing or failed builds.Oct 27 2022, 4:47 AM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptOct 27 2022, 4:47 AM

This broke builds where clang/lld/lldb are symlinked into llvm/tools instead of specified via LLVM_ENABLE_PROJECTS - since ${CMAKE_CURRENT_SOURCE_DIR}/../cmake doesn't find anything in that context.

(The reason for wanting to stick to such a legacy setup, is that ccache caches can't be shared between workdirs for source files that are found outside of the base llvm-project/llvm directory - and I'm heavily reliant on ccache for keeping build times manageable in my dev workflow.)

This broke builds where clang/lld/lldb are symlinked into llvm/tools instead of specified via LLVM_ENABLE_PROJECTS - since ${CMAKE_CURRENT_SOURCE_DIR}/../cmake doesn't find anything in that context.

This aspect, for finding the shared cmake directory, is handled elsewhere by the main LLVM build setting the LLVM_COMMON_CMAKE_UTILS variable, and if not found, it's assumed to be in ${CMAKE_CURRENT_SOURCE_DIR}/... See e.g. https://github.com/llvm/llvm-project/blob/llvmorg-15.0.0/lld/CMakeLists.txt#L198-L206.

Since this is invoked very early in the per-project cmakefiles, I guess we can't assume to have all those variables set up yet, but e.g. something like this fixes the issue for me:

diff --git a/lld/CMakeLists.txt b/lld/CMakeLists.txt
index 964bc3fd8b17..530f52b05bac 100644
--- a/lld/CMakeLists.txt
+++ b/lld/CMakeLists.txt
@@ -1,6 +1,11 @@
 cmake_minimum_required(VERSION 3.13.4)
-include(${CMAKE_CURRENT_SOURCE_DIR}/../cmake/Modules/CMakePolicy.cmake
-  NO_POLICY_SCOPE)
+if(DEFINED LLVM_COMMON_CMAKE_UTILS)
+  include(${LLVM_COMMON_CMAKE_UTILS}/Modules/CMakePolicy.cmake
+    NO_POLICY_SCOPE)
+else()
+  include(${CMAKE_CURRENT_SOURCE_DIR}/../cmake/Modules/CMakePolicy.cmake
+    NO_POLICY_SCOPE)
+endif()
 
 # If we are not building as a part of LLVM, build LLD as an
 # standalone project, using LLVM as an external library:

(And the same applied for all the other subprojects.) It's admittedly not very pretty though...

I've reverted it right now, and I'll update the patch later today.

mgorny reopened this revision.Oct 27 2022, 8:15 AM
This revision is now accepted and ready to land.Oct 27 2022, 8:15 AM
mgorny updated this revision to Diff 471173.Oct 27 2022, 8:16 AM

Move LLVM_COMMON_CMAKE_UTILS earlier and use it for CMakePolicy path.

@mstorsjo, could you check this version?

It'll be good if we have a wiki describing how downstream users invoke cmake, so that large cmake refactoring can be verified beforehand.
(like usage verification https://github.com/opencollab/llvm-toolchain-integration-test-suite, but for cmake invocations)

mstorsjo accepted this revision.Oct 27 2022, 11:28 AM

@mstorsjo, could you check this version?

LGTM, this seems to work for my usecase (with lld,lldb,clang enabled this way). Thanks for the quick revert and fix!

Also for the record, I'd love to get rid of this symlink based setup, if I could make cmake produce project files where caching works in the same way. The main requirement for that would be to have the whole cmake build started from the toplevel llvm-project directory (so that all source from all subprojects are subdirectories to this), instead of the llvm-project/llvm directory.

Also for the record, I'd love to get rid of this symlink based setup, if I could make cmake produce project files where caching works in the same way. The main requirement for that would be to have the whole cmake build started from the toplevel llvm-project directory (so that all source from all subprojects are subdirectories to this), instead of the llvm-project/llvm directory.

To be honest, I also find it weird to start monorepo builds from within llvm subdirectory. Perhaps adding a top-level CMakeLists.txt that would work and wouldn't break people's workflows wouldn't be that hard, though.

This revision was landed with ongoing or failed builds.Oct 27 2022, 11:47 PM
This revision was automatically updated to reflect the committed changes.

Also for the record, I'd love to get rid of this symlink based setup, if I could make cmake produce project files where caching works in the same way. The main requirement for that would be to have the whole cmake build started from the toplevel llvm-project directory (so that all source from all subprojects are subdirectories to this), instead of the llvm-project/llvm directory.

To be honest, I also find it weird to start monorepo builds from within llvm subdirectory. Perhaps adding a top-level CMakeLists.txt that would work and wouldn't break people's workflows wouldn't be that hard, though.

This has been discussed briefly before - the main issue is that LLVM does some cmake trickery to recreate the installed directory structure (with executables in $(pwd)/bin etc) directly in the build folder, and if llvm-project/llvm is included from a toplevel llvm-project/CMakeLists.txt, this all ends up in $(pwd)/llvm/bin as things stand right now. See e.g. https://discourse.llvm.org/t/rfc-llvm-build-system-future-direction/53430/18. Hopefully it wouldn't be too hard to resolve - I had a brief look at it (just creating a stub CMakeLists.txt at the root and including the llvm subdirectory) a couple years ago, but didn't spend further time on looking into how things are set up and what it would take to fix it.