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.

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

Conversation

teo-tsirpanis
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis commented Jun 3, 2023

pcre2-config.cmake finds PCRE2's libraries with find_library. This approach does not work well with vcpkg. This PR switches to a more standard way that precisely exports the targets, specifying the library files.

I also made the COMPONENTS optional in find_package; they are not really needed, the libraries will exist or not exist regardless of any components specified.

Contributes to #115. Fixes #264. FYI @dg0yt

Copy link
Contributor

@carenas carenas left a comment

Choose a reason for hiding this comment

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

squash all commits together and make sure that the syntax used actually matches the minimum_version advertised.

there are a lot of different use cases (some of them conflicting) supported by the old code, so make sure there are no regressions for those as well.

the maintainer doesn't use cmake, so we rely on external users that do to make sure it works as expected.

@teo-tsirpanis
Copy link
Contributor Author

squash all commits together

Done.

make sure that the syntax used actually matches the minimum_version advertised

I identified the following new constructs in the PR, and all exist in the documentation of CMake 3.3.

  • if (TARGET).
  • The BUILD_INTERFACE and INSTALL_INTERFACE generator expressions.
  • PUBLIC_HEADER.
  • The CMakePackageConfigHelpers module.
  • ALIAS libraries.

there are a lot of different use cases (some of them conflicting) supported by the old code, so make sure there are no regressions for those as well.

To the best of my knowledge there should not be any breaking changes in existing scenarios; the same targets are present, and are acquired the same way. But I am a newbie in CMake so a second opinion would be very much appreciated.

@teo-tsirpanis teo-tsirpanis marked this pull request as ready for review June 7, 2023 17:15
@teo-tsirpanis teo-tsirpanis requested review from carenas and dg0yt June 7, 2023 17:15
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

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

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).

Copy link
Contributor

@carenas carenas left a comment

Choose a reason for hiding this comment

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

cmake --install seems to be installing pcre2.h and pcre2posix.h twice, which is a regression of sorts and results in the following error while uninstalling in the recommended way:

# xargs rm < install_manifest.txt
rm: /usr/include/pcre2.h: No such file or directory
rm: /usr/include/pcre2posix.h: No such file or directory

@teo-tsirpanis
Copy link
Contributor Author

I reverted to the previous way of installing the targets, and they are listed in the install manifest only once. The include directories in the exported targets also seem to be specified fine.

Let me know what you think.

@teo-tsirpanis teo-tsirpanis requested a review from carenas June 7, 2023 19:43
@carenas
Copy link
Contributor

carenas commented Jun 7, 2023

Let me know what you think

I think we have a misunderstanding.

You can't expect a "review" before merging your change from any of the PCRE2 developers, because none of us use cmake. The changes we do to those files are at best educated guesses, and had even sometimes introduced bugs just like the couple I found while "reviewing" your proposal (and yes, I installed cmake 3.3 and used that, but only in macOS, while I suspect there might be additional issues if I try to get Windows running as well)

as I explained before, there are a lot of use cases (none formally documented except for the code, but there is information in the old issue you commented on), and they were all contributed by users of cmake with their own itches, and their use cases shouldn't be regressed unless you both agree that yours is more important.

if the small issue I found was solved by reverting, maybe there are more of those that should be considered before we try to continue?

Copy link
Contributor

@carenas carenas left a comment

Choose a reason for hiding this comment

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

the following currently working CMakeLists.txt :

cmake_minimum_required(VERSION 3.12)

project(pcre2posix_test C)

set(PCRE2_USE_STATIC_LIBS ON)
find_package(PCRE2 10.30 REQUIRED COMPONENTS 8BIT POSIX)
add_executable(pcre2posix_test src/pcre2posix_test.c)
target_compile_definitions(pcre2posix_test PUBLIC PCRE2::8BIT PCRE2::POSIX)
set_property(TARGET pcre2posix_test PROPERTY INTERFACE_COMPILE_OPTIONS -DPCRE2POSIX_SHARED)
target_link_libraries(pcre2posix_test PCRE2::8BIT PCRE2::POSIX)

No longer works with the new version because the removal of the modules was not done in a backward compatible way, as shown by:

# cmake ..
-- The C compiler identification is GNU 13.1.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Configuring done (0.2s)
CMake Error at CMakeLists.txt:10 (target_link_libraries):
  Target "pcre2posix_test" links to:

    PCRE2::8BIT

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.



-- Generating done (0.0s)
CMake Generate step failed.  Build files cannot be regenerated correctly.

@teo-tsirpanis
Copy link
Contributor Author

the removal of the modules was not done in a backward compatible way

Oops, I had gotten confused by the differing internal and exported names (they were right intitially but I later renamed them to fix a "typo"). Now fixed, thanks!

@carenas
Copy link
Contributor

carenas commented Jun 8, 2023

not sure how you are testing your changes, but in CentOS 7 with the EPEL provided cmake3 (3.17.5):

# cmake3 -GNinja ..
-- The C compiler identification is GNU 4.8.5
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc - works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
CMake Error at /usr/lib64/cmake/pcre2/pcre2-config.cmake:83 (add_library):
  add_library cannot create ALIAS target "PCRE2::8BIT" because target
  "pcre2::pcre2-8-static" is imported but not globally visible.
Call Stack (most recent call first):
  /usr/lib64/cmake/pcre2/pcre2-config.cmake:89 (_pcre2_add_component_target)
  CMakeLists.txt:6 (find_package)


CMake Error at /usr/lib64/cmake/pcre2/pcre2-config.cmake:83 (add_library):
  add_library cannot create ALIAS target "PCRE2::POSIX" because target
  "pcre2::pcre2-posix-static" is imported but not globally visible.
Call Stack (most recent call first):
  /usr/lib64/cmake/pcre2/pcre2-config.cmake:92 (_pcre2_add_component_target)
  CMakeLists.txt:6 (find_package)


-- Configuring incomplete, errors occurred!

Copy link
Contributor

@carenas carenas left a comment

Choose a reason for hiding this comment

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

not installing pcre2test is a regression, additionally note that you need to build all of them or otherwise ctest will break

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@teo-tsirpanis
Copy link
Contributor Author

add_library cannot create ALIAS target "PCRE2::8BIT" because target
"pcre2::pcre2-8-static" is imported but not globally visible.

Hmm, I found "Export names are considered global so any directory may contribute a target installation." at the end of https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#exporting-targets. Maybe it's not applicable to older CMake versions. 🤔

not sure how you are testing your changes

I am using a modified vcpkg port. It has worked for me since the beginning.

@carenas
Copy link
Contributor

carenas commented Jun 9, 2023

I am using a modified vcpkg port. It has worked for me since the beginning.

Assuming you care about all other use cases than yours, it would be better IMHO to test cmake directly.

After deploying a PCRE2 built from your latest update in a Debian 12 system using the cmake they provide (3.25.1) and trying to build a very simple application against it (using a working script that was already provided, which is obviously hacky, badly coded, and has definitely bugs because I have no clue how to use cmake otherwise), you get:

# cmake -DCMAKE_VERBOSE_MAKEFILE=ON ..
-- The C compiler identification is GNU 12.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Configuring done
CMake Error at /usr/lib/x86_64-linux-gnu/cmake/pcre2/pcre2-targets.cmake:71 (set_target_properties):
  The link interface of target "pcre2::pcre2-8-static" contains:

    Threads::Threads

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  /usr/lib/x86_64-linux-gnu/cmake/pcre2/pcre2-config.cmake:66 (include)
  CMakeLists.txt:6 (find_package)


-- Generating done

the same happens when trying to use the shared libraries (ex: commenting out the set(PCRE2_USE_STATIC_LIBS) call) as well.

of course I could add find_package(Threads) before doing the one that calls PCRE2 and that might even allow me to build the application, but again, doing so is not backward compatible.

maybe it would be a good idea to change this PR back into a draft and check the code with some people that really know cmake?

let me know if maybe providing a whole container where the issue is reproducible might help debug, but as I mentioned in my first comment, we are not the place where you can get feedback on your cmake code, so I will unsubscribe from this, since I am not doing any "reviewing".

I have to admit I was really looking forward to fix those 2 bugs I found with this PR, but seems that somehow hacking in the old code works even in Windows, unlike this one. I would suggest rebasing on top of those commits to minimize conflicts for both of us otherwise, and thanks for your indirect suggestions for fixes through the "reviewing" of this one.

@dg0yt
Copy link

dg0yt commented Jun 10, 2023

of course I could add find_package(Threads) before doing the one that calls PCRE2 and that might even allow me to build my application, but again, doing so is not backward compatible.

For static linkage, the cmake config file needs to call find_dependency to recreate the targets which are used as target link libraries.
(vcpkg CI doesn't run unit tests, but it does tests static linkage in a full integration test with all affected ports. IMO it would be fair to test the changes in a draft PR there, given that vcpkg benefits from the permanent solution And extra tests might be enabled during the draft state..)

teo-tsirpanis added a commit to teo-tsirpanis/vcpkg that referenced this pull request Jun 13, 2023
The patch has been submitted upstream in PCRE2Project/pcre2#260.
@teo-tsirpanis
Copy link
Contributor Author

Any additional feedback before syncing the vcpkg patch @dg0yt? CI passes on vcpkg before some recent changes, and because it takes four hours I don't want to make frequent pushes.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
# to set the variable PCRE2_USE_STATIC_LIBS to ON before calling find_package.
# To make use of the static library instead of the shared one if both have been
# built, one needs to set the variable PCRE2_USE_STATIC_LIBS to ON before calling find_package.
# PCRE2_USE_STATIC_LIBS will be ignored if only the static or the shared library has been built.
Copy link

Choose a reason for hiding this comment

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

I'm unsure if ignore is the desired behavior if PCRE2_USE_STATIC_LIBS is set. (Vcpkg wants it because the triplet determines the linkage.) In general, CMake config mode would be able to continue search for config in other locations - and it might find another one which provides static libs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. If PCRE2_USE_STATIC_LIBS is set, the shared libraries will not be imported.

)
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.

Comment on lines 55 to 58
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?

cmake/pcre2-config.cmake.in Outdated Show resolved Hide resolved
_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.

@teo-tsirpanis teo-tsirpanis requested a review from dg0yt June 15, 2023 12:10
@teo-tsirpanis
Copy link
Contributor Author

@dg0yt can you take another look?

ihnorton pushed a commit to TileDB-Inc/TileDB that referenced this pull request Jul 18, 2023
[SC-29902](https://app.shortcut.com/tiledb-inc/story/29902/support-acquiring-libmagic-from-vcpkg)

This PR ports libmagic, the last of our external dependencies to have been exclusively available via `ExternalProject`, to vcpkg.

[The upstream libmagic vcpkg port](https://github.com/microsoft/vcpkg/tree/master/ports/libmagic) was unsuitable for our use because it uses `autoconfig` and `make`, and therefore does not expose CMake targets. For this reason I created a custom CMake-powered port, inspired from both the upstream port and [our own fork of libmagic that we have been using](https://github.com/TileDB-Inc/file-windows).

In case `regex.h` is not available (namely on Windows), the upstream vcpkg port used the tre library. This vcpkg port always uses pcre2 for consistency, matching our fork's behavior. Pcre2's CMake integration had some problems so I added a custom port for it as well.

I have also opened PCRE2Project/pcre2#260 to fix pcre2's CMake integration upstream, and after that gets merged I plan to contribute these two custom ports to upstream vcpkg.

It builds locally on Linux; on Windows it has some issues that might or might not occur due to my local configuration. Let's see what CI thinks.

---
TYPE: IMPROVEMENT
DESC: Support acquiring libmagic from vcpkg.
@teo-tsirpanis teo-tsirpanis marked this pull request as ready for review July 25, 2023 15:47
@teo-tsirpanis
Copy link
Contributor Author

@dg0yt is it OK?

@PhilipHazel
Copy link
Collaborator

I was waiting for somebody who understands Cmake to say ok or not, but if nobody does, I will do the merge sometime.

@dg0yt
Copy link

dg0yt commented Aug 5, 2023

I did't see anything concerning with regard to the new config, but I didn't look for migrations.
We can test it in vcpkg CI (which would cover popular ports for GDAL, POCO, Qt, wxWidgets). Better to see it fail now, not after the next release.

@dg0yt
Copy link

dg0yt commented Aug 5, 2023

We can test it in vcpkg CI (which would cover popular ports for GDAL, POCO, Qt, wxWidgets). Better to see it fail now, not after the next release.

... only that vcpkg never provided the previous CMake config because of the problems it had.

@PhilipHazel PhilipHazel merged commit def175f into PCRE2Project:master Aug 15, 2023
@PhilipHazel
Copy link
Collaborator

This seems to have settled; it works for me on Linux (which is all I can test on) and all the GitHub tests pass, so I have now done the merge.

@teo-tsirpanis teo-tsirpanis deleted the cmake-fix branch August 25, 2023 15:08
teo-tsirpanis added a commit to teo-tsirpanis/vcpkg that referenced this pull request Aug 25, 2023
The patch has been submitted upstream in PCRE2Project/pcre2#260.
vicroms pushed a commit to microsoft/vcpkg that referenced this pull request Sep 22, 2023
* Remove dependencies to bzip2 and zlib.

They are used only by pcre2grep which we don't build in the port.

* Fix CMake integration.

The patch has been submitted upstream in PCRE2Project/pcre2#260.

* Add a usage file and use `vcpkg_install_copyright`.

* Bring back patching pcre2.h.

* Bump port version.

* Update version database.

* Fix CRLF damage.

* Fix target capitalization.

* Update version database.

---------

Co-authored-by: Billy Robert O'Neal III <[email protected]>
carenas added a commit to carenas/pcre2 that referenced this pull request Dec 22, 2023
PCRE2Project#260)"

breaks `make dist` and while it might be tweaked further, is likely to
need time to stabilize so doing so has been punted for now.

This mostly reverts commit def175f.
PhilipHazel pushed a commit that referenced this pull request Dec 23, 2023
#260)" (#364)

breaks `make dist` and while it might be tweaked further, is likely to
need time to stabilize so doing so has been punted for now.

This mostly reverts commit def175f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cmake --installincorrectly installs development binaries in production
4 participants