Bug 207159

Created attachment 389586 [details]
Patch

Created attachment 389593 [details]
Patch

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

(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

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

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)

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

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.

I stand corrected, indeed it works this way

Comment on attachment 389593 [details]
Patch

Clearing flags on attachment: 389593

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

All reviewed patches have been landed.  Closing bug.

<rdar://problem/59147075>