Bug 181656 – WEBKIT_FRAMEWORK should not modify file-global include directories

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Michael Catanzaro]

WEBKIT_FRAMEWORK should not modify file-global include directories, it should only modify target include directories.

[Michael Catanzaro]

Created attachment 331354 [details]
Patch

[Konstantin Tokarev]

That's right direction, but WebCore certainly needs to clean up the mess, and maybe not only WebCore

[Michael Catanzaro]

This broke the WPE EWS, I need to fix that.

GTK EWS seems to have gone crazy, it's the old "thinks it's not an ELF system" error we've never managed to figure out. I'd ignore that as I've tested it locally.

(In reply to Konstantin Tokarev from comment #2)
> That's right direction, but WebCore certainly needs to clean up the mess,
> and maybe not only WebCore

What do you mean?

I know we have a lot of room to tighten up include directories throughout the build system.

[Konstantin Tokarev]

I mean these include_directories() were used by other targets in same CMakeListsts.txt, e.g. WebCoreTestSupport, WebCoreDerivedSources, so there may be side effects. But if you tested it locally it's fine. Also note that as you made these system include directories PUBLIC they may be propagated to targets which weren't using them before.

[Michael Catanzaro]

(In reply to Konstantin Tokarev from comment #4)
> I mean these include_directories() were used by other targets in same
> CMakeListsts.txt, e.g. WebCoreTestSupport, WebCoreDerivedSources, so there
> may be side effects. But if you tested it locally it's fine.

Fine except for WPE, yes. ;)

Well, and maybe Mac... who knows about Mac, since there is no CMake EWS.

> Also note that
> as you made these system include directories PUBLIC they may be propagated
> to targets which weren't using them before.

Hm, I suppose that was not intentional... I had to pick PUBLIC/PRIVATE/INTERFACE, but was thinking "PUBLIC is the default for normal include_directories()". But that's wrong: I should have used PRIVATE.

Probably better for me to take the time to do this properly.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> Probably better for me to take the time to do this properly.

It's a bit silly how complex it is to decide whether the include directories should be part of the target's interface or not. And we don't really want _SYSTEM_INCLUDE_DIRECTORIES to be surprisingly different from normal _INCLUDE_DIRECTORIES is anything but SYSTEM. So I'll stick with the original plan here, and just try to fix the WPE build error.

[Michael Catanzaro]

Created attachment 331496 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #6)
> It's a bit silly how complex it is to decide whether the include directories
> should be part of the target's interface or not.

This could be a big future cleanup, but I don't want to even think about tackling it in order to fix this obvious issue with WEBKIT_FRAMEWORK. ;)

[Konstantin Tokarev]

Comment on attachment 331496 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:1951
> +target_include_directories(WebCoreTestSupport PUBLIC ${WebCoreTestSupport_INCLUDE_DIRECTORIES} ${WebCore_INCLUDE_DIRECTORIES} ${WebCore_SYSTEM_INCLUDE_DIRECTORIES})

No, it should be a separate line

target_include_directories(WebCoreTestSupport SYSTEM PUBLIC ${WebCore_SYSTEM_INCLUDE_DIRECTORIES})

[Konstantin Tokarev]

Comment on attachment 331496 [details]
Patch

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

> Source/cmake/WebKitMacros.cmake:137
> +    target_include_directories(${_target} SYSTEM PUBLIC "$<BUILD_INTERFACE:${${_target}_SYSTEM_INCLUDE_DIRECTORIES}>")

And I would really use PRIVATE here. If something breaks we'll see quickly

[Konstantin Tokarev]

Comment on attachment 331496 [details]
Patch

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

>> Source/WebCore/CMakeLists.txt:1951
>> +target_include_directories(WebCoreTestSupport PUBLIC ${WebCoreTestSupport_INCLUDE_DIRECTORIES} ${WebCore_INCLUDE_DIRECTORIES} ${WebCore_SYSTEM_INCLUDE_DIRECTORIES})
> 
> No, it should be a separate line
> 
> target_include_directories(WebCoreTestSupport SYSTEM PUBLIC ${WebCore_SYSTEM_INCLUDE_DIRECTORIES})

SYSTEM PRIVATE, sorry

[Konstantin Tokarev]

(In reply to Michael Catanzaro from comment #6)
> It's a bit silly how complex it is to decide whether the include directories
> should be part of the target's interface or not.

All SYSTEM inlcude dirs were "directory-private" before your change, so it should be safe to make them PRIVATE (if satellite targets of WebCore are fixed, and maybe WebKit processes also)

Maybe a few things like ICU and GLib could be made SYSTEM PUBLIC, but most of system libraries will probably stay PRIVATE

[Konstantin Tokarev]

Note that we don't have to invent new variables anymore, as Platform files can use target-specific commands directly now

[Michael Catanzaro]

(In reply to Konstantin Tokarev from comment #9)
> > Source/WebCore/CMakeLists.txt:1951
> > +target_include_directories(WebCoreTestSupport PUBLIC ${WebCoreTestSupport_INCLUDE_DIRECTORIES} ${WebCore_INCLUDE_DIRECTORIES} ${WebCore_SYSTEM_INCLUDE_DIRECTORIES})
> 
> No, it should be a separate line

Yup, oops.

(In reply to Konstantin Tokarev from comment #10)
> And I would really use PRIVATE here. If something breaks we'll see quickly

OK....

[Michael Catanzaro]

(In reply to Konstantin Tokarev from comment #13)
> Note that we don't have to invent new variables anymore, as Platform files
> can use target-specific commands directly now

Can you point me to where you changed this? I tried modifying WebCoreTestSupport directly from WebCore/PlatformWPE.cmake but didn't have access to it.

[Konstantin Tokarev]

>Can you point me to where you changed this? 

In https://bugs.webkit.org/show_bug.cgi?id=174557

WebCoreTestSupport is not "forward-declared" with WEBKIT_FRAMEWORK_DECLARE, that's why it's inaccessible. Can be fixed easily

[Michael Catanzaro]

Let's see how this does on EWS....

[Michael Catanzaro]

Created attachment 331521 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 331521 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:1952
> +target_include_directories(WebCoreTestSupport SYSTEM PUBLIC ${WebCore_SYSTEM_INCLUDE_DIRECTORIES})

Looks great overall, but couldn't this be PRIVATE too?

[Michael Catanzaro]

(In reply to Konstantin Tokarev from comment #19)
> Looks great overall, but couldn't this be PRIVATE too?

Um, yes.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #20)
> (In reply to Konstantin Tokarev from comment #19)
> > Looks great overall, but couldn't this be PRIVATE too?
> 
> Um, yes.

But PUBLIC is used on the line above... so I'll leave it be for now. Lots of room for future cleanups....

[Michael Catanzaro]

Committed r227090: <https://trac.webkit.org/changeset/227090>

[Radar WebKit Bug Importer]

<rdar://problem/36598424>