Bug 198752

Summary: [WPE][GTK] Fix build with unified sources disabled
Product: WebKit Reporter: Adrian Perez <aperez>
Component: Tools / TestsAssignee: Adrian Perez <aperez>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, mcatanzaro, megan_gardner, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Linux   
Bug Depends on: 198722    
Bug Blocks:    
Attachments:
Description Flags
List of build errors fixed
none
Patch
none
Patch for landing
none
Patch v2
none
Patch v3 none

Description Adrian Perez 2019-06-11 05:47:05 PDT
Both the WPE and GTK WebKit ports cannot be built when passing
the “-DENABLE_UNIFIED_BUILDS=OFF” option to CMake. Fixing the
build with that will avoid mystery build failures when usage
of unified sources is enabled as well.
Comment 1 Adrian Perez 2019-06-11 07:53:30 PDT
Created attachment 371844 [details]
List of build errors fixed

I am attaching a (cleaned up) build log of the errors that
prevent building with “-DENABLE_UNIFIED_BUILDS=OFF”.
Comment 2 Adrian Perez 2019-06-11 07:53:57 PDT
Created attachment 371845 [details]
Patch
Comment 3 Adrian Perez 2019-06-11 07:55:55 PDT
(In reply to Adrian Perez from comment #2)
> Created attachment 371845 [details]
> Patch

This patch makes non-unified builds of the WPE and GTK ports
work for me with “trunk” using the default build options,
when the build is configured directly by invoking CMake.

Unified builds still work fine for the two ports; checked
using “build-webkit”.
Comment 4 Michael Catanzaro 2019-06-11 07:59:40 PDT
Comment on attachment 371845 [details]
Patch

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

It looks like you considered forward-declarations in headers whenever possible, which was the only thing that really needed to be reviewed here. Good, and thanks!

> Source/WebKit/WebProcess/WebPage/wpe/WebPageWPE.cpp:32
> +#if HAVE(ACCESSIBILITY)
> +#include "WebPageProxy.h"
> +#endif

Just #include it always, adding guards only increases the odds of configuration-dependent build failures and has almost zero benefit.

If it really deserves guards (it doesn't) then it would need to be #included in a separate block, at the bottom.
Comment 5 Adrian Perez 2019-06-11 08:09:33 PDT
(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 371845 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371845&action=review
> 
> It looks like you considered forward-declarations in headers whenever
> possible, which was the only thing that really needed to be reviewed here.
> Good, and thanks!

Yes, I tried to avoid blindly adding #includes to avoid increasing
the build time =)

> > Source/WebKit/WebProcess/WebPage/wpe/WebPageWPE.cpp:32
> > +#if HAVE(ACCESSIBILITY)
> > +#include "WebPageProxy.h"
> > +#endif
> 
> Just #include it always, adding guards only increases the odds of
> configuration-dependent build failures and has almost zero benefit.
> 
> If it really deserves guards (it doesn't) then it would need to be #included
> in a separate block, at the bottom.

Makes sense. I will apply your suggestion before landing.

Also, I have to fix this build issue caught by the Mac EWSs:

  In file included from ./Modules/indexeddb/server/MemoryObjectStore.cpp:39:
  ./Modules/indexeddb/server/UniqueIDBDatabase.h:54:1: error: class 'IDBGetRecordData' was previously declared as a struct [-Werror,-Wmismatched-tags]
  class IDBGetRecordData;
  ^
Comment 6 Adrian Perez 2019-06-11 08:18:52 PDT
Created attachment 371846 [details]
Patch for landing
Comment 7 Adrian Perez 2019-06-11 09:54:33 PDT
(In reply to Adrian Perez from comment #6)
> Created attachment 371846 [details]
> Patch for landing

…aaaand another Mac failure to take care of:

In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource6.cpp:2:
In file included from /Volumes/Data/EWS/WebKit/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:40:
In file included from /Volumes/Data/EWS/WebKit/Source/WebKit/UIProcess/WebFrameProxy.h:32:
/Volumes/Data/EWS/WebKit/Source/WebKit/UIProcess/WebPageProxy.h:1162:95: error: unknown type name 'DataCallback'; did you mean 'Messages::WebPageProxy::DataCallback'?
    void drawPagesToPDF(WebFrameProxy*, const PrintInfo&, uint32_t first, uint32_t count, Ref<DataCallback>&&);
                                                                                              ^~~~~~~~~~~~
                                                                                              Messages::WebPageProxy::DataCallback
Comment 8 Adrian Perez 2019-06-19 05:39:28 PDT
Created attachment 372454 [details]
Patch v2


Should fix the Mac build issue, and adds a few more fixes needed
for non-unified builds to complete successfully.
Comment 9 Adrian Perez 2019-06-19 07:13:40 PDT
Created attachment 372459 [details]
Patch v3


Another stab at fixing the Mac build issue.
Comment 10 Adrian Perez 2019-06-19 10:18:26 PDT
Comment on attachment 372459 [details]
Patch v3

Setting cq+ — I'll keep an eye on the bots, but it looks
quite safe to land this now that all the EWS bots are green :)
Comment 11 WebKit Commit Bot 2019-06-19 10:42:12 PDT
Comment on attachment 372459 [details]
Patch v3

Clearing flags on attachment: 372459

Committed r246596: <https://trac.webkit.org/changeset/246596>
Comment 12 WebKit Commit Bot 2019-06-19 10:42:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-06-19 10:43:13 PDT
<rdar://problem/51902752>