Page MenuHomePhabricator

[CMake] Add <Project>ConfigVersion.cmake files
AcceptedPublic

Authored by alexreinking on Feb 25 2021, 4:31 PM.

Details

Summary

When supplying a package with <Project>Config.cmake files, it is required to also supply a <Project>ConfigVersion.cmake file which defines compatibility with version numbers. If these files aren't distributed then supplying a version number will fail. But on the other hand, not supplying a version number can result in the wrong package being selected. For example, because the base LLVM package does supply LLVMConfigVersion.cmake, the following can occur:

find_package(LLVM 11.0 REQUIRED)  # Succeeds
find_package(LLD 11.0)  # Fails because there's no version config
find_package(LLD)  # Succeeds, finds version 10.

This patch would not affect any existing uses of the CMake build/packages because the ConfigVersion.cmake file is ignored when no version is given and the status quo is that supplying a version fails.

This patch also fixes a bug I opened a year ago here: https://bugs.llvm.org/show_bug.cgi?id=45027

Diff Detail

Event Timeline

alexreinking created this revision.Feb 25 2021, 4:31 PM
alexreinking requested review of this revision.Feb 25 2021, 4:31 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 25 2021, 4:31 PM

So with this patch, I tried to imitate the existing code as much as possible. However, I think it would be quite a bit better to use the write_basic_package_version_file command from CMakePackageConfigHelpers. It gained support for major + minor version compatibility matching in CMake 3.11. Since LLVM is on 3.13 now, I don't see a reason not to use this. Instead of the existing configure_file and *ConfigVersion.cmake.in combo, we would delete the template (.in) and just write:

include(CMakePackageConfigHelpers)
write_basic_package_version_file(
    ${CMAKE_CURRENT_BINARY_DIR}/CMakeFiles/PollyConfigVersion.cmake
    VERSION ${POLLY_VERSION}
    COMPATIBILITY SameMinorVersion
)

If the subproject had proper calls to project(), like project(Polly VERSION ${POLLY_VERSION}), then write_basic_package_version_file would default its VERSION argument to ${PROJECT_VERSION}.

The template ConfigVersion.cmake.in files are also missing an architecture check, so it's possible that a 64-bit build of LLVM could get picked up by a project calling find_package while cross compiling to 32-bit. The ones generated by write_basic_package_version_file have this check.

steveire added inline comments.
lld/cmake/modules/LLDConfigVersion.cmake.in
1 ↗(On Diff #326547)

Did you consider using write_basic_package_version_file instead of adding these files?

steveire added inline comments.Fri, Mar 26, 3:18 PM
lld/cmake/modules/LLDConfigVersion.cmake.in
1 ↗(On Diff #326547)

Ah, sorry, I see you did and wrote a comment about it.

Uses write_basic_package_version_file

Use PACKAGE_VERSION instead

Drop *ConfigVersion.cmake files from CMakeFiles tree

Remove unused version variables (that were used in a previous revision)

Update & wrap commit message

steveire added inline comments.Sat, Mar 27, 3:00 PM
flang/cmake/modules/CMakeLists.txt
15

Instead of including this multiple times, please add it to AddLLVM.cmake. That file is used in project top-level builds too and it already contains other includes needed in the build. It is installed, but only for the purpose of building other parts of LLVM, so it's not a concern for third parties either (It is not included by LLVMConfig.cmake).

alexreinking retitled this revision from Add <Project>ConfigVersion.cmake files to [CMake] Add <Project>ConfigVersion.cmake files.

As requested, move instances of include(CMakePackageConfigHelpers) to AddLLVM.cmake.

Herald added a project: Restricted Project. · View Herald TranscriptSat, Mar 27, 3:05 PM
alexreinking marked an inline comment as done.Sat, Mar 27, 3:06 PM
steveire accepted this revision.Sat, Mar 27, 3:09 PM

LGTM, thanks!

I'll commit this on Wednesday if none of the other reviewers object by then.

This revision is now accepted and ready to land.Sat, Mar 27, 3:09 PM
mehdi_amini accepted this revision.Sat, Mar 27, 7:40 PM
srj added a subscriber: srj.Tue, Mar 30, 2:42 PM