Add target_include_directories to CMakefile

[GregThain]

The pcre2 CMakeLists.txt works great for building pcre2 standalone. However, many projects want to vendor their own copy of pcre2, maybe to get a more recent version, or to compile with their own flags. To do this, CMake based projects can just cmake add_subdirectory, and the various library targets, like pcre2-8-shared appear in their root Cmakefiles, all ready to use. However, the root project needs to explicitly add the include directory when compile against pcre2, which is unfortunate. Modern cmake provides a method, target_include_directories, which attaches the name of this include directory to the target, so that cmake knows which include directory to add to the -I compile like when the target is used. This small PR adds those target_include_directory lines to the CMakeLists.txt.

[carenas]

The pcre2 CMakeLists.txt works great for building pcre2 standalone

I wouldn't agree with this statement; it does work but is suboptimal, overly complex and has known bugs and more importantly is used less (and therefore less tested) than the autotools (or even the do it by hand) option.

If you would "vendor" this library not using cmake (like other codebases do), would be probably better, but more details about why you decided to do it with cmake might be useful.

[carenas]

why is it PUBLIC and not PRIVATE or INTERFACE, and also why does PCRE need to include your project headers, is there some code change that goes along with this and that is missing from this proposal?

[GregThain]

INTERFACE would work just fine. It can't be PRIVATE, because we want to tell clients of the pcre2 library targets of the need to add -I to compile lines of sources that use pcre2. Note that cmake's PROJECT_BINARY_DIR is per-cmake project(), and refers to the pcre2 build directory in this case. So, this directive does not cause cmake to pollute pcre2's build with any external headers or directories, it tells users of the pcre2 library target where to find header files.

Consider a minimal example. Assume I have a C source file, mytest.c, that just #include's "pcre2.h", with a trivial main() and a CMakeLists.txt that looks like this

project(mytest LANGUAGES C)

cmake_minimum_required(VERSION 3.0)

include(FetchContent)
FetchContent_declare( pcre2
  GIT_REPOSITORY https://github.com/PCRE2Project/pcre2
  GIT_TAG master
)
FetchContent_MakeAvailable(pcre2)

add_executable(mytest mytest.c)
target_link_libraries(mytest pcre2-8)

This fails to build with the following error:

Building C object CMakeFiles/mytest.dir/mytest.c.o
/usr/bin/cc -DPCRE2_STATIC   -o CMakeFiles/mytest.dir/mytest.c.o   -c /tmp/mytest.c
/tmp/mytest.c:1:10: fatal error: pcre2.h: No such file or directory
    1 | #include "pcre2.h"
      |          ^~~~~~~

Note that because the pcre2 CMake file helpfully contains

TARGET_COMPILE_DEFINITIONS(pcre2-8-static PUBLIC PCRE2_STATIC)

-DPCRE2_STATIC is added to the compile line when building my client of the library (I suspect this is only needed on Windows, but never mind that). In the same way that we bring this -D into my build, I'd like the appropriate -I to be added as well. And indeed, with this patch, with either PUBLIC or INTERFACE, we see an additional -I line added to the compile of mytest.c, and it successfully compiles and links.

[carenas]

I think what you are getting at is that since our CMakefile syntax is so ancient that we don't provide a useful import target, you need something like what you proposed to be able to get the include headers just like you got the defines, right?

BTW FetchContent is only available with 3.11 and FetchContent_MakeAvailable with 3.14, so presume your cmake_minimum_version has a bug.

Until then, adding something like the following to your cmake would do the trick:

target_include_directories(mytest PRIVATE ${pcre2_BINARY_DIR})

[GregThain]

I think what you are getting at is that since our CMakefile syntax is so ancient that we don't provide a useful import target, you need something like what you proposed to be able to get the include headers just like you got the defines, right?

Exactly. But, I wouldn't say the pcre2 Cmake syntax is super ancient. It almost works for this use case, save for the INCLUDE_DIRECTORIES.

Until then, adding something like the following to your cmake would do the trick:

target_include_directories(mytest PRIVATE ${pcre2_BINARY_DIR})

This is what I do today, but seems like this PCRE2 specific knowledge is best stored and maintained in the pcre2 Cmakefile, don't you think? And thus this PR.

[carenas]

seems like this PCRE2 specific knowledge is best stored and maintained in the pcre2 Cmakefile, don't you think? And thus this PR.

agree, but it feels like a hack; it works because the last project command cmake saw was pcre2 (which might be still correct, I am no cmake expert so there is no way I would know).

I think it would be ideal to instead go all the way and do at least the include part of what is required to make this work correctly like a proper modern cmake target should do, which should at least include both install and build interfaces:

target_include_directories($TARGET INTERFACE
  $<BUILD_INTERFACE:..>
  $<INSTALL_INTERFACE:..>
)

it would be also nice to avoid all the copy/pasting so it is written only once, specially since now we got a cmake expert that could do so

maybe we could even go all the way and make the full interface work without the current quircks of maybe setting the wrong defines for the wrong platform, don't you think?

[carenas]

Just to clarify; my concern with this change is that somebody already did part of it and for whatever reason decided NOT to do this part (maybe they have an old CMake or the order their includes are setup is very important).

While your change is correct and solves YOUR case, and more importantly is a step in the right direction, it might break theirs (maybe they have something like I what proposed and that you are obviously going to remove in tandem with this change), so the least we can do is to look at it carefully and try to do it as best as we can, so if it ends up breaking their setup, they might be at least happy to now realize they might had been using it wrong all this time and now they might be able to ALSO remove their workarounds.

[GregThain]

If you would "vendor" this library not using cmake (like other codebases do), would be probably better, but more details about why you decided to do it with cmake might be useful.

There are a couple of reasons why I'd prefer to vendor with CMake, but I'm open to other alternatives. First, my project is cross platform, and autotools doesn't work on Windows (and CMake ships with vc++ now). Second, all the rest of my project is built with CMake, so it is convenient to use one (meta) build system for everything.

[carenas]

autotools doesn't work on Windows

while I wouldn't say it is ideal, msys2[1] does provide autotools for Windows natively, among other far more useful things.

but it would seem from the rest of your comment that you are ideally suited to enhance our cmake experience (we are opensource after all), and if your development environment is mainly Windows, then you should be happy to know that our cmake support was built to help you and therefore might be more reliable there.

the caveat about cmake not being ideal outside of Windows still applies though, so you'd need to consider how important your non Windows users are and how much work are you willing to do to make sure your use of our vendored through cmake library still works for them.

[1] https://www.msys2.org/

[PhilipHazel]

Has this discussion reached a conclusion? I know practically nothing about CMake, so I rely on external advice. Should I or should I not merge this pull request?

[carenas]

Should I or should I not merge this pull request?

After the explanation, I have no objection; but still think we could do slighly better by:

• describing in the commit message that this is a deficiency of the current CMakeFiles.txt and why it is needed
• if possible make it simpler and less repetitive

Having to include the PCRE2 headers is not unfortunate, it is part of how you integrate with PCRE2 and the current solution doesn't provide the cmake way to do that, so whoever vendors our library might need to use the pkgconf way or the by hand way or not use cmake.

Somehow previous changes implemented part of it, so either we implement a little bit more so it works also in this case, or we look at it hard and make sure it is implemented the right way all the way, modernizing our cmake and using current best practices in the process.

[dg0yt]

I just touched the vcpkg port of pcre2, which is using CMake, and I came here out of curiosity for pending CMake issues or PRs.

IMO adding including directories to individual targets should be complemented by removing the now-obsolete global setting,

pcre2/CMakeLists.txt

Line 124 in 15a82c3

INCLUDE_DIRECTORIES(${PROJECT_SOURCE_DIR}/src)

However, the global setting uses the src subdir. Shouldn't this be done in the new statements, too?

It will need more modifications when pcre2's CMake build system learns to actually export CMake config for downstream usage. (ATM there is a hand-written config file which operates like a Find module.) And this leads to another comment:
Projects which embed a copy of pcre2 shouldn't use the actual (internal) targets, but CMakeLists.txt should create alias targets which match the namespaced target names from the CMake config. This makes it easy to switch between the embedded copy and an external build of PCRE2.

[carenas]

However, the global setting uses the src subdir. Shouldn't this be done in the new statements, too?

AFAIK whoever is using this, decided they wanted to push the created file somewhere else than src, since in CMake most of the time the source and binary directories are completely independent unlike autotools.

Note also that even in autotools (after you do ./autogen.sh of course, or when using a release package) we support creating an independent directory to build on (for example):

mkdir build && cd build
../configure

and in that case pcre2.h is generated inside that directory (in a "src" subdirectory, though), so it would seem to me that it would be a "regression" to move it now, unless you are advocating for ${PROJECT_BINARY_DIR}/src which will make it consistent by both build systems.

of course, moving the files around is even more likely to affect the setup of current users, so might not be wise; but having a clear idea of how to get our CMake configuration so it is usable for people vendoring or alternatively using an installed version seems like a useful thing to do, even if it seems the cmake folks themselves had given up[1] on us already.

[1] https://gitlab.kitware.com/cmake/cmake/-/issues/22061

[dg0yt]

Sorry, I didn't notice the BINARY vs. SOURCE difference in the variable names. I didn't want to suggest moving around any files.

The part about the targets and exported config remains valid IMO. (I don't think the CMake gave up on pcre2, they rather gave up on adding find modules, for good reasons.)

[GregThain]

My goal here was to propose the minimal change required for a vendored cmake pcre2 to work. If folks are interested in a more comprehensive change, I'd be willing to write up something, but it will take several weeks until I will have the time to do so.

As far as the INCLUDE_DIRECTORIES pointing into the source, yes, that could be re-implemented as a PRIVATE TARGET_INCLUDE_DIRECTORIES for all the libraries, but I don't think that makes much functional difference. In some ways, the "old, bad" way with one INCLUDE_DIRECTORIES statement that applies to all the targets in the file works better, otherwise, we have to repeat the TARGET_INCLUDE_DIRECTORIES for each library, violating the "Don't repeat yourself" principle.

@dg0yt There's a comment in the vcpkg pcre2 module about how pcre2's cmake files aren't modern enough for vcpkg. Do you what, specifically, vcpkg needs from the pcre2 CMake? It would be nice to get that fixed too, while we are in here. I suspect it isn't much of a change.

[PhilipHazel]

It would be really great if a CMake expert could take this in hand and provide the project with an up-do-date (and hopefully, maintained) CMake configuration. Can you do this, Greg? Timing is not at all urgent, as 10.40 is a recent release and there probably won't be another one for several months (unless a showstopper turns up). The GitHub "build" workflow does test CMake under Windows; should there be a test under Linux as well? (I do a simple manual test on my Arch Linux box before each release).

In the meantime, shall I go ahead and merge this pull request?

[PhilipHazel]

Oh, and note #104 -- is that relevant?

[dg0yt]

I didn't want to go off-topic. Basically I wanted to say that the change is good for now.

@dg0yt There's a comment in the vcpkg pcre2 module about how pcre2's cmake files aren't modern enough for vcpkg. Do you what, specifically, vcpkg needs from the pcre2 CMake? It would be nice to get that fixed too, while we are in here. I suspect it isn't much of a change.

It is not my comment, but I could give some background.

It would be really great if a CMake expert could take this in hand and provide the project with an up-do-date (and hopefully, maintained) CMake configuration.

Indeed, but it doesn't need to be rushed in. The change should start with adding a post-install (usage) test. (That's what helped with improvements in GDAL.)
And you should ask yourself if you must really force autotools patterns (static and shared in one build) on CMake. Cf. 2410fbe#commitcomment-68176307.

[dg0yt]

Oh, and note #104 -- is that relevant?

No.

[carenas]

And you should ask yourself if you must really force autotools patterns (static and shared in one build) on CMake

believe it or not, that was something that was requested recently in 2410fbe; the original cmake didn't have it and if that is preventing for it to be cleaned it up, it might be worth removing IMHO, once we figure out why whoever requested it though it might be needed, of course.

Sadly that change predate the git migration and so some of the discussion might had been lost in the old bugzilla but the email thread still exists in :

https://lists.exim.org/lurker/message/20210701.070030.8dbb0b5b.en.html

[ltrzesniewski]

The GitHub "build" workflow does test CMake under Windows

Note that the workflow didn't run for this PR yet, as by default GitHub requires an approval to run workflows for PRs from first-time contributors.

I suppose you should approve the workflow before merging the PR in order to make sure everything works correctly.

Alternatively, you can disable this security feature in the project options if you wish.

[dg0yt]

I created a separate issue for CMake discussions: #115

[PhilipHazel]

I saw the request for workflow approval, but I had forgotten that this tests CMake as well as the library code, so didn't immediately let them go. Now running.