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

Jump to bottom

Add target_include_directories to CMakefile #113

Merged
merged 1 commit into from
May 3, 2022
Merged

Add target_include_directories to CMakefile #113

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,7 @@ 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})
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@carenas carenas Apr 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

@carenas carenas Apr 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

IF(REQUIRE_PTHREAD)
TARGET_LINK_LIBRARIES(pcre2-8-static Threads::Threads)
ENDIF(REQUIRE_PTHREAD)
Expand All @@ -716,6 +717,7 @@ IF(PCRE2_BUILD_PCRE2_8)
SOVERSION ${LIBPCRE2_POSIX_SOVERSION})
TARGET_LINK_LIBRARIES(pcre2-posix-static pcre2-8-static)
TARGET_COMPILE_DEFINITIONS(pcre2-posix-static PUBLIC PCRE2_STATIC)
TARGET_INCLUDE_DIRECTORIES(pcre2-posix-static PUBLIC ${PROJECT_BINARY_DIR})
SET(targets ${targets} pcre2-posix-static)

IF(MSVC)
Expand All @@ -732,6 +734,7 @@ 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})
SET_TARGET_PROPERTIES(pcre2-8-shared PROPERTIES
COMPILE_DEFINITIONS PCRE2_CODE_UNIT_WIDTH=8
MACHO_COMPATIBILITY_VERSION "${LIBPCRE2_8_MACHO_COMPATIBILITY_VERSION}"
Expand All @@ -744,6 +747,7 @@ IF(PCRE2_BUILD_PCRE2_8)
ENDIF(REQUIRE_PTHREAD)
SET(targets ${targets} pcre2-8-shared)
ADD_LIBRARY(pcre2-posix-shared SHARED ${PCRE2POSIX_HEADERS} ${PCRE2POSIX_SOURCES})
TARGET_INCLUDE_DIRECTORIES(pcre2-posix-shared PUBLIC ${PROJECT_BINARY_DIR})
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 @@ -778,6 +782,7 @@ 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})
SET_TARGET_PROPERTIES(pcre2-16-static PROPERTIES
COMPILE_DEFINITIONS PCRE2_CODE_UNIT_WIDTH=16
MACHO_COMPATIBILITY_VERSION "${LIBPCRE2_32_MACHO_COMPATIBILITY_VERSION}"
Expand All @@ -802,6 +807,7 @@ 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})
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 @@ -836,6 +842,7 @@ 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})
SET_TARGET_PROPERTIES(pcre2-32-static PROPERTIES
COMPILE_DEFINITIONS PCRE2_CODE_UNIT_WIDTH=32
MACHO_COMPATIBILITY_VERSION "${LIBPCRE2_32_MACHO_COMPATIBILITY_VERSION}"
Expand All @@ -860,6 +867,7 @@ 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})
SET_TARGET_PROPERTIES(pcre2-32-shared PROPERTIES
COMPILE_DEFINITIONS PCRE2_CODE_UNIT_WIDTH=32
MACHO_COMPATIBILITY_VERSION "${LIBPCRE2_32_MACHO_COMPATIBILITY_VERSION}"
Expand Down