This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Only detect the linker once in AddLLVM.cmake
ClosedPublic

Authored by ldionne on Oct 8 2019, 8:07 AM.

Details

Summary

Otherwise, the build output contains a bunch of "Linker detection: <xxx>"
lines that are really redundant. We also make redundant calls to the
linker, although that's a smaller concern.

Event Timeline

ldionne created this revision.Oct 8 2019, 8:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2019, 8:07 AM
smeenai accepted this revision.Oct 8 2019, 6:22 PM

This only works because of a specific detail of LLVM's build system. The variable will get set in the directory scope, so different directories including this module will still duplicate the check. Here's a simple test I did, using cmake version 3.12.1 (run this as a script):

mkdir /tmp/cmaketest
cd /tmp/cmaketest
cat > CMakeLists.txt <<'EOF'
cmake_minimum_required(VERSION 3.4.3)
list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR}/modules)
add_subdirectory(dir1)
add_subdirectory(dir2)
EOF

mkdir modules
cat > modules/detect.cmake <<'EOF'
if(NOT DEFINED DETECTED)
  message(STATUS Detected)
  set(DETECTED YES)
endif()
EOF

mkdir dir1
cat > dir1/CMakeLists.txt <<'EOF'
include(detect)
add_subdirectory(subdir)
EOF

mkdir dir1/subdir
echo 'include(detect)' > dir1/subdir/CMakeLists.txt

mkdir dir2
echo 'include(detect)' > dir2/CMakeLists.txt

mkdir build
cd build
cmake -G Ninja ..

For me, the "Detected" message is still printed twice, for dir1 and dir2. It doesn't print for subdir, since that inherits the variable for dir1, but independent directories still duplicate the message.

In LLVM, we include(AddLLVM) in the top-level CMakeLists.txt, so any variable created in that directory scope is essentially global. Nevertheless, it'd be nicer to explicitly use a global property for this, but I'm also okay with this going in as-is if you prefer.

This revision is now accepted and ready to land.Oct 8 2019, 6:22 PM
ldionne updated this revision to Diff 244954.Feb 17 2020, 5:57 AM

Use a global variable to store the variables, per @smeenai's comment.

@smeenai I'm now using CACHE variables, which should mean the detection is performed exactly once per build.

Works for me!

(my previous accept still stands)

This revision was automatically updated to reflect the committed changes.