Bug 132555

Summary: CMake per-target property INCLUDE_DIRECTORIES incompatible with CMake 2.8.7
Product: WebKit Reporter: Nikos Andronikos <nikos.andronikos>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: gustavo, mrobinson, rakuco, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   

Description Nikos Andronikos 2014-05-04 21:00:55 PDT
Background:
My machine configuration:
Architecture: x86_64
Linux Distribution: Ubuntu 12.04
GCC/G++ version: 4.8
CMake version: 2.8.7

By tracing back the error: "fatal error: gtk/gtk.h: No such file or directory" on my system I found out that the problem is due to some cmake instructions used in the build system (more details below).

Problem:
The per-target INCLUDE_DIRECTORIES set_property command is included in numerous files.
For example, WebCore/PlatformGTK.cmake:

set_property(
    TARGET WebCorePlatformGTK
    APPEND
    PROPERTY INCLUDE_DIRECTORIES
        ${WebCore_INCLUDE_DIRECTORIES}
        ${GTK_INCLUDE_DIRS}
        ${GDK_INCLUDE_DIRS}
)

This is not compatible with CMake 2.8.7 (it was introduced in CMake 2.8.8 as indicated here http://www.cmake.org/Wiki/CMake_Version_Compatibility_Matrix/Properties_on_Targets).

Possible solutions:
1. Check for CMake versions below 2.8.8 and throw up an error message indicating that the cmake build system is not compatible with earlier versions
2. Try to work around it by manipulating the includes depending on the target directory

Any thoughts?
Comment 1 Raphael Kubo da Costa (:rakuco) 2014-05-05 04:49:09 PDT
How easy is it for Ubuntu 12.04 users to move to a newer CMake? From a purely technical point of view just raising the CMake requirements to 2.8.8 would be the easiest approach (it's a matter of changing the fist line of the top-level CMakeLists.txt).
Comment 2 Martin Robinson 2014-05-05 07:47:55 PDT
The GTK+ port would like maintain compatibility for old CMake versions as long as possible. Probably the best thing to do here is to make a macro that checks the CMake version and falls back to setting the global include directory.
Comment 3 Nikos Andronikos 2014-05-05 22:25:28 PDT
(In reply to comment #2)
> The GTK+ port would like maintain compatibility for old CMake versions as long as possible. Probably the best thing to do here is to make a macro that checks the CMake version and falls back to setting the global include directory.

From what I understand the logic is currently structured as follows:

USE_GTK2 in Source/cmake/OptionsGTK.cmake is undefined.
Therefore, GTK_LIBRARIES, GTK_INCLUDE_DIRS etc.. are defined to use the GTK3/GDK3 libraries and include directories (Source/cmake/OptionsGTK.cmake:181).

Then, in the Source/WebCore/PlatformGTK.cmake file the following properties are set:
1. The WebCorePlatformGTK2 target uses the GTK2_INCLUDE_DIRS and GDK2_INCLUDE_DIRS
2. The WebCorePlatformGTK target uses the GTK_INCLUDE_DIRS and GDK_INCLUDE_DIRS (which are defined as GTK3/GDK3)

So if this is the required logic I am not sure that overriding with a global include directory is the right way to go.
Comment 4 Martin Robinson 2014-05-08 17:50:32 PDT
(In reply to comment #3)

> So if this is the required logic I am not sure that overriding with a global include directory is the right way to go.

That does present a bit of a problem then. Do you know how per-target includes were handled before 2.8.8?
Comment 5 Nikos Andronikos 2014-05-08 18:01:01 PDT
(In reply to comment #4)
> (In reply to comment #3)
> 
> > So if this is the required logic I am not sure that overriding with a global include directory is the right way to go.
> 
> That does present a bit of a problem then. Do you know how per-target includes were handled before 2.8.8?

According to this link:
http://www.cmake.org/pipermail/cmake/2013-February/053765.html

There seem to be a couple of options:
1. Create a folder per target and call include_directories() in each folder
2. Create a wrapper around the "add_target" command
Comment 6 Martin Robinson 2014-05-09 09:50:50 PDT
So, if my understanding is correct we should make the implicit requirement explicit and then open a bug about extending support into the past.