Bug 183240 - [CMake][Win] Forwarding headers of WTF and PAL are copied twice in Visual Studio builds
Summary: [CMake][Win] Forwarding headers of WTF and PAL are copied twice in Visual Stu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-28 23:05 PST by Fujii Hironori
Modified: 2018-03-12 20:25 PDT (History)
5 users (show)

See Also:


Attachments
Screenshot of Solution Explore (120.34 KB, image/png)
2018-02-28 23:11 PST, Fujii Hironori
no flags Details
WIP patch for solution #3 (5.50 KB, patch)
2018-02-28 23:26 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
another WIP patch (3.18 KB, patch)
2018-03-01 00:58 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (7.30 KB, patch)
2018-03-05 11:36 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (7.11 KB, patch)
2018-03-05 14:19 PST, 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 Fujii Hironori 2018-02-28 23:05:54 PST
[CMake][Win] Forwarding headers of WTF and PAL are copied twice in Visual Studio builds

Here is the full build log: https://gist.github.com/fujii/84acffa6bd648c8af5bbac5b01ed954f
For example, DerivedSources/ForwardingHeaders/pal/system/Clock.h was copied twice in PALForwardingHeaders and PAL projects.

> 7>------ Build started: Project: PALForwardingHeaders, Configuration: Release x64 ------
> 7>Generating ../../../../DerivedSources/ForwardingHeaders/pal/system/Clock.h
> 23>------ Build started: Project: PAL, Configuration: Release x64 ------
> 23>Generating ../../../../DerivedSources/ForwardingHeaders/pal/system/Clock.h
Comment 1 Fujii Hironori 2018-02-28 23:11:17 PST
Created attachment 334797 [details]
Screenshot of Solution Explore

Those header files reside in both projects.
Comment 2 Fujii Hironori 2018-02-28 23:14:16 PST
I think we have two choices of solutions:

* Use cmake -E copy_if_different
* Copy in PAL target and Remove PALForwardingHeaders target
Comment 3 Fujii Hironori 2018-02-28 23:16:59 PST
One more way:

* Do not add those headers in WTF and PAL
Comment 4 Fujii Hironori 2018-02-28 23:26:36 PST
Created attachment 334798 [details]
WIP patch for solution #3
Comment 5 Fujii Hironori 2018-03-01 00:58:35 PST
Created attachment 334799 [details]
another WIP patch

(In reply to Fujii Hironori from comment #2)
> * Copy in PAL target and Remove PALForwardingHeaders target

It turns out this approach doesn't work as expected.
Because LLIntOffsetsExtractor needs forwarding header of JSC.
So JavaScriptCore and JavaScriptCoreForwardingHeaders targets should be separate.

I created another WIP patch of meta CMake approach.
Comment 6 Don Olmstead 2018-03-01 12:56:02 PST
(In reply to Fujii Hironori from comment #5)
> Created attachment 334799 [details]
> another WIP patch
> 
> (In reply to Fujii Hironori from comment #2)
> > * Copy in PAL target and Remove PALForwardingHeaders target
> 
> It turns out this approach doesn't work as expected.
> Because LLIntOffsetsExtractor needs forwarding header of JSC.
> So JavaScriptCore and JavaScriptCoreForwardingHeaders targets should be
> separate.
> 
> I created another WIP patch of meta CMake approach.

I'm trying to split the headers project into 2 in https://bugs.webkit.org/show_bug.cgi?id=183251 which would solve this issue.

WIP solution #3 is the route I would go after this but I would keep with the naming ${framework}_PUBLIC_FRAMEWORK_HEADERS
Comment 7 Don Olmstead 2018-03-05 11:36:32 PST
Created attachment 335009 [details]
Patch

Patch renaming WTF_HEADERS to WTF_PUBLIC_FRAMEWORK_HEADERS and PAL_HEADERS to PAL_PUBLIC_FRAMEWORK_HEADERS.

This stops the twice copying
Comment 8 Konstantin Tokarev 2018-03-05 14:04:05 PST
Comment on attachment 335009 [details]
Patch

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

> Source/WTF/wtf/CMakeLists.txt:1
> +set(WTF_PUBLIC_FRAMEWORK_HEADERS

WTF_PUBLIC_HEADERS would be better IMO
Comment 9 Don Olmstead 2018-03-05 14:19:16 PST
Created attachment 335024 [details]
Patch

Updating based on review comments.
Comment 10 Konstantin Tokarev 2018-03-05 15:55:12 PST
Where WTF_PUBLIC_HEADERS is used?
Comment 11 WebKit Commit Bot 2018-03-12 20:19:25 PDT
Comment on attachment 335024 [details]
Patch

Clearing flags on attachment: 335024

Committed r229572: <https://trac.webkit.org/changeset/229572>
Comment 12 WebKit Commit Bot 2018-03-12 20:19:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-03-12 20:25:45 PDT
<rdar://problem/38402775>