Bug 207159 – [CMake] Add Cairo::Cairo target

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

[CMake] Add Cairo::Cairo target

[Ross Kirsling]

Created attachment 389586 [details]
Patch

[Ross Kirsling]

Created attachment 389593 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 389593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389593&action=review

> Source/WebCore/platform/Cairo.cmake:-22
> -    ${CAIRO_INCLUDE_DIRS}

Cairo::Cairo should use INTERFACE_SYSTEM_INCLUDE_DIRECTORIES instead of INTERFACE_INCLUDE_DIRECTORIES, otherwise you are replacing system includes with non-system

[Don Olmstead]

(In reply to Konstantin Tokarev from comment #3)
> Comment on attachment 389593 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389593&action=review
> 
> > Source/WebCore/platform/Cairo.cmake:-22
> > -    ${CAIRO_INCLUDE_DIRS}
> 
> Cairo::Cairo should use INTERFACE_SYSTEM_INCLUDE_DIRECTORIES instead of
> INTERFACE_INCLUDE_DIRECTORIES, otherwise you are replacing system includes
> with non-system

I don't see any cases in the CMake repository of a Find module using INTERFACE_SYSTEM_INCLUDE_DIRECTORIES when it creates a target.

https://gitlab.kitware.com/search?utf8=%E2%9C%93&search=INTERFACE_SYSTEM_INCLUDE_DIRECTORIES&group_id=&project_id=541&search_code=true&repository_ref=master&nav_source=navbar

[Konstantin Tokarev]

Most people are probably fine with seeing compiler warnings from library includes, however in WebKit we have enough of our own

[Konstantin Tokarev]

Note that it's easy to system includes with 3rd party CMake targets, however it requires keeping WebCore_SYSTEM_INCLUDE_DIRECTORIES instead of removing them (fwiw, no compiler flag duplication occurs from that)

[Konstantin Tokarev]

I guess if built-in find modules used INTERFACE_SYSTEM_INCLUDE_DIRECTORIES, it just wouldn't be possible for end users to uses them as non-system, because system takes priority

[Stephan Szabo]

According to https://gitlab.kitware.com/cmake/cmake/issues/17374 INTERFACE_INCLUDE_DIRECTORIES from imported targets should be added as system by default.

A simple test with a CMakeLists.txt:
cmake_minimum_required(VERSION 3.13)

project(x)
find_package(XXX REQUIRED)
add_executable(app app.cpp)
target_link_libraries(app XXX)

and a FindXXX.cmake:
add_library(XXX UNKNOWN IMPORTED GLOBAL)
set_target_properties(XXX PROPERTIES
 INTERFACE_INCLUDE_DIRECTORIES "C:/github/tests/import/XXX"
)

results for the ninja generator at least in:
build CMakeFiles/app.dir/app.cpp.obj: CXX_COMPILER__app ../app.cpp || cmake_object_order_depends_target_app
  DEP_FILE = CMakeFiles\app.dir\app.cpp.obj.d
  FLAGS = -g -Xclang -gcodeview -O0 -D_DEBUG -D_DLL -D_MT -Xclang --dependent-lib=msvcrtd
  INCLUDES = -isystem ../XXX
  OBJECT_DIR = CMakeFiles\app.dir
  OBJECT_FILE_DIR = CMakeFiles\app.dir
  TARGET_COMPILE_PDB = CMakeFiles\app.dir\
  TARGET_PDB = app.pdb

So, using -isystem on the INCLUDES line for that lib's includes.

[Konstantin Tokarev]

I stand corrected, indeed it works this way

[WebKit Commit Bot]

Comment on attachment 389593 [details]
Patch

Clearing flags on attachment: 389593

Committed r255670: <https://trac.webkit.org/changeset/255670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59147075>