Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use standard CMake constructs to export the library's targets. #260

Merged
merged 10 commits into from
Aug 15, 2023
Merged

Use standard CMake constructs to export the library's targets. #260

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8e157d1
Use standard CMake constructs to export the library's targets.
teo-tsirpanis Jun 3, 2023
5b1beab
Fix typo.
teo-tsirpanis Jun 11, 2023
8ee4c22
Merge remote-tracking branch 'origin/master' into cmake-fix
teo-tsirpanis Jun 14, 2023
e85d2d8
Document that PCRE2_USE_STATIC_LIBS is not required if only one editi…
teo-tsirpanis Jun 14, 2023
b8d27cf
Replace some missed stuff from previous review.
teo-tsirpanis Jun 14, 2023
8acaf91
Don't explicitly set `PCRE2_FOUND` to true.
teo-tsirpanis Jun 14, 2023
1cff028
Avoid defining the target aliases if they are already defined.
teo-tsirpanis Jun 14, 2023
0551a75
Set `PCRE2_LIBRARIES` and `PCRE2_***_LIBRARY` for compatibility.
teo-tsirpanis Jun 14, 2023
7514b61
Change `PCRE2_USE_STATIC_LIBS` to prohibit loading the shared library…
teo-tsirpanis Jun 14, 2023
b66f049
Merge branch 'master' into cmake-fix
teo-tsirpanis Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 37 additions & 16 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@
# 2021-08-28 PH added test for realpath()
# 2022-12-10 PH added support for pcre2posix_test
# 2023-01-15 Carlo added C99 as the minimum required
# 2023-06-03 Theodore used standard CMake constructs to export the library's targets.

# Increased minimum to 3.3 to support visibility.
CMAKE_MINIMUM_REQUIRED(VERSION 3.3)
Expand Down Expand Up @@ -141,6 +142,7 @@ INCLUDE(CheckFunctionExists)
INCLUDE(CheckSymbolExists)
INCLUDE(CheckIncludeFile)
INCLUDE(CheckTypeSize)
INCLUDE(CMakePackageConfigHelpers)
INCLUDE(GNUInstallDirs) # for CMAKE_INSTALL_LIBDIR

CHECK_INCLUDE_FILE(dirent.h HAVE_DIRENT_H)
Expand Down Expand Up @@ -730,7 +732,9 @@ IF(PCRE2_BUILD_PCRE2_8)
VERSION ${LIBPCRE2_8_VERSION}
SOVERSION ${LIBPCRE2_8_SOVERSION})
TARGET_COMPILE_DEFINITIONS(pcre2-8-static PUBLIC PCRE2_STATIC)
TARGET_INCLUDE_DIRECTORIES(pcre2-8-static PUBLIC ${PROJECT_BINARY_DIR})
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved
TARGET_INCLUDE_DIRECTORIES(pcre2-8-static PUBLIC
$<BUILD_INTERFACE:${PROJECT_BINARY_DIR}>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved
IF(REQUIRE_PTHREAD)
TARGET_LINK_LIBRARIES(pcre2-8-static Threads::Threads)
ENDIF(REQUIRE_PTHREAD)
Expand All @@ -743,7 +747,9 @@ IF(PCRE2_BUILD_PCRE2_8)
VERSION ${LIBPCRE2_POSIX_VERSION}
SOVERSION ${LIBPCRE2_POSIX_SOVERSION})
TARGET_LINK_LIBRARIES(pcre2-posix-static pcre2-8-static)
TARGET_INCLUDE_DIRECTORIES(pcre2-posix-static PUBLIC ${PROJECT_BINARY_DIR})
TARGET_INCLUDE_DIRECTORIES(pcre2-posix-static PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)
SET(targets ${targets} pcre2-posix-static)

IF(MSVC)
Expand All @@ -760,7 +766,9 @@ IF(PCRE2_BUILD_PCRE2_8)

IF(BUILD_SHARED_LIBS)
ADD_LIBRARY(pcre2-8-shared SHARED ${PCRE2_HEADERS} ${PCRE2_SOURCES} ${PROJECT_BINARY_DIR}/config.h)
TARGET_INCLUDE_DIRECTORIES(pcre2-8-shared PUBLIC ${PROJECT_BINARY_DIR})
TARGET_INCLUDE_DIRECTORIES(pcre2-8-shared PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)
SET_TARGET_PROPERTIES(pcre2-8-shared PROPERTIES
COMPILE_DEFINITIONS PCRE2_CODE_UNIT_WIDTH=8
MACHO_COMPATIBILITY_VERSION "${LIBPCRE2_8_MACHO_COMPATIBILITY_VERSION}"
Expand All @@ -773,8 +781,10 @@ IF(PCRE2_BUILD_PCRE2_8)
ENDIF(REQUIRE_PTHREAD)
SET(targets ${targets} pcre2-8-shared)

ADD_LIBRARY(pcre2-posix-shared SHARED ${PCRE2POSIX_HEADERS} ${PCRE2POSIX_SOURCES})
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved
TARGET_INCLUDE_DIRECTORIES(pcre2-posix-shared PUBLIC ${PROJECT_BINARY_DIR})
ADD_LIBRARY(pcre2-posix-shared SHARED ${PCRE2POSIX_SOURCES})
TARGET_INCLUDE_DIRECTORIES(pcre2-posix-shared PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)
SET_TARGET_PROPERTIES(pcre2-posix-shared PROPERTIES
COMPILE_DEFINITIONS PCRE2_CODE_UNIT_WIDTH=8
MACHO_COMPATIBILITY_VERSION "${LIBPCRE2_POSIX_MACHO_COMPATIBILITY_VERSION}"
Expand Down Expand Up @@ -812,7 +822,9 @@ ENDIF(PCRE2_BUILD_PCRE2_8)
IF(PCRE2_BUILD_PCRE2_16)
IF(BUILD_STATIC_LIBS)
ADD_LIBRARY(pcre2-16-static STATIC ${PCRE2_HEADERS} ${PCRE2_SOURCES} ${PROJECT_BINARY_DIR}/config.h)
TARGET_INCLUDE_DIRECTORIES(pcre2-16-static PUBLIC ${PROJECT_BINARY_DIR})
TARGET_INCLUDE_DIRECTORIES(pcre2-16-static PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)
SET_TARGET_PROPERTIES(pcre2-16-static PROPERTIES
COMPILE_DEFINITIONS PCRE2_CODE_UNIT_WIDTH=16
MACHO_COMPATIBILITY_VERSION "${LIBPCRE2_32_MACHO_COMPATIBILITY_VERSION}"
Expand All @@ -837,7 +849,9 @@ IF(PCRE2_BUILD_PCRE2_16)

IF(BUILD_SHARED_LIBS)
ADD_LIBRARY(pcre2-16-shared SHARED ${PCRE2_HEADERS} ${PCRE2_SOURCES} ${PROJECT_BINARY_DIR}/config.h)
TARGET_INCLUDE_DIRECTORIES(pcre2-16-shared PUBLIC ${PROJECT_BINARY_DIR})
TARGET_INCLUDE_DIRECTORIES(pcre2-16-shared PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)
SET_TARGET_PROPERTIES(pcre2-16-shared PROPERTIES
COMPILE_DEFINITIONS PCRE2_CODE_UNIT_WIDTH=16
MACHO_COMPATIBILITY_VERSION "${LIBPCRE2_32_MACHO_COMPATIBILITY_VERSION}"
Expand Down Expand Up @@ -874,7 +888,9 @@ ENDIF(PCRE2_BUILD_PCRE2_16)
IF(PCRE2_BUILD_PCRE2_32)
IF(BUILD_STATIC_LIBS)
ADD_LIBRARY(pcre2-32-static STATIC ${PCRE2_HEADERS} ${PCRE2_SOURCES} ${PROJECT_BINARY_DIR}/config.h)
TARGET_INCLUDE_DIRECTORIES(pcre2-32-static PUBLIC ${PROJECT_BINARY_DIR})
TARGET_INCLUDE_DIRECTORIES(pcre2-32-static PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)
SET_TARGET_PROPERTIES(pcre2-32-static PROPERTIES
COMPILE_DEFINITIONS PCRE2_CODE_UNIT_WIDTH=32
MACHO_COMPATIBILITY_VERSION "${LIBPCRE2_32_MACHO_COMPATIBILITY_VERSION}"
Expand All @@ -899,7 +915,9 @@ IF(PCRE2_BUILD_PCRE2_32)

IF(BUILD_SHARED_LIBS)
ADD_LIBRARY(pcre2-32-shared SHARED ${PCRE2_HEADERS} ${PCRE2_SOURCES} ${PROJECT_BINARY_DIR}/config.h)
TARGET_INCLUDE_DIRECTORIES(pcre2-32-shared PUBLIC ${PROJECT_BINARY_DIR})
TARGET_INCLUDE_DIRECTORIES(pcre2-32-shared PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)
SET_TARGET_PROPERTIES(pcre2-32-shared PROPERTIES
COMPILE_DEFINITIONS PCRE2_CODE_UNIT_WIDTH=32
MACHO_COMPATIBILITY_VERSION "${LIBPCRE2_32_MACHO_COMPATIBILITY_VERSION}"
Expand Down Expand Up @@ -970,13 +988,11 @@ IF(PCRE2_BUILD_TESTS)

IF(PCRE2_BUILD_PCRE2_8)
ADD_EXECUTABLE(pcre2posix_test src/pcre2posix_test.c)
SET(targets ${targets} pcre2posix_test)
TARGET_LINK_LIBRARIES(pcre2posix_test pcre2-posix pcre2-8)
ENDIF(PCRE2_BUILD_PCRE2_8)

IF(PCRE2_SUPPORT_JIT)
ADD_EXECUTABLE(pcre2_jit_test src/pcre2_jit_test.c)
SET(targets ${targets} pcre2_jit_test)
SET(PCRE2_JIT_TEST_LIBS)
IF(PCRE2_BUILD_PCRE2_8)
LIST(APPEND PCRE2_JIT_TEST_LIBS pcre2-8)
Expand Down Expand Up @@ -1101,9 +1117,13 @@ ENDIF(PCRE2_BUILD_TESTS)
SET(CMAKE_INSTALL_ALWAYS 1)

INSTALL(TARGETS ${targets}
RUNTIME DESTINATION bin
EXPORT pcre2-targets
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
INSTALL(EXPORT pcre2-targets
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/pcre2
NAMESPACE pcre2::)
INSTALL(FILES ${pkg_config_files} DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig)
INSTALL(FILES "${CMAKE_CURRENT_BINARY_DIR}/pcre2-config"
DESTINATION bin
Expand All @@ -1115,11 +1135,12 @@ INSTALL(FILES ${PCRE2_HEADERS} ${PCRE2POSIX_HEADERS} DESTINATION include)
# CMake config files.
set(PCRE2_CONFIG_IN ${CMAKE_CURRENT_SOURCE_DIR}/cmake/pcre2-config.cmake.in)
set(PCRE2_CONFIG_OUT ${CMAKE_CURRENT_BINARY_DIR}/cmake/pcre2-config.cmake)
configure_file(${PCRE2_CONFIG_IN} ${PCRE2_CONFIG_OUT} @ONLY)
set(PCRE2_CONFIG_VERSION_IN ${CMAKE_CURRENT_SOURCE_DIR}/cmake/pcre2-config-version.cmake.in)
configure_package_config_file(${PCRE2_CONFIG_IN} ${PCRE2_CONFIG_OUT} INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/pcre2)
Copy link

@dg0yt dg0yt Jun 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INSTALL_DESTINATION implies an installation action.
Either no INSTALL_DESTINATION here, or no PCRE2_CONFIG_OUT two lines later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had tried removing the install below, the file does not get copied.

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)
write_basic_package_version_file(${PCRE2_CONFIG_VERSION_OUT}
VERSION ${PCRE2_MAJOR}.${PCRE2_MINOR}.0
COMPATIBILITY SameMajorVersion)
install(FILES ${PCRE2_CONFIG_OUT} ${PCRE2_CONFIG_VERSION_OUT} DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/pcre2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this change the output directory from cmake to "cmake/pcre2"?, does that make a difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's a regression (thanks for catching it!), but I don't think it would have any impact; it is one of the paths find_package will search.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you verify that it doesn't have an impact?, is there a minimum cmake version that will be needed to include those extra paths?

no idea why it was set that way originally but the change is somehow recent with 2410fbe by jwsblokland

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The earliest CMake version I found that specifies this path for both Windows and Unix is 3.7. For reference the minimum CMake version that Google supports is 3.13 (the earliest version that ships in a supported Linux distribution).

Copy link
Contributor

@carenas carenas Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I am not advocating to use cmake 3.3 forever (indeed, if you look at the git log, I'd been quietly rising it from 3.0, as new features were used), just making sure that if we are going to say that is the minimum version supported, it actually is.

One interesting thing I noticed after installing cmake 3.3.2 and doing a deployment is that the original code actually puts those 2 files in ${INSTALL_PREFIX}/cmake, so I wouldn't be surprised if whoever is using this might be adding that special path somehow already and therefore will get a nasty surprise when it goes away.

I am testing in a Fedora 38 system, so maybe it is different in Windows and they might be using that and it might work based on your reading of the documentation, but either way it is not a backward compatible change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where can we document this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, the code will be changed to be backward compatible so there is no need to document it.

Otherwise, we need to make @PhilipHazel aware so it is mentioned in the ChangeLog and he includes a note of it for the next release announcement. It might even force us to do first a RC release since whoever might be affected will need to make sure to update their code first.

In this case I am sympathetic to the change, since it is likely to work by default and is more consistent, although it might require setting FIND_LIBRARY_USE_LIB64_PATHS in some cases.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are discussing the install location of the CMake config file, which is the key to locating the package.
For sane values of CMAKE_INSTALL_LIBDIR in relation to the prefix, both the old and the new location is searched by find_package Config mode, also in version 3.3, and regardless of operating system:
The old location matches <prefix>/(cmake|CMake)/.
The new location matches <prefix>/(lib/<arch>|lib|share)/cmake/<name>*/.

Given that exported configs usually consist of several interacting files (config, version, targets, per-config details), this change is necessary and expected, in order to organize the files in a local unit.

Copy link
Contributor

@carenas carenas Jun 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the change seems to be in the right direction (specially for *NIX), but we are likely going to need something to put in the release announcement so that anyone that might be affected, is aware of the change and can do whatever change might be needed on their side.

Our old code, wasn't very friendly to multiarch systems or cross compilation because it was installed in such a generic location, but that path was specifically chosen for some reason I am not aware of, so the least we could do is check with the author and come with some scenarios for a migration (if needed).


FILE(GLOB html ${PROJECT_SOURCE_DIR}/doc/html/*.html)
FILE(GLOB man1 ${PROJECT_SOURCE_DIR}/doc/*.1)
Expand Down
15 changes: 0 additions & 15 deletions cmake/pcre2-config-version.cmake.in
View file
Open in desktop

This file was deleted.

134 changes: 36 additions & 98 deletions cmake/pcre2-config.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,13 @@
# -----------------
# To make use of the static library instead of the shared one, one needs
# to set the variable PCRE2_USE_STATIC_LIBS to ON before calling find_package.
#
# The following components are supported: 8BIT, 16BIT, 32BIT and POSIX.
# They used to be required but not anymore; all available targets will
# be defined regardless of the requested components.
# Example:
# set(PCRE2_USE_STATIC_LIBS ON)
# find_package(PCRE2 CONFIG COMPONENTS 8BIT)
# find_package(PCRE2 CONFIG)
#
# This will define the following variables:
#
Expand All @@ -23,70 +27,40 @@
# PCRE2::32BIT - The 32 bit PCRE2 library.
# PCRE2::POSIX - The POSIX PCRE2 library.

set(PCRE2_NON_STANDARD_LIB_PREFIX @NON_STANDARD_LIB_PREFIX@)
set(PCRE2_NON_STANDARD_LIB_SUFFIX @NON_STANDARD_LIB_SUFFIX@)
set(PCRE2_8BIT_NAME pcre2-8)
set(PCRE2_16BIT_NAME pcre2-16)
set(PCRE2_32BIT_NAME pcre2-32)
set(PCRE2_POSIX_NAME pcre2-posix)
find_path(PCRE2_INCLUDE_DIR NAMES pcre2.h DOC "PCRE2 include directory")
if (PCRE2_USE_STATIC_LIBS)
if (MSVC)
set(PCRE2_8BIT_NAME pcre2-8-static)
set(PCRE2_16BIT_NAME pcre2-16-static)
set(PCRE2_32BIT_NAME pcre2-32-static)
set(PCRE2_POSIX_NAME pcre2-posix-static)
endif ()
@PACKAGE_INIT@

set(PCRE2_PREFIX ${CMAKE_STATIC_LIBRARY_PREFIX})
set(PCRE2_SUFFIX ${CMAKE_STATIC_LIBRARY_SUFFIX})
else ()
set(PCRE2_PREFIX ${CMAKE_SHARED_LIBRARY_PREFIX})
if (MINGW AND PCRE2_NON_STANDARD_LIB_PREFIX)
set(PCRE2_PREFIX "")
endif ()
include(CMakeFindDependencyMacro)
if("@REQUIRE_PTHREAD@") # REQUIRE_PTHREAD
find_dependency(Threads)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this have REQUIRED?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't play well if pcre2 itself is not REQUIRED. find_dependency will put REQUIRED if the requesting package is.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This a key thing about CMake config search mode, even for find_package(pcre2 REQUIRED):
With REQUIRED, configuration would fail immediately. This isn't desired.
If a dependency isn't found, only the current config shall be ignored, and the search for a suitable config shall continue in other locations.

endif()

set(PCRE2_SUFFIX ${CMAKE_SHARED_LIBRARY_SUFFIX})
if (MINGW AND PCRE2_NON_STANDARD_LIB_SUFFIX)
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved
set(PCRE2_SUFFIX "-0.dll")
endif ()
endif ()
find_library(PCRE2_8BIT_LIBRARY NAMES ${PCRE2_PREFIX}${PCRE2_8BIT_NAME}${PCRE2_SUFFIX} ${PCRE2_PREFIX}${PCRE2_8BIT_NAME}d${PCRE2_SUFFIX} DOC "8 bit PCRE2 library")
find_library(PCRE2_16BIT_LIBRARY NAMES ${PCRE2_PREFIX}${PCRE2_16BIT_NAME}${PCRE2_SUFFIX} ${PCRE2_PREFIX}${PCRE2_8BIT_NAME}d${PCRE2_SUFFIX} DOC "16 bit PCRE2 library")
find_library(PCRE2_32BIT_LIBRARY NAMES ${PCRE2_PREFIX}${PCRE2_32BIT_NAME}${PCRE2_SUFFIX} ${PCRE2_PREFIX}${PCRE2_8BIT_NAME}d${PCRE2_SUFFIX} DOC "32 bit PCRE2 library")
find_library(PCRE2_POSIX_LIBRARY NAMES ${PCRE2_PREFIX}${PCRE2_POSIX_NAME}${PCRE2_SUFFIX} ${PCRE2_PREFIX}${PCRE2_8BIT_NAME}d${PCRE2_SUFFIX} DOC "8 bit POSIX PCRE2 library")
unset(PCRE2_NON_STANDARD_LIB_PREFIX)
unset(PCRE2_NON_STANDARD_LIB_SUFFIX)
unset(PCRE2_8BIT_NAME)
unset(PCRE2_16BIT_NAME)
unset(PCRE2_32BIT_NAME)
unset(PCRE2_POSIX_NAME)
include("${CMAKE_CURRENT_LIST_DIR}/@[email protected]")
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved

# Set version
if (PCRE2_INCLUDE_DIR)
set(PCRE2_VERSION "@PCRE2_MAJOR@.@[email protected]")
endif ()
set(PCRE2_VERSION "@PCRE2_MAJOR@.@[email protected]")

# Which components have been found.
if (PCRE2_8BIT_LIBRARY)
set(PCRE2_8BIT_FOUND TRUE)
endif ()
if (PCRE2_16BIT_LIBRARY)
set(PCRE2_16BIT_FOUND TRUE)
endif ()
if (PCRE2_32BIT_LIBRARY)
set(PCRE2_32BIT_FOUND TRUE)
endif ()
if (PCRE2_POSIX_LIBRARY)
set(PCRE2_POSIX_FOUND TRUE)
endif ()

# Check if at least one component has been specified.
list(LENGTH PCRE2_FIND_COMPONENTS PCRE2_NCOMPONENTS)
if (PCRE2_NCOMPONENTS LESS 1)
message(FATAL_ERROR "No components have been specified. This is not allowed. Please, specify at least one component.")
endif ()
unset(PCRE2_NCOMPONENTS)
# Chooses the linkage of the library to expose in the
# unsuffixed edition of the target.
macro(_pcre2_add_component_target component target)
# If the static or shared editions of the library exist,
# we mark that we found the package itself and the corresponding component.
if (TARGET pcre2::pcre2-${target}-static OR TARGET pcre2::pcre2-${target}-shared)
set(PCRE2_FOUND TRUE)
teo-tsirpanis marked this conversation as resolved.
Show resolved Hide resolved
set(PCRE2_${component}_FOUND TRUE)
endif()
# If the static library exists and either PCRE2_USE_STATIC_LIBS
# is defined, or the dynamic library does not exist, use the static library.
if (TARGET pcre2::pcre2-${target}-static AND (PCRE2_USE_STATIC_LIBS OR NOT TARGET pcre2::pcre2-${target}-shared))
add_library(PCRE2::${component} ALIAS pcre2::pcre2-${target}-static)
# Otherwise use the dynamic library if it exists.
elseif(TARGET pcre2::pcre2-${target}-shared)
add_library(PCRE2::${component} ALIAS pcre2::pcre2-${target}-shared)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before adding a target in a config file, you must check that it doesn't already exist - a config file may be included multiple times via transitive usage.
(Note that the use of ALIAS is not entirely without problems - there are some scope issues. On the user side, a mitigation is the use of IMPORTED_GLOBAL (since CMake 3.11). The alternative are INTERFACE targets.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a config file may be included multiple times via transitive usage

I will add a check. Should I also add an include_guard?

[…] there are some scope issues. On the user side, a mitigation is […]

Or find_package(PCRE2 GLOBAL), right?

endif()
endmacro()
_pcre2_add_component_target(8BIT 8)
_pcre2_add_component_target(16BIT 16)
_pcre2_add_component_target(32BIT 32)
_pcre2_add_component_target(POSIX posix)

# When POSIX component has been specified make sure that also 8BIT component is specified.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of removing this logic. The dependency of POSIX to 8BIT is expressed in target dependencies, and the fatal error might interfere with CMake keeping searching to other locations.

set(PCRE2_8BIT_COMPONENT FALSE)
Expand All @@ -105,41 +79,5 @@ endif()
unset(PCRE2_8BIT_COMPONENT)
unset(PCRE2_POSIX_COMPONENT)

include(FindPackageHandleStandardArgs)
set(${CMAKE_FIND_PACKAGE_NAME}_CONFIG "${CMAKE_CURRENT_LIST_FILE}")
find_package_handle_standard_args(PCRE2
FOUND_VAR PCRE2_FOUND
REQUIRED_VARS PCRE2_INCLUDE_DIR
HANDLE_COMPONENTS
VERSION_VAR PCRE2_VERSION
CONFIG_MODE
)

set(PCRE2_LIBRARIES)
if (PCRE2_FOUND)
foreach(component ${PCRE2_FIND_COMPONENTS})
if (PCRE2_USE_STATIC_LIBS)
add_library(PCRE2::${component} STATIC IMPORTED)
target_compile_definitions(PCRE2::${component} INTERFACE PCRE2_STATIC)
else ()
add_library(PCRE2::${component} SHARED IMPORTED)
endif ()
set_target_properties(PCRE2::${component} PROPERTIES
IMPORTED_LOCATION "${PCRE2_${component}_LIBRARY}"
INTERFACE_INCLUDE_DIRECTORIES "${PCRE2_INCLUDE_DIR}"
)
if (component STREQUAL "POSIX")
set_target_properties(PCRE2::${component} PROPERTIES
INTERFACE_LINK_LIBRARIES "PCRE2::8BIT"
LINK_LIBRARIES "PCRE2::8BIT"
)
endif ()

set(PCRE2_LIBRARIES ${PCRE2_LIBRARIES} ${PCRE2_${component}_LIBRARY})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, you are removing the output variable interface. While it is legacy CMake (in contrast to targets), it is unclear if some users still rely on it.
However, it is also unclear which variables form the public interface. IMO it is at least:
PCRE2_LIBRARIES (subject to components)
PCRE2_INCLUDE_DIR
maybe also PCRE2_${component}_LIBRARY.
I would recommend to at least place the target names in the lib variables. (This still leaves room for incompatibilities but common cases would be covered.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set the library variables to the library paths.

mark_as_advanced(PCRE2_${component}_LIBRARY)
endforeach()
endif ()

mark_as_advanced(
PCRE2_INCLUDE_DIR
)
# Check for required components.
check_required_components("PCRE2")