Use write_basic_package_version_file for LLVM

[alexreinking]

Summary

Use the CMake 3.13 features of CMakeConfigPackageHelpers to generate LLVMConfigVersion.cmake with proper architecture detection, major+minor version matching, etc.

Also adds the file to the build tree, which the comments suggested would happen, but wasn't.

[steveire]

Also adds the file to the build tree, which the comments suggested would happen, but wasn't.

I don't think this is true. Even before your patch, the generated LLVMConfigVersion.cmake is in ${CMAKE_BINARY_DIR}/lib/cmake/llvm which is where it should be? That location is what enables using find_package with the CMAKE_PREFIX_PATH pointing at the build dir.

Before this patch the generated file contains

set(PACKAGE_VERSION "13.0.0git")

by default (because LLVM_VERSION_SUFFIX is git by default), and after we have

set(PACKAGE_VERSION "13.0.0")

Please use PACKAGE_VERSION instead of LLVM_VERSION in the command call. This maintains the (good) goal of imitating the existing code as much as possible. Using

find_package(llvm 13.0.0git)

does not work anyway, but the user/vendor can set -DLLVM_VERSION_SUFFIX=.3 or something and then

find_package(llvm 13.0.0.3)

which should continue to work.

Please use PACKAGE_VERSION in the follow-up patch too.

I think using this command is the right direction, thanks!

llvm/cmake/modules/CMakeLists.txt

132

This doesn't seem necessary? It looks like only the "${llvm_cmake_builddir}/LLVMConfigVersion.cmake" one is required.

[alexreinking]

In D99451#2654142, @steveire wrote:

Also adds the file to the build tree, which the comments suggested would happen, but wasn't.

I don't think this is true. Even before your patch, the generated LLVMConfigVersion.cmake is in ${CMAKE_BINARY_DIR}/lib/cmake/llvm which is where it should be? That location is what enables using find_package with the CMAKE_PREFIX_PATH pointing at the build dir.

There are LLVMConfig.cmake files in both ${CMAKE_CURRENT_BINARY_DIR}/CMakeFiles/LLVMConfig.cmake and ${llvm_cmake_builddir}/LLVMConfig.cmake, so there should be LLVMConfigVersion.cmake files next to both as well.

I'll update the command to use PACKAGE_VERSION.

[alexreinking]

Use PACKAGE_VERSION instead

[steveire]

In D99451#2654218, @alexreinking wrote:

There are LLVMConfig.cmake files in both ${CMAKE_CURRENT_BINARY_DIR}/CMakeFiles/LLVMConfig.cmake and ${llvm_cmake_builddir}/LLVMConfig.cmake, so there should be LLVMConfigVersion.cmake files next to both as well.

Your conclusion is not correct.

The latter file is what find_package finds when instructed to look for the package in the build dir. The former file is generated with different content for the purpose of installing. [Putting it in the CMakeFiles directory is strange as that directory is arguably internal to the CMake implementation and third parties shouldn't put things there, but that's beside the point for this MR anyway.]

The point is that both of those files have a reason to exist and they have different content.

You are proposing creating a file in two locations with identical content. There is no need for both files you propose to exist. You are also proposing in the follow up patch to repeat that unneeded duplication unnecessarily for the the other projects in the repo.

Instead, we should

• not introduce that superfluous duplication here
• not repeat the duplication in other projects
• only generate the file in ${llvm_cmake_builddir}

The install(FILES command already correctly installs it from ${llvm_cmake_builddir}.

[alexreinking]

Remove LLVMConfigVersion from CMakeFiles tree

[alexreinking]

The former file is generated with different content for the purpose of installing. [Putting it in the CMakeFiles directory is strange as that directory is arguably internal to the CMake implementation and third parties shouldn't put things there, but that's beside the point for this MR anyway.]

This is what was confusing me. I have updated both diffs.

[steveire]

LGTM, thanks!

[phosek]

This broke the runtimes build with the following error:

CMake Warning at CMakeLists.txt:28 (find_package):
  Could not find a configuration file for package "LLVM" that is compatible
  with requested version "".

  The following configuration files were considered but not accepted:

    /b/s/w/ir/x/w/staging/llvm_build/lib/cmake/llvm/LLVMConfig.cmake, version: 13.0.0git (64bit)

Would it be possible to revert this change?

[alexreinking]

In D99451#2654963, @phosek wrote:

This broke the runtimes build with the following error:

CMake Warning at CMakeLists.txt:28 (find_package):
  Could not find a configuration file for package "LLVM" that is compatible
  with requested version "".

  The following configuration files were considered but not accepted:

    /b/s/w/ir/x/w/staging/llvm_build/lib/cmake/llvm/LLVMConfig.cmake, version: 13.0.0git (64bit)

Would it be possible to revert this change?

I am not able to reproduce this with the following command.

cmake -G Ninja -S runtimes -B runtimes/build -DLLVM_BINARY_DIR=/path/to/llvm/build

What command are you running?

[phosek]

We're using this cache file: https://github.com/llvm/llvm-project/blob/main/clang/cmake/caches/Fuchsia-stage2.cmake. You can see the full bot run here: https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang-linux-x64/b8851445615912583104/steps. I cannot reproduce this locally with CMake 3.18.4, but we're using 3.19.20210119 on the bot so it's possible that it's related to the CMake version.

[bjope]

We are also seeing problems with this when rebasing downstream. A somewhat strange thing was that it worked if I invoked the build a second time without removing my build directory in between. But with at clean (non existing) build dir I got failures.
I thought that it maybe was something wrong with my merge (or just a downstream problem), but if others also see problems with this then maybe not. Anyway, if I revert this patch things works fine again.

Just wanted to highlight that one might need to remove the build dir between tests when trying to reproduce the problem as my experience is that when invoking the build a second time it might pass. So some kind of dependency problem perhaps?

[phosek]

I tried doing a local build using the same version of CMake we use on our bots but it still doesn't reproduce so there must be some other factor.

In D99451#2655119, @bjope wrote:

We are also seeing problems with this when rebasing downstream. A somewhat strange thing was that it worked if I invoked the build a second time without removing my build directory in between. But with at clean (non existing) build dir I got failures.
I thought that it maybe was something wrong with my merge (or just a downstream problem), but if others also see problems with this then maybe not. Anyway, if I revert this patch things works fine again.

Just wanted to highlight that one might need to remove the build dir between tests when trying to reproduce the problem as my experience is that when invoking the build a second time it might pass. So some kind of dependency problem perhaps?

In our case we always do a clean build on bots so I'm not sure if it's the same issue.

[bjope]

In D99451#2655119, @bjope wrote:

We are also seeing problems with this when rebasing downstream. A somewhat strange thing was that it worked if I invoked the build a second time without removing my build directory in between. But with at clean (non existing) build dir I got failures.
I thought that it maybe was something wrong with my merge (or just a downstream problem), but if others also see problems with this then maybe not. Anyway, if I revert this patch things works fine again.

Just wanted to highlight that one might need to remove the build dir between tests when trying to reproduce the problem as my experience is that when invoking the build a second time it might pass. So some kind of dependency problem perhaps?

In our case we always do a clean build on bots so I'm not sure if it's the same issue.

It always fail for me on the clean build as well. I'm just saying that when I tried to bisect to find the offending commit I noticed that I needed to remove the build dir between the rebuilds, because even with this patch it happens to work when invoking the build twice (the build succeed on the second attempt even though it failed on the first attempt). Maybe it just happens to work that way for me, I don't know, but it was a bit of a surprise (and fooled me a bit at first when bisecting).

[alexreinking]

In trying to reproduce this, I have done the following.

1. I acquired every officially released CMake version between 3.13 and 3.20 (inclusive; latest patch versions only). I created symbolic links to each version, appropriately named cmake-${VER}. Notably, I did not test with the non-official CMake version 3.19.20210119 because I am not sure where to acquire it.
2. I freshly cloned the llvm-project repo from GitHub, revision b082e6f88acffe76841b9095ba024f585174b13a
3. I created a new directory adjacent to the repo
4. From this directory, I used every version of CMake to configure LLVM into separate directories.
5. Also from this directory, I used every version of CMake to configure runtimes into separate directories, and against every one of the aforementioned LLVM directories

For step 4, I ran the following command:

cmake-${CM_VER} -S ../llvm-project/llvm -B llvm/${CM_VER} -DLLVM_ENABLE_PROJECTS=all

For step 5, I ran the following command:

cmake-${RT_VER} -S ../llvm-project/runtimes -B runtimes/${CM_VER}/${RT_VER} -DLLVM_BUILD_DIR=$PWD/llvm/${CM_VER}

RT_VER is the CMake version used to configure runtimes and CM_VER is the CMake version used to configure LLVM.

In all cases, the first configure (ie. clean build) succeeds. I would be very happy to run this experiment again with different combination of commands, cache variables, etc. for either step.

[bjope]

Here are some diffs I get depending on having this patch or not:

> diff build-all-builtins/runtimes/runtimes-i386-unknown-linux-gnu-bins/CMakeCache.txt build-all-builtins.fail/runtimes/runtimes-i386-unknown-linux-gnu-bins/CMakeCache.txt
290c290,293
< COMPILER_RT_ENABLE_WERROR:BOOL=ON
---
> COMPILER_RT_ENABLE_WERROR:BOOL=OFF
>
> //Path where built compiler-rt executables should be stored.
> COMPILER_RT_EXEC_OUTPUT_DIR:PATH=/repo/llvm-project/llvm/build-all-builtins/runtimes/runtimes-i386-unknown-linux-gnu-bins/compiler-rt/bin
300c303,306
< COMPILER_RT_INCLUDE_TESTS:BOOL=ON
---
> COMPILER_RT_INCLUDE_TESTS:BOOL=OFF
>
> //Path where built compiler-rt libraries should be installed.
> COMPILER_RT_INSTALL_PATH:PATH=/compiler-clang
307a314,316
> //Path where built compiler-rt libraries should be stored.
> COMPILER_RT_OUTPUT_DIR:PATH=/repo/llvm-project/llvm/build-all-builtins/runtimes/runtimes-i386-unknown-linux-gnu-bins/compiler-rt
>
313a323,328
> //Compiler to use for testing
> COMPILER_RT_TEST_COMPILER:PATH=/repo/llvm-project/llvm/build-all-builtins/./bin/clang
>
> //C++ Compiler to use for testing
> COMPILER_RT_TEST_CXX_COMPILER:PATH=/repo/llvm-project/llvm/build-all-builtins/./bin/clang++
>
426c441
< LIBCXXABI_LIBCXX_LIBRARY_PATH:PATH=/repo/llvm-project/llvm/build-all-builtins/./lib/i386-unknown-linux-gnu/c++
---
> LIBCXXABI_LIBCXX_LIBRARY_PATH:PATH=/i386-unknown-linux-gnu/c++
805,807d819
< //Executor to use when running tests.
< LIBUNWIND_EXECUTOR:STRING=/app/vbuild/RHEL7-x86_64/python/3.8.0/bin/python3.8 /repo/llvm-project/libunwind/../libcxx/utils/run.py
<
847,849d858
< //TargetInfo to use when setting up test environment.
< LIBUNWIND_TARGET_INFO:STRING=libcxx.test.target_info.LocalTI
<
900c909
< LLVM_DIR:PATH=/repo/llvm-project/llvm/build-all-builtins/lib/cmake/llvm
---
> LLVM_DIR:PATH=/proj/flexasic/app/llvm/3.8/share/llvm/cmake
922a932,934
> //Enable compiler warnings.
> LLVM_ENABLE_WARNINGS:BOOL=ON
>
1148c1160
< CMAKE_NUMBER_OF_MAKEFILES:INTERNAL=47
---
> CMAKE_NUMBER_OF_MAKEFILES:INTERNAL=38

LLVM_DIR does not point to my build tree when it fails.
And LIBCXXABI_LIBCXX_LIBRARY_PATH looks quite different as it is an incomplete path when it fails.
Not sure if that is a clue or not, but seems a bit strange.

[phosek]

After multiple retries, the build failed for me locally:

CMake Error: Could not open file for write in copy operation /usr/lib/llvm-6.0/runtimes/armv7-unknown-linux-gnueabihf/Components.cmake.tmp
CMake Error: : System Error: No such file or directory
CMake Error at CMakeLists.txt:188 (configure_file):
  configure_file Problem configuring file

This is definitely unexpected because the build should never write anything under /usr/lib matching @bjope's observation that LLVM_DIR doesn't point to the build dir.

It's also worth pointing out that this doesn't fail on every run so it looks like there's some kind of build race that for some reason reproduces reliably on our bots.

@alexreinking can we please revert this change while we continue the investigation? It's now clear that there are some issues with this change and our bots have been broken for more than a day now.

llvm/cmake/modules/CMakeLists.txt

 set(LLVM_CONFIG_EXPORTS_FILE "\${LLVM_CMAKE_DIR}/LLVMExports.cmake")
 set(LLVM_CONFIG_EXPORTS "${LLVM_EXPORTS}")
 configure_file(
   LLVMConfig.cmake.in
   ${CMAKE_CURRENT_BINARY_DIR}/CMakeFiles/LLVMConfig.cmake
   @ONLY)
 
 # Generate LLVMConfigVersion.cmake for build and install tree.
-configure_file(
-  LLVMConfigVersion.cmake.in
-  ${llvm_cmake_builddir}/LLVMConfigVersion.cmake
-  @ONLY)
+include(CMakePackageConfigHelpers)
+write_basic_package_version_file(
+  "${CMAKE_CURRENT_BINARY_DIR}/CMakeFiles/LLVMConfigVersion.cmake"
+  VERSION "${PACKAGE_VERSION}"
+  COMPATIBILITY SameMinorVersion
+)
+write_basic_package_version_file(
+  "${llvm_cmake_builddir}/LLVMConfigVersion.cmake"
+  VERSION "${PACKAGE_VERSION}"
+  COMPATIBILITY SameMinorVersion
+)
 
 if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
   get_property(llvm_has_exports GLOBAL PROPERTY LLVM_HAS_EXPORTS)
   if(llvm_has_exports)
     install(EXPORT LLVMExports DESTINATION ${LLVM_INSTALL_PACKAGE_DIR}
             COMPONENT cmake-exports)
   endif()
 

llvm/cmake/modules/LLVMConfigVersion.cmake.in

-set(PACKAGE_VERSION "@PACKAGE_VERSION@")
-
-# LLVM is API-compatible only with matching major.minor versions
-# and patch versions not less than that requested.
-if("@LLVM_VERSION_MAJOR@.@LLVM_VERSION_MINOR@" VERSION_EQUAL
-    "${PACKAGE_FIND_VERSION_MAJOR}.${PACKAGE_FIND_VERSION_MINOR}"
-   AND NOT "@LLVM_VERSION_PATCH@" VERSION_LESS "${PACKAGE_FIND_VERSION_PATCH}")
-  set(PACKAGE_VERSION_COMPATIBLE 1)
-  if("@LLVM_VERSION_PATCH@" VERSION_EQUAL
-      "${PACKAGE_FIND_VERSION_PATCH}")
-    set(PACKAGE_VERSION_EXACT 1)
-  endif()
-endif()