Bug 182512 - [CMake] Make WebCore headers copies
Summary: [CMake] Make WebCore headers copies
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords: InRadar
Depends on: 182757 182766 182800
Blocks: 180064
  Show dependency treegraph
 
Reported: 2018-02-05 14:56 PST by Don Olmstead
Modified: 2019-04-29 06:41 PDT (History)
17 users (show)

See Also:


Attachments
WIP Patch (63.40 KB, patch)
2018-02-05 15:01 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (71.45 KB, patch)
2018-02-05 18:10 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (71.46 KB, patch)
2018-02-05 18:17 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (73.51 KB, patch)
2018-02-05 19:07 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (72.92 KB, patch)
2018-02-05 19:27 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Installed files for xcode frameworks (248.54 KB, text/plain)
2018-02-07 20:13 PST, Don Olmstead
no flags Details
WIP Patch (87.96 KB, patch)
2018-02-12 18:54 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (87.75 KB, patch)
2018-02-12 18:59 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (89.78 KB, patch)
2018-02-12 19:15 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (89.73 KB, patch)
2018-02-12 19:35 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (91.75 KB, patch)
2018-02-12 20:03 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (92.48 KB, patch)
2018-02-12 20:17 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
GTK and WPE changes (8.41 KB, patch)
2018-02-13 03:16 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (105.34 KB, patch)
2018-02-13 10:12 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (105.91 KB, patch)
2018-02-13 10:27 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (106.47 KB, patch)
2018-02-13 10:49 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (107.05 KB, patch)
2018-02-13 11:19 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (107.24 KB, application/zip)
2018-02-13 12:52 PST, Build Bot
no flags Details
WIP Patch (66.92 KB, patch)
2018-11-13 15:20 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (68.08 KB, patch)
2018-11-13 15:24 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (67.04 KB, patch)
2018-11-13 16:17 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (92.12 KB, patch)
2018-11-13 18:09 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (92.12 KB, patch)
2018-11-13 18:21 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (94.61 KB, patch)
2018-11-13 21:37 PST, Don Olmstead
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.72 MB, application/zip)
2018-11-13 22:21 PST, Build Bot
no flags Details
WIP Patch (94.62 KB, patch)
2018-11-13 22:34 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (95.51 KB, patch)
2018-11-19 19:42 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (95.51 KB, patch)
2018-11-19 19:47 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (98.14 KB, patch)
2018-11-20 10:51 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (95.32 KB, patch)
2018-11-20 12:22 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (99.60 KB, patch)
2018-11-20 12:57 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (99.63 KB, patch)
2018-11-20 13:13 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (99.63 KB, patch)
2018-11-20 13:38 PST, Don Olmstead
Hironori.Fujii: commit-queue-
Details | Formatted Diff | Diff
Patch (98.92 KB, patch)
2019-04-15 17:31 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (98.86 KB, patch)
2019-04-15 17:38 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (99.44 KB, patch)
2019-04-15 18:03 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (100.18 KB, patch)
2019-04-15 18:28 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (100.80 KB, patch)
2019-04-15 22:45 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (101.79 KB, patch)
2019-04-15 22:58 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (101.79 KB, patch)
2019-04-15 23:03 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (101.79 KB, patch)
2019-04-15 23:10 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (101.79 KB, patch)
2019-04-15 23:18 PDT, Don Olmstead
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.60 MB, application/zip)
2019-04-16 01:16 PDT, Build Bot
no flags Details
Patch (104.62 KB, patch)
2019-04-16 09:39 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (104.60 KB, patch)
2019-04-16 10:10 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (106.83 KB, patch)
2019-04-16 14:34 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (107.40 KB, patch)
2019-04-16 14:53 PDT, Don Olmstead
Hironori.Fujii: review-
Details | Formatted Diff | Diff
Patch (102.22 KB, patch)
2019-04-16 19:31 PDT, Don Olmstead
achristensen: review+
Details | Formatted Diff | Diff
Patch (5.51 KB, patch)
2019-04-18 10:58 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (109.55 KB, patch)
2019-04-18 10:59 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (109.54 KB, patch)
2019-04-18 11:14 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (110.09 KB, patch)
2019-04-18 12:19 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (110.07 KB, patch)
2019-04-18 14:25 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Build fix (1015 bytes, patch)
2019-04-18 16:39 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Build fix (1015 bytes, patch)
2019-04-18 16:42 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2018-02-05 14:56:47 PST
WebCore headers should be copied during the build to match the behavior of Apple ports.
Comment 1 Don Olmstead 2018-02-05 15:01:40 PST Comment hidden (obsolete)
Comment 2 Build Bot 2018-02-05 15:03:48 PST Comment hidden (obsolete)
Comment 3 Don Olmstead 2018-02-05 18:10:35 PST Comment hidden (obsolete)
Comment 4 Build Bot 2018-02-05 18:12:25 PST Comment hidden (obsolete)
Comment 5 Don Olmstead 2018-02-05 18:17:41 PST Comment hidden (obsolete)
Comment 6 Build Bot 2018-02-05 18:21:14 PST Comment hidden (obsolete)
Comment 7 Don Olmstead 2018-02-05 19:07:24 PST Comment hidden (obsolete)
Comment 8 Build Bot 2018-02-05 19:09:38 PST Comment hidden (obsolete)
Comment 9 Don Olmstead 2018-02-05 19:27:53 PST Comment hidden (obsolete)
Comment 10 Build Bot 2018-02-05 19:29:38 PST Comment hidden (obsolete)
Comment 11 Michael Catanzaro 2018-02-07 15:50:07 PST
Comment on attachment 333148 [details]
WIP Patch

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

> Source/WebCore/CMakeLists.txt:1020
> +set(WebCore_FORWARDING_HEADERS

This is really unfortunate. Forwarding headers are supposed to make things easier for us, not harder. Having a 1500 line list of header files to copy around is not great.

Maybe we should take a step back and ask: since forwarding headers are causing problems, why do we have them in the first place? What is the real purpose here? Other software projects don't do this: it's a WebKitism, and I'm not sure it's justified. What would it take to get rid of forwarding headers entirely?
Comment 12 Fujii Hironori 2018-02-07 17:47:56 PST
Don can use DIRECTORIES instead of FILES of WEBKIT_MAKE_FORWARDING_HEADERS.

(In reply to Michael Catanzaro from comment #11)
> Maybe we should take a step back and ask: since forwarding headers are
> causing problems, why do we have them in the first place? What is the real
> purpose here? Other software projects don't do this: it's a WebKitism, and
> I'm not sure it's justified. What would it take to get rid of forwarding
> headers entirely?

That's because WebKit is using #include <JavaScriptCore/VM.h> style instread of #include <JavaScriptCore/runtime/VM.h> style.
We can remove all forwarding headers if we use the latter style.

Include header path is a part of API. This change is a API breakage.
If Safari, for example, is including those header, it might be difficult to change.
Comment 13 Don Olmstead 2018-02-07 20:13:42 PST
Created attachment 333355 [details]
Installed files for xcode frameworks

(In reply to Michael Catanzaro from comment #11)
> Comment on attachment 333148 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333148&action=review
> 
> > Source/WebCore/CMakeLists.txt:1020
> > +set(WebCore_FORWARDING_HEADERS
> 
> This is really unfortunate. Forwarding headers are supposed to make things
> easier for us, not harder. Having a 1500 line list of header files to copy
> around is not great.
> 
> Maybe we should take a step back and ask: since forwarding headers are
> causing problems, why do we have them in the first place? What is the real
> purpose here? Other software projects don't do this: it's a WebKitism, and
> I'm not sure it's justified. What would it take to get rid of forwarding
> headers entirely?

Ok so this is going to be long winded because I am so far down the rabbit hole with this right now, but hopefully afterwards we can all see why the concept of forwarding headers needs to go away.

Within WebKit there are currently two types of forwarding headers.

Checked in files

This was the case for Source/WebCore/ForwardingHeaders where there was a directory structure matching the structure within Source/JavaScriptCore which would then point to the installed header. This was mainly caused by generated tools output, for example https://bugs.webkit.org/show_bug.cgi?id=182448 which was a dependent bug of https://bugs.webkit.org/show_bug.cgi?id=182347 which has many more examples.

These files can and should all go away.

Generated

This is what generate-forwarding-headers.pl is doing. It generates a file relative to the root Source directory that points to the actual file on disk and forwards includes to it.

Its actually been around for a long time, https://bugs.webkit.org/show_bug.cgi?id=44336, and appears to have been done despite the fact that Apple ports were copying since before then.

This file and its generated files should also go away.

So in the end my goal is to get rid of both of those types because they are causing problems, like with pragma once, and create an inconsistency between Xcode and CMake builds.

Remember that Xcode has this concept of public and private headers, the type of which is defined in Xcode. When you build a framework it installs those headers by copying their contents into Name.framework/Headers and Name.framework/PrivateHeaders and flattens them.

I just attached a text file with a find within the build directory when building through Xcode that Ross made for me which shows what goes where.

Currently the CMake function that makes this target is named WEBKIT_MAKE_FORWARDING_HEADERS. This is really a misnomer for what the function actually does which is an install of headers. WEBKIT_INSTALL_FRAMEWORK_HEADERS would probably be a better name. I'll start working through it and classifying headers like the above in https://bugs.webkit.org/show_bug.cgi?id=182593.

Having a listing of files is obviously a CMakeism. The macro Fujii created can just take directories which will glob and then create the proper targets. This obviously breaks down when a new file is added to an incremental build which is why CMake treats globbing as an anti-pattern.

Using whats there in generate-forwarding-headers.pl like what Konstantin suggested over in https://bugs.webkit.org/show_bug.cgi?id=182555#c17 wouldn't work because its not recursive, meaning it doesn't go looking inside the file it detects for more files. I have a script I'm using locally that is doing that which is why there are so many more files versus what actually is needed.

I am open to maybe piggy backing onto what Keith did with the unified source builds and making a Headers.txt or something along those lines but making things work first is my priority and cleanup is being saved for later. I did play with that some but decided to just use Fujii's work to get this task moving.
Comment 14 Michael Catanzaro 2018-02-08 08:09:22 PST
I don't want to start listing internal header files in CMakeLists.txt or Headers.txt or Sources.txt. But it's really starting to look like that might be what we have to do, isn't it....

(In reply to Fujii Hironori from comment #12)
> That's because WebKit is using #include <JavaScriptCore/VM.h> style instread
> of #include <JavaScriptCore/runtime/VM.h> style.
> We can remove all forwarding headers if we use the latter style.
> 
> Include header path is a part of API. This change is a API breakage.
> If Safari, for example, is including those header, it might be difficult to
> change.

Those headers are not part of the API. The API consists of headers that get installed. For JavaScriptCore, those are the headers under Source/JavaScriptCore/API. There should be no way for Safari to include VM.h, because it's not installed. Right? (Right?)

(In reply to Don Olmstead from comment #13)
> Remember that Xcode has this concept of public and private headers, the type
> of which is defined in Xcode. When you build a framework it installs those
> headers by copying their contents into Name.framework/Headers and
> Name.framework/PrivateHeaders and flattens them.

I think the real problem is that (a) the Xcode build is flattening the headers, which requires that we come up with some way to manually mimic this behavior, and (b) all the source code files in WebKit depend on flattened headers, so if we change it, we have to change the include style used in every source code file. We could write a script to do this for us, though.

Writing out the full path to the include directories, like Google does, seems best to me, e.g. #include "JavaScriptCore/runtime/VM.h". The alternative would be to add a bazillion include directories to every build target so that we can do #include "VM.h", but that seems far less desirable as then I lose important context (what sort of VM? oh, it's the JavaScriptCore VM).

That's my $0.02. I guess getting rid of flattened headers is maybe not likely, but that seems best to me.
Comment 15 Don Olmstead 2018-02-12 18:54:30 PST Comment hidden (obsolete)
Comment 16 Build Bot 2018-02-12 18:58:49 PST Comment hidden (obsolete)
Comment 17 Don Olmstead 2018-02-12 18:59:53 PST Comment hidden (obsolete)
Comment 18 Build Bot 2018-02-12 19:02:16 PST Comment hidden (obsolete)
Comment 19 Don Olmstead 2018-02-12 19:15:42 PST Comment hidden (obsolete)
Comment 20 Build Bot 2018-02-12 19:17:45 PST Comment hidden (obsolete)
Comment 21 Don Olmstead 2018-02-12 19:35:02 PST Comment hidden (obsolete)
Comment 22 Build Bot 2018-02-12 19:36:14 PST Comment hidden (obsolete)
Comment 23 Don Olmstead 2018-02-12 20:03:57 PST Comment hidden (obsolete)
Comment 24 Build Bot 2018-02-12 20:06:34 PST Comment hidden (obsolete)
Comment 25 Don Olmstead 2018-02-12 20:17:23 PST Comment hidden (obsolete)
Comment 26 Build Bot 2018-02-12 20:20:04 PST Comment hidden (obsolete)
Comment 27 Don Olmstead 2018-02-12 20:29:14 PST
I feel like the GTK/WPE bots are having issues with this patch...

Michael any chance you can try and apply this and see what happens locally? I really need to get a GTK or WPE environment up and running locally...
Comment 28 Zan Dobersek 2018-02-13 03:16:29 PST
Created attachment 333679 [details]
GTK and WPE changes

This complements attachment #333667 [details], fixing GTK+ and WPE builds.
Comment 29 Michael Catanzaro 2018-02-13 05:32:16 PST
Thank you Zan!
Comment 30 Don Olmstead 2018-02-13 10:12:27 PST Comment hidden (obsolete)
Comment 31 Build Bot 2018-02-13 10:13:36 PST Comment hidden (obsolete)
Comment 32 Don Olmstead 2018-02-13 10:27:36 PST Comment hidden (obsolete)
Comment 33 Build Bot 2018-02-13 10:29:52 PST Comment hidden (obsolete)
Comment 34 Don Olmstead 2018-02-13 10:49:22 PST Comment hidden (obsolete)
Comment 35 Don Olmstead 2018-02-13 11:19:07 PST Comment hidden (obsolete)
Comment 36 Build Bot 2018-02-13 11:22:11 PST Comment hidden (obsolete)
Comment 37 Build Bot 2018-02-13 12:52:30 PST Comment hidden (obsolete)
Comment 38 Build Bot 2018-02-13 12:52:32 PST Comment hidden (obsolete)
Comment 39 Don Olmstead 2018-02-13 12:56:34 PST Comment hidden (obsolete)
Comment 40 Keith Miller 2018-02-13 13:23:32 PST
Comment on attachment 333708 [details]
Patch

r=me, ideally, we wouldn't need to list all the forwarded headers but I can't think of another way specify the subset of headers that need to be exported.
Comment 41 Don Olmstead 2018-02-13 13:31:43 PST
Comment on attachment 333708 [details]
Patch

Clearing flags on attachment: 333708

Committed r228431: <https://trac.webkit.org/changeset/228431>
Comment 42 Don Olmstead 2018-02-13 13:31:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 43 Radar WebKit Bug Importer 2018-02-13 13:33:27 PST
<rdar://problem/37510435>
Comment 44 Fujii Hironori 2018-02-13 18:04:47 PST
Filed: Bug 182757 – REGRESSION(r228431) [CMake][Ninja] Fails to compile TestWebCore due to missing WebCore's derived headers
Comment 45 Fujii Hironori 2018-02-13 18:10:05 PST
Comment on attachment 333708 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:3295
> +    FILES ${WebCore_PRIVATE_FRAMEWORK_HEADERS}

Why is this variable named WebCore_PRIVATE_FRAMEWORK_HEADERS?
In my understanding, this is public framework headers because they are referred from external modules.
Comment 46 Don Olmstead 2018-02-13 18:12:38 PST
I'm told by perarne that the Apple Win EWS is having problems after this patch. I didn't think that a build change would add crashes especially since AppleWin already copied headers.

If it needs to be rolled out then we can do that. I'll keep looking into it tomorrow. If someone else wants to take a look its appreciated!
Comment 47 Don Olmstead 2018-02-13 18:13:38 PST
(In reply to Fujii Hironori from comment #45)
> Comment on attachment 333708 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333708&action=review
> 
> > Source/WebCore/CMakeLists.txt:3295
> > +    FILES ${WebCore_PRIVATE_FRAMEWORK_HEADERS}
> 
> Why is this variable named WebCore_PRIVATE_FRAMEWORK_HEADERS?
> In my understanding, this is public framework headers because they are
> referred from external modules.

If you look at https://bug-182512-attachments.webkit.org/attachment.cgi?id=333355 all the headers are listed as private within Apple's xcode setup so that's why.
Comment 48 Don Olmstead 2018-02-13 18:38:55 PST
(In reply to Don Olmstead from comment #46)
> I'm told by perarne that the Apple Win EWS is having problems after this
> patch. I didn't think that a build change would add crashes especially since
> AppleWin already copied headers.
> 
> If it needs to be rolled out then we can do that. I'll keep looking into it
> tomorrow. If someone else wants to take a look its appreciated!

Getting when running

Run-Time Check Failure #0 - The value of ESP was not properly saved across a function call.  This is usually a result of calling a function declared with one calling convention with a function pointer declared with a different calling convention.

I have a feeling that this might be due to config.h being removed in WebKitPrefix. I'm thinking of rolling this out then patching that separately to see what happens.
Comment 49 WebKit Commit Bot 2018-02-13 20:20:54 PST
Re-opened since this is blocked by bug 182766
Comment 50 Don Olmstead 2018-02-13 21:39:14 PST
This is currently blocked on https://bugs.webkit.org/show_bug.cgi?id=182757. Because of that I'll bust off some of these smaller changes that were required into separate bugs and land those.
Comment 51 Don Olmstead 2018-11-13 15:20:29 PST Comment hidden (obsolete)
Comment 52 Build Bot 2018-11-13 15:23:44 PST Comment hidden (obsolete)
Comment 53 Don Olmstead 2018-11-13 15:24:03 PST Comment hidden (obsolete)
Comment 54 Build Bot 2018-11-13 15:27:21 PST Comment hidden (obsolete)
Comment 55 Don Olmstead 2018-11-13 16:17:10 PST Comment hidden (obsolete)
Comment 56 Build Bot 2018-11-13 16:20:31 PST Comment hidden (obsolete)
Comment 57 Don Olmstead 2018-11-13 18:09:45 PST Comment hidden (obsolete)
Comment 58 Build Bot 2018-11-13 18:16:10 PST Comment hidden (obsolete)
Comment 59 Don Olmstead 2018-11-13 18:21:13 PST Comment hidden (obsolete)
Comment 60 Don Olmstead 2018-11-13 21:37:26 PST Comment hidden (obsolete)
Comment 61 Build Bot 2018-11-13 22:21:12 PST Comment hidden (obsolete)
Comment 62 Build Bot 2018-11-13 22:21:15 PST Comment hidden (obsolete)
Comment 63 Don Olmstead 2018-11-13 22:34:56 PST Comment hidden (obsolete)
Comment 64 Don Olmstead 2018-11-19 19:42:35 PST Comment hidden (obsolete)
Comment 65 Don Olmstead 2018-11-19 19:47:57 PST Comment hidden (obsolete)
Comment 66 Don Olmstead 2018-11-20 10:51:31 PST Comment hidden (obsolete)
Comment 67 Build Bot 2018-11-20 10:54:54 PST Comment hidden (obsolete)
Comment 68 Don Olmstead 2018-11-20 12:22:30 PST Comment hidden (obsolete)
Comment 69 Build Bot 2018-11-20 12:25:30 PST Comment hidden (obsolete)
Comment 70 Don Olmstead 2018-11-20 12:57:15 PST Comment hidden (obsolete)
Comment 71 Build Bot 2018-11-20 12:59:50 PST Comment hidden (obsolete)
Comment 72 Don Olmstead 2018-11-20 13:13:22 PST Comment hidden (obsolete)
Comment 73 Build Bot 2018-11-20 13:22:16 PST Comment hidden (obsolete)
Comment 74 Don Olmstead 2018-11-20 13:38:19 PST Comment hidden (obsolete)
Comment 75 Build Bot 2018-11-20 13:47:09 PST Comment hidden (obsolete)
Comment 76 Don Olmstead 2018-11-20 14:42:50 PST Comment hidden (obsolete)
Comment 77 Fujii Hironori 2018-11-20 14:49:52 PST
Comment on attachment 355361 [details]
Patch

Don’t land this until 
Bug 182757 is addressed.
Comment 78 Don Olmstead 2018-11-20 15:24:02 PST
(In reply to Fujii Hironori from comment #77)
> Comment on attachment 355361 [details]
> Patch
> 
> Don’t land this until 
> Bug 182757 is addressed.

I'm not able to reproduce this on WinCairo through Ninja. This is with ninja running 36 instances of clang-cl. 

I do think we need to enumerate all the DerivedSources that need to be copied to make sure there are no issues but the current implementation of WEBKIT_MAKE_FORWARDING_HEADERS doesn't handle that and it would be easier to change that once everything is copying throughout WebKit.
Comment 79 Don Olmstead 2019-04-15 17:31:03 PDT Comment hidden (obsolete)
Comment 80 Build Bot 2019-04-15 17:33:20 PDT Comment hidden (obsolete)
Comment 81 Don Olmstead 2019-04-15 17:38:27 PDT Comment hidden (obsolete)
Comment 82 Build Bot 2019-04-15 17:41:01 PDT Comment hidden (obsolete)
Comment 83 Don Olmstead 2019-04-15 18:03:11 PDT Comment hidden (obsolete)
Comment 84 Build Bot 2019-04-15 18:06:49 PDT Comment hidden (obsolete)
Comment 85 Don Olmstead 2019-04-15 18:28:21 PDT Comment hidden (obsolete)
Comment 86 Don Olmstead 2019-04-15 22:45:28 PDT Comment hidden (obsolete)
Comment 87 Don Olmstead 2019-04-15 22:58:32 PDT Comment hidden (obsolete)
Comment 88 Build Bot 2019-04-15 23:00:50 PDT Comment hidden (obsolete)
Comment 89 Don Olmstead 2019-04-15 23:03:38 PDT Comment hidden (obsolete)
Comment 90 Don Olmstead 2019-04-15 23:10:33 PDT Comment hidden (obsolete)
Comment 91 Don Olmstead 2019-04-15 23:18:18 PDT Comment hidden (obsolete)
Comment 92 Build Bot 2019-04-16 01:16:13 PDT Comment hidden (obsolete)
Comment 93 Build Bot 2019-04-16 01:16:16 PDT Comment hidden (obsolete)
Comment 94 Don Olmstead 2019-04-16 09:39:50 PDT Comment hidden (obsolete)
Comment 95 Build Bot 2019-04-16 09:42:42 PDT Comment hidden (obsolete)
Comment 96 Don Olmstead 2019-04-16 10:10:15 PDT
Created attachment 367543 [details]
Patch
Comment 97 Don Olmstead 2019-04-16 14:34:21 PDT
Created attachment 367570 [details]
Patch

Hopefully fixes the AppleWin build. GTK one passed in the last patch but took over 2 hours to complete. WPE bots seem to be in a bad state so I'm not sure if everything is ok with that one.

I do think this is ready for review though.
Comment 98 Don Olmstead 2019-04-16 14:53:37 PDT
Created attachment 367576 [details]
Patch

Fix bad include found by WPE bot.
Comment 99 Fujii Hironori 2019-04-16 19:23:16 PDT
Comment on attachment 367576 [details]
Patch

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

> Source/WebCore/PlatformAppleWin.cmake:58
> +list(APPEND WebCore_PRIVATE_FORWARDING_HEADERS

WebCore_PRIVATE_FORWARDING_HEADERS should be WebCore_PRIVATE_FRAMEWORK_HEADERS. This is the reason for the AppleWin EWS failure.

> Source/WebCore/PlatformAppleWin.cmake:73
> +    list(APPEND WebCore_PRIVATE_INCLUDE_DIRECTORIES

Ditto.
Comment 100 Fujii Hironori 2019-04-16 19:28:30 PDT
Comment on attachment 367576 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        will break the build whenever a file is added.

Yeah! explicitly listing is the right way to solve the repeating incremental build issue.

> Source/WebCore/ChangeLog:22
> +        * Headers.cmake: Added.

It would reduce the maintenance cost for header listing if you unify Headers.cmake and Sources.txt. How do you think this idea?
Comment 101 Don Olmstead 2019-04-16 19:31:03 PDT
Created attachment 367601 [details]
Patch

Hopefully make AppleWin happy
Comment 102 Don Olmstead 2019-04-16 19:35:18 PDT
(In reply to Fujii Hironori from comment #99)
> Comment on attachment 367576 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367576&action=review
> 
> > Source/WebCore/PlatformAppleWin.cmake:58
> > +list(APPEND WebCore_PRIVATE_FORWARDING_HEADERS
> 
> WebCore_PRIVATE_FORWARDING_HEADERS should be
> WebCore_PRIVATE_FRAMEWORK_HEADERS. This is the reason for the AppleWin EWS
> failure.
> 
> > Source/WebCore/PlatformAppleWin.cmake:73
> > +    list(APPEND WebCore_PRIVATE_INCLUDE_DIRECTORIES
> 
> Ditto.

Good catches!

(In reply to Fujii Hironori from comment #100)
> Comment on attachment 367576 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367576&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        will break the build whenever a file is added.
> 
> Yeah! explicitly listing is the right way to solve the repeating incremental
> build issue.
> 
> > Source/WebCore/ChangeLog:22
> > +        * Headers.cmake: Added.
> 
> It would reduce the maintenance cost for header listing if you unify
> Headers.cmake and Sources.txt. How do you think this idea?

I have some local patches that attempt to do something along this line. I was looking at this in terms of flattening PAL. Its super simple in CMake but there needs to be some kind of script for XCode.

I agree that this should be future work. I can gather up some of my patches and share where I was going with it.
Comment 103 Fujii Hironori 2019-04-16 19:40:45 PDT
(In reply to Don Olmstead from comment #102)
> I have some local patches that attempt to do something along this line. I
> was looking at this in terms of flattening PAL. Its super simple in CMake
> but there needs to be some kind of script for XCode.

You need to do nothing for XCode because generate-unified-source-bundles.rb bundles only .cpp and .mm.
It is harmless for XCode to add .h files in Sources.txt.

> I agree that this should be future work. I can gather up some of my patches
> and share where I was going with it.

Got it.
Comment 104 Fujii Hironori 2019-04-16 19:44:26 PDT
Comment on attachment 367601 [details]
Patch

Looks good to me. I'd like to wait EWS results and Moscow work time.
Comment 105 Alex Christensen 2019-04-17 09:01:35 PDT
Comment on attachment 367601 [details]
Patch

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

Looks good to me

> Source/WebCore/Headers.cmake:1211
> +    plugins/npfunctions.h

It looks like the AppleWin build can't find this file from DumpRenderTree.
Comment 106 Don Olmstead 2019-04-18 10:58:46 PDT Comment hidden (obsolete)
Comment 107 Don Olmstead 2019-04-18 10:59:22 PDT Comment hidden (obsolete)
Comment 108 Konstantin Tokarev 2019-04-18 11:07:13 PDT
Comment on attachment 367736 [details]
Patch

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

> Source/WebCore/Headers.cmake:1
> +set(WebCore_PRIVATE_FRAMEWORK_HEADERS

Manual maintenance of this header list sounds like a nightmare. Old macro at least supported directories
Comment 109 Don Olmstead 2019-04-18 11:14:52 PDT
Created attachment 367738 [details]
Patch

Fix a file that was renamed
Comment 110 Don Olmstead 2019-04-18 11:16:13 PDT
(In reply to Konstantin Tokarev from comment #108)
> Comment on attachment 367736 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367736&action=review
> 
> > Source/WebCore/Headers.cmake:1
> > +set(WebCore_PRIVATE_FRAMEWORK_HEADERS
> 
> Manual maintenance of this header list sounds like a nightmare. Old macro at
> least supported directories

Between when I used a script to enumerate the headers and posting this patch a file was renamed. This would force a rebuild because the directories would still have the old file. We know that CMake is bad about globbing so I don't really see any other options than enumerating things.
Comment 111 Don Olmstead 2019-04-18 12:19:59 PDT Comment hidden (obsolete)
Comment 112 Build Bot 2019-04-18 13:21:11 PDT Comment hidden (obsolete)
Comment 113 Don Olmstead 2019-04-18 14:25:35 PDT
Created attachment 367757 [details]
Patch

....
Comment 114 Build Bot 2019-04-18 14:31:48 PDT
Attachment 367757 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 3 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 115 WebKit Commit Bot 2019-04-18 16:20:33 PDT
Comment on attachment 367757 [details]
Patch

Clearing flags on attachment: 367757

Committed r244443: <https://trac.webkit.org/changeset/244443>
Comment 116 WebKit Commit Bot 2019-04-18 16:20:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 117 Don Olmstead 2019-04-18 16:39:35 PDT
Created attachment 367773 [details]
Build fix
Comment 118 Don Olmstead 2019-04-18 16:42:00 PDT
Reopening to land build fix
Comment 119 Don Olmstead 2019-04-18 16:42:39 PDT
Created attachment 367774 [details]
Build fix

A header got added between the time the patch was submitted to EWS and landing.
Comment 120 WebKit Commit Bot 2019-04-18 17:12:40 PDT
Comment on attachment 367774 [details]
Build fix

Clearing flags on attachment: 367774

Committed r244445: <https://trac.webkit.org/changeset/244445>
Comment 121 WebKit Commit Bot 2019-04-18 17:12:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 122 Don Olmstead 2019-04-18 17:26:53 PDT
Ok so looks like all the CMake based ports are green after the build fix.

Igalia folks if you have any problems after r244445 I would suggest doing a clean build. The aperez EWS bots were all having problems with this patch so hopefully those EWS bots can be looked at or maybe they'll end up nuking their WebKitBuild directory and get back to green.
Comment 123 Konstantin Tokarev 2019-04-22 14:34:19 PDT
(In reply to Don Olmstead from comment #110)
> (In reply to Konstantin Tokarev from comment #108)
> > Comment on attachment 367736 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=367736&action=review
> > 
> > > Source/WebCore/Headers.cmake:1
> > > +set(WebCore_PRIVATE_FRAMEWORK_HEADERS
> > 
> > Manual maintenance of this header list sounds like a nightmare. Old macro at
> > least supported directories
> 
> Between when I used a script to enumerate the headers and posting this patch
> a file was renamed. This would force a rebuild because the directories would
> still have the old file. We know that CMake is bad about globbing so I don't
> really see any other options than enumerating things.

What about the following approach: for each SomeFile.cpp of WebCore if SomeFile.h exists we add it to header list? Probably there will be a few remaining headers, but there should be much less manual work.

As compared to generate-forwarding-headers.pl approach, which scans whichever headers we want to include, this method will do some excessive work in the clean build, however it doesn't require re-scanning sources on each build, reducing incremental build time, and guarantees that no WebKit port will be broken because of someone forgetting to add new port-independent header to the list