Incorrect re2Config.cmake.in for CMake 3.29

[moubctez]

The expansion of @PACKAGE_INIT@ has changed in CMake 3.29. Now ${PACKAGE_PREFIX_DIR} is not defined anymore, so re2Config.cmake.in fails to configure the package correctly. The following patch fixes the problem.

--- re2Config.cmake.in.orig	2024-04-05 16:12:34.658881208 +0000
+++ re2Config.cmake.in
@@ -6,7 +6,7 @@
 
 include(CMakeFindDependencyMacro)
 
-set_and_check(re2_INCLUDE_DIR ${PACKAGE_PREFIX_DIR}/@CMAKE_INSTALL_INCLUDEDIR@)
+set_and_check(re2_INCLUDE_DIR @CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_INCLUDEDIR@)
 
 if(UNIX)
   set(THREADS_PREFER_PTHREAD_FLAG ON)

[junyer]

Thanks for the patch!

The use of ${PACKAGE_PREFIX_DIR} was introduced in commit cb5bbb2. @bencsikandrei, could you please chime in regarding whether you think @CMAKE_INSTALL_PREFIX@ will be okay on Linux, macOS and Windows? The CMakePackageConfigHelpers documentation says:

This has the effect that the resulting FooConfig.cmake file would work poorly under Windows and OSX, where users are used to choose the install location of a binary package at install time, independent from how CMAKE_INSTALL_PREFIX was set at build/cmake time.

I must admit that I can't read CMake well enough to grok what's going on in commit Kitware/CMake@6ddf871, but should we be setting PATH_VARS and referring to @PACKAGE_…@? (FWIW, https://gitlab.kitware.com/cmake/cmake/-/issues/25827 isn't enlightening me either.)

[bencsikandrei]

Hello,

I've only had time fo a quick look at the docs and changes and they seems to indeed need that variable removal - which they used to recommend
by the way; there seems to be @PACKAGE_INCLUDE_INSTALL_DIR@ now (see the example in version 3
29 - bottom of the configure package doc)

Edit: actually seems that that was there for a long time...

I'll try to give a better response a tad later. Maybe that's the new cool thing to use.

I'll analyze further as soon as I have a laptop on hand.

[bencsikandrei]

Back with some more insights: it seems these are a valid choice:
(took the ideas from Craig's commit which has unit tests: https://gitlab.kitware.com/cmake/cmake/-/issues/25827)

we also need to use the PATH_VARS thing inside the cmake configure file command: https://cmake.org/cmake/help/v3.28/module/CMakePackageConfigHelpers.html#example-generating-package-files

I'll try to get my hands on a cmake 3.29.1 cause my 3.29 doesn't have the change made by Craig
EDIT: checked/tested and works fine on 3.29.1, here's a rough patch:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index bdac5af..90024a0 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -232,13 +232,15 @@ if(RE2_BUILD_TESTING)
   endforeach()
 endif()
 
+set(re2_INCLUDE_INSTALL_DIR ${CMAKE_INSTALL_INCLUDEDIR}/re2)
+
 install(TARGETS re2
         EXPORT re2Targets
         ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
         LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
         RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
         FRAMEWORK DESTINATION ${CMAKE_INSTALL_LIBDIR}
-        PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/re2
+        PUBLIC_HEADER DESTINATION ${re2_INCLUDE_INSTALL_DIR}
         INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
 install(EXPORT re2Targets
         DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/re2
@@ -246,7 +248,8 @@ install(EXPORT re2Targets
 
 configure_package_config_file(${CMAKE_CURRENT_SOURCE_DIR}/re2Config.cmake.in
                               ${CMAKE_CURRENT_BINARY_DIR}/re2Config.cmake
-                              INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/re2)
+                              INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/re2
+                              PATH_VARS re2_INCLUDE_INSTALL_DIR)
 write_basic_package_version_file(${CMAKE_CURRENT_BINARY_DIR}/re2ConfigVersion.cmake
                                  VERSION ${SONAME}.0.0
                                  COMPATIBILITY SameMajorVersion)
diff --git a/re2Config.cmake.in b/re2Config.cmake.in
index 6a177c6..951449a 100644
--- a/re2Config.cmake.in
+++ b/re2Config.cmake.in
@@ -6,7 +6,7 @@
 
 include(CMakeFindDependencyMacro)
 
-set_and_check(re2_INCLUDE_DIR ${PACKAGE_PREFIX_DIR}/@CMAKE_INSTALL_INCLUDEDIR@)
+set_and_check(re2_INCLUDE_DIR "@PACKAGE_re2_INCLUDE_INSTALL_DIR@")
 
 if(UNIX)
   set(THREADS_PREFER_PTHREAD_FLAG ON)

I reckon we should AVOID the CMAKE_INSTALL_PREFIX and use these to ensure relocatable. There are other open source projects using these.

Hope it helps,

Cheers!

[junyer]

Many thanks for digging into this! :D

My apologies if this is a silly question, but... the FindPkgConfig documentation mentions <XXX>_INCLUDE_DIRS and <YYY>_INCLUDEDIR, so does re2_INCLUDE_DIR follow a third naming convention and, more to the point, does anything actually require it to exist? Which is to say, could we simply delete that line?

[bencsikandrei]

hello,

I just threw a random name there TBH (that at least seemed correct); it should adhere to the common practice I guess, unsure which that is, I'd have to dig a bit.

EDIT: this seems to recommend <NAME>_INCLUDE_DIR; but as you mentioned I think using 'targets' you get what you need from those, so that variable might as well not exist

[junyer]

Thanks for confirming! I will go ahead and delete that line. We can always revisit, obviously, if it turns out to be necessary for something.