WEBKIT_FRAMEWORK should not modify file-global include directories, it should only modify target include directories.
Created attachment 331354 [details] Patch
That's right direction, but WebCore certainly needs to clean up the mess, and maybe not only WebCore
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.
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.
(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.
(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.
Created attachment 331496 [details] Patch
(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. ;)
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})
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
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
(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
Note that we don't have to invent new variables anymore, as Platform files can use target-specific commands directly now
(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....
(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.
>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
Let's see how this does on EWS....
Created attachment 331521 [details] Patch
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?
(In reply to Konstantin Tokarev from comment #19) > Looks great overall, but couldn't this be PRIVATE too? Um, yes.
(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....
Committed r227090: <https://trac.webkit.org/changeset/227090>
<rdar://problem/36598424>