cmake: Install cmake files in a more common path #366

diizzyy · 2023-12-30T09:57:25Z

Install cmake files in prefix/lib/cmake/pcre2 rather than prefix/cmake

carenas

there was an unmerged proposal before to change this only for POSIX systems (where it makes more sense as well, as it allows correctly detecting the right bitness for PCRE2 in multilib systems)

carenas · 2023-12-30T15:51:09Z

CMakeLists.txt

@@ -1124,7 +1124,7 @@ configure_file(${PCRE2_CONFIG_IN} ${PCRE2_CONFIG_OUT} @ONLY)
 set(PCRE2_CONFIG_VERSION_IN  ${CMAKE_CURRENT_SOURCE_DIR}/cmake/pcre2-config-version.cmake.in)
 set(PCRE2_CONFIG_VERSION_OUT ${CMAKE_CURRENT_BINARY_DIR}/cmake/pcre2-config-version.cmake)
 configure_file(${PCRE2_CONFIG_VERSION_IN} ${PCRE2_CONFIG_VERSION_OUT} @ONLY)
-install(FILES ${PCRE2_CONFIG_OUT} ${PCRE2_CONFIG_VERSION_OUT} DESTINATION cmake)
+install(FILES ${PCRE2_CONFIG_OUT} ${PCRE2_CONFIG_VERSION_OUT} DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/pcre2)


there used to be a time (at least in Linux) when CMAKE_INSTALL_LIBDIR was lib64 for 64bit builds, and I presume avoiding that variance made using a "bitness" agnostic directory, attractive.

I also wonder how that would work in Windows, where the path is usually just "cmake" (usually with some funny case ignoring but preserving variant).

could we check with the rest of the "cmake communitiy" around #115 for advice before merging this backward incompatible change?

https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure suggests that this is viable on both Windows and Unix

agree it is "viable", the point I was making was how to minimize "backward compatibility" issues

ex: currently it "might" fail to allow locatiing the cmake files if ${CMAKE_INSTALL_LIBDIR} is (arguibly incorrectly) set to lib64 and the system is Windows. Making this change ONLY if not Windows would "solve" that and IMHO was simple enough to do as shown by:

teo-tsirpanis#1 (comment)

I'm not sure what you really want as several have suggested the same thing by now
https://github.com/PCRE2Project/pcre2/pull/260/files#r1225428249

funny enough you linked to my "ask", which was "what can we put in the release notes to make sure whoever might be affected by this change fixes their setup".

I don't maintan any software that uses pcre2 and that would be using this code, so I am not the best person to provide that.

Indeed, I dont't even know how to create an application that could be affected, but assume one running Windows that uses cmake and linking with a PCRE2 that provides this file (meaning it was installed by running cmake --install instead of the more common way to get PCRE2 installed which is calling make install as "root" while using a POSIX compatible shell), hence maybe mingw64 bash.

Not sure if this answers your question but vcpkg does this too
https://github.com/microsoft/vcpkg/blob/master/ports/pcre2/portfile.cmake#L54

some of the patches we are discusing were tested with vcpkg but had to be reverted before 10.43RC1 because they broke autotools bootstrapping.

it doesn't answer though how to document the hopefully small backward incompatibility that they introduce in their current form.

alexreinking · 2024-01-17T15:58:15Z

As long as you're making a change like this, you might as well make the path configurable via a cache variable like PCRE2_INSTALL_CMAKEDIR. For example (sketch):

include(GNUInstallDirs)

set(PCRE2_INSTALL_CMAKEDIR "${CMAKE_INSTALL_LIBDIR}/cmake/pcre2"
    CACHE STRING "Relative path to PCRE2's CMake config files from the installation root")

install(
    FILES ${PCRE2_CONFIG_OUT} ${PCRE2_CONFIG_VERSION_OUT}
    DESTINATION "${PCRE2_INSTALL_CMAKEDIR}"
)

I go into a lot more detail on CMake packaging on my blog here: https://alexreinking.com/blog/building-a-dual-shared-and-static-library-with-cmake.html

…#428) Historically, pcre2grep has done minor processing of the patterns that were read through the `-f` option. The end result is that for some patterns there are different results depending if they were provided through `-e`, `-f` or as a parameter in the command line. Add a flag that could be provided to skip that processing so that the same pattern file used with other grep implementations could be used directly for the same result.

zherczeg · 2024-11-15T17:30:21Z

Is this still valid/needed?

alexreinking · 2024-11-19T19:13:18Z

Is this still valid/needed?

Yes, it looks like a change along the lines of my suggestion above would be a good one.

pcre2/CMakeLists.txt

Line 1135 in db95416

    
           install(FILES ${PCRE2_CONFIG_OUT} ${PCRE2_CONFIG_VERSION_OUT} DESTINATION cmake)

NWilson · 2024-11-20T13:29:26Z

I am curious what your use-case is for "make install". Personally, it would never occur to me to use it for a library.

I mean, for a binary perhaps: you might want to check it out, build, and install it to ~/bin or /usr/local/bin, but a library? Who installs a library like that? Libs will come packaged by the OS (on Linux) or bundled by an application that builds in the library using its own toolchain.

I have never done a "make install" for a library, so I just don't understand the purpose of it.

In principle though, I think we'd be happy to go along with any suggestions users have.

PhilipHazel · 2024-11-20T13:41:23Z

@NWilson - More History! When PCRE was new, the only way to install it was "make install". It was only after it started to get widely used that distros started packaging it. So I guess you have never wanted to install an uncommon library. :-)

NWilson · 2024-11-20T13:50:09Z

If I were writing an Apache httpd plugin using PCRE (for example), and there wasn't a PCRE2 bundled by the OS, then I'd statically link it in.

That probably comes from my background shipping commercial software on Unix platforms. The apps are always shipped with dependencies included, so you don't get support requests from users on different Linux distros. I can see that the open-source way probably would instruct users to fetch and install the library globally, outside of the OS packaging system.

Things have moved on maybe. These days, open-source packages for Linux seem to be either bundled by distros, or the upstream project provides an apt/RPM feed of their own, or they provide a build with dependencies statically linked - precisely to avoid the hassle of incompatible library versions on the user's system.

carenas · 2024-11-20T14:41:26Z

If I were writing an Apache httpd plugin using PCRE (for example), and there wasn't a PCRE2 bundled by the OS, then I'd statically link it in.

That is very common in Windows but not so in Linux or the BSD.

Note that the OP is the FreeBSD packager for PCRE2, and they had (IMHO) mistakenly moved to use cmake instead of autotools to deploy those system wide.

PhilipHazel · 2024-11-20T14:45:17Z

Early versions of PCRE were indeed bundled with Exim because PCRE was then an unknown library, but I stopped doing that once other software started to use PCRE and needed a properly installed separate library.

diizzyy · 2024-11-20T20:25:50Z

Is this still valid/needed?
Can also confirm that it is

NWilson · 2024-12-08T15:17:59Z

I've added the excellent suggestion from @alexreinking to allow clients to override the new lib/cmake/pcre2 location (for example, to go back to the old directory!).

This should address @carenas's concerns, by allowing affected users to go back to how it used to be.

I am satisfied with this change, and I think we should merge it for the 10.45-RC1. We can always make changes if there is any feedback.

Thank you @diizzyy for raising this!

cmake: Install cmake files in a more common path

fba4a6a

Install cmake files in prefix/lib/cmake/pcre2 rather than prefix/cmake

carenas reviewed Dec 30, 2023

View reviewed changes

NWilson added 2 commits December 8, 2024 15:01

Merge remote-tracking branch 'origin/master' into patch-1

cba0f84

Add PCRE2_INSTALL_CMAKEDIR, as suggested by Alex Reinking

15044ac

NWilson added this to the 10.45-RC1 milestone Dec 8, 2024

NWilson approved these changes Dec 8, 2024

View reviewed changes

NWilson merged commit 0212293 into PCRE2Project:master Dec 9, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: Install cmake files in a more common path #366

cmake: Install cmake files in a more common path #366

diizzyy commented Dec 30, 2023

carenas left a comment •

edited

Loading

carenas Dec 30, 2023 •

edited

Loading

diizzyy Dec 30, 2023

carenas Dec 30, 2023 •

edited

Loading

diizzyy Dec 30, 2023

carenas Dec 30, 2023 •

edited

Loading

diizzyy Dec 31, 2023

carenas Dec 31, 2023 •

edited

Loading

alexreinking commented Jan 17, 2024 •

edited

Loading

zherczeg commented Nov 15, 2024

alexreinking commented Nov 19, 2024 •

edited

Loading

NWilson commented Nov 20, 2024 •

edited

Loading

PhilipHazel commented Nov 20, 2024

NWilson commented Nov 20, 2024

carenas commented Nov 20, 2024

PhilipHazel commented Nov 20, 2024

diizzyy commented Nov 20, 2024

NWilson commented Dec 8, 2024

cmake: Install cmake files in a more common path #366

cmake: Install cmake files in a more common path #366

Conversation

diizzyy commented Dec 30, 2023

carenas left a comment • edited Loading

Choose a reason for hiding this comment

carenas Dec 30, 2023 • edited Loading

Choose a reason for hiding this comment

diizzyy Dec 30, 2023

Choose a reason for hiding this comment

carenas Dec 30, 2023 • edited Loading

Choose a reason for hiding this comment

diizzyy Dec 30, 2023

Choose a reason for hiding this comment

carenas Dec 30, 2023 • edited Loading

Choose a reason for hiding this comment

diizzyy Dec 31, 2023

Choose a reason for hiding this comment

carenas Dec 31, 2023 • edited Loading

Choose a reason for hiding this comment

alexreinking commented Jan 17, 2024 • edited Loading

zherczeg commented Nov 15, 2024

alexreinking commented Nov 19, 2024 • edited Loading

NWilson commented Nov 20, 2024 • edited Loading

PhilipHazel commented Nov 20, 2024

NWilson commented Nov 20, 2024

carenas commented Nov 20, 2024

PhilipHazel commented Nov 20, 2024

diizzyy commented Nov 20, 2024

NWilson commented Dec 8, 2024

carenas left a comment •

edited

Loading

carenas Dec 30, 2023 •

edited

Loading

carenas Dec 30, 2023 •

edited

Loading

carenas Dec 30, 2023 •

edited

Loading

carenas Dec 31, 2023 •

edited

Loading

alexreinking commented Jan 17, 2024 •

edited

Loading

alexreinking commented Nov 19, 2024 •

edited

Loading

NWilson commented Nov 20, 2024 •

edited

Loading