Bug 139438 - [CMake] Enable pre-compiled headers (PCH)
Summary: [CMake] Enable pre-compiled headers (PCH)
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords:
Depends on: 231905
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-09 02:43 PST by Carlos Alberto Lopez Perez
Modified: 2022-02-16 14:59 PST (History)
23 users (show)

See Also:


Attachments
WIP patch. Enables pre-compiled headers on WTF and JSC. Fails on WebCore (commented out) (156.65 KB, patch)
2014-12-09 03:13 PST, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (175.25 KB, patch)
2017-09-01 10:47 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Alternative take, reusing existing macro (10.97 KB, patch)
2017-09-01 20:30 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Simple approach, now with include paths and defines passed when building PCHs (14.42 KB, patch)
2017-09-02 07:15 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
WIP, fixes WebCoreDerivedSourcesPrefix; the style checker will still fail. (14.41 KB, patch)
2017-09-02 08:55 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (19.51 KB, patch)
2017-09-09 10:15 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (not for landing) (29.13 KB, patch)
2017-09-09 14:47 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
WIP Patch (16.06 KB, patch)
2021-10-18 02:28 PDT, Adrian Perez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch v2 (16.78 KB, patch)
2021-10-18 02:44 PDT, Adrian Perez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch v3 (16.91 KB, patch)
2021-10-18 04:44 PDT, Adrian Perez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch v4 (16.93 KB, patch)
2021-10-18 05:32 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
WIP Patch v5 (17.73 KB, patch)
2021-10-18 12:09 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
WIP Patch v6 (17.68 KB, patch)
2021-10-18 12:11 PDT, Adrian Perez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch v7 (18.23 KB, patch)
2021-10-18 13:17 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
WIP Patch v8 (18.23 KB, patch)
2021-10-18 15:34 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
WIP Patch v9 (17.58 KB, patch)
2021-10-19 12:27 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
WIP Patch v10 (32.48 KB, patch)
2021-10-19 14:04 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Windows fixes (28.63 KB, patch)
2021-10-20 17:00 PDT, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Windows and PlayStation fixes (29.57 KB, patch)
2021-10-21 13:28 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch v11 (6.91 KB, patch)
2021-10-25 15:48 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
WIP Patch v12 (31.01 KB, patch)
2021-10-25 15:54 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
WIP Patch v13 (32.18 KB, patch)
2021-11-03 14:57 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch v14 (32.15 KB, patch)
2021-12-20 11:32 PST, Adrian Perez
aperez: review?
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2014-12-09 02:43:45 PST
The usage of pre-compiled headers can reduce compilation times. It has brought to my attention the existence of the the following CMake module to enable them: https://github.com/sakra/cotire
Comment 1 Carlos Alberto Lopez Perez 2014-12-09 03:13:43 PST
Created attachment 242904 [details]
WIP patch. Enables pre-compiled headers on WTF and JSC. Fails on WebCore (commented out)

WIP: It enables pre-compiled headers on WTF and JSC. Fails on WebCore (its commented out). I didn't get performance measures yet, so I'm not sure if this is worth. Test it by building with --cmakeargs="-DENABLE_PRECOMPILED_HEADERS=true". Check the main ChangeLog in the patch for more details.
Comment 2 Adrian Perez 2017-09-01 10:47:28 PDT
Created attachment 319615 [details]
Patch

Here goes another attempt at this, with a new patch which works well for both
JavaScriptCore and WebCore. The approach followed is to reuse the existing
prefix headers, adding system includes to them for GTK/WPE, and using them as
input for creation of the precompiled header. This way:

 - Only headers which are known to not change often get precompiled (that is:
   system headers, and those from the internal JHBuild for development builds).

 - We avoid breaking the build (super important!)

With the patch applied, with Clang, without CCache/IceCC, and using
“webkit-build --release --gtk --makeargs=WebCore”, build times are:

 - ~39 minutes WITH precompiled headers.
 - ~44 minutes WITHOUT precompiled headers.

This is certainly promising, and I already have some changes locally to also
enable precompiled headers for other targets (mainly WebKit, also WebProcess,
NetworkProcess, etc).

My main (and only) doubt is: Is it okay to reuse the existing prefix headers
for this?

(BTW, the builds I made here work fine, MiniBrowser works, and I also tried
running some layout tests (which worked), but I didn't wait for a full run to
finish.)
Comment 3 Adrian Perez 2017-09-01 10:52:48 PDT
One comment: to use precompiled headers *and* CCache at the same
time, one needs to:

  export CCACHE_SLOPPINESS=pch_defines,time_macros

Cotire will emit a warning when CMake runs if it doesn't find the
environment variable set.
Comment 4 Konstantin Tokarev 2017-09-01 11:32:02 PDT
I think we should use https://github.com/larsch/cmake-precompiled-header for PCH and static *AllInOne.cpp files for unity builds.
Comment 5 Michael Catanzaro 2017-09-01 11:32:19 PDT
Comment on attachment 319615 [details]
Patch

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

Awesome! I did not realize precompiled headers existed on Linux.

The necessity for the CCACHE_SLOPPINESS environment variable is the main blocker here. It has to work without needing any environment variables. ccache gets pulled in with the @developer-tools group on Fedora so it's expected to be present, but developers should not be expected to know what it is.

> Source/JavaScriptCore/CMakeLists.txt:1607
> +if (COMMAND cotire)

How does this conditional work? It's testing for what exactly?

> Source/JavaScriptCore/CMakeLists.txt:1611
> +        COTIRE_PREFIX_HEADER_INCLUDE_PATH "${CMAKE_SOURCE_DIR}/WebKitBuild/DependenciesGTK/Root;${CMAKE_SOURCE_DIR}/WebKitBuild/DependenciesWPE/Root"

r- also for the hardcoded paths. This is at least easy to fix; you can define some CMake variable differently in OptionsGTK.cmake and OptionsWPE.cmake and reuse it here.

> Source/WebCore/CMakeLists.txt:4103
> +        COTIRE_PREFIX_HEADER_INCLUDE_PATH "${CMAKE_SOURCE_DIR}/WebKitBuild/DependenciesGTK/Root;${CMAKE_SOURCE_DIR}/WebKitBuild/DependenciesWPE/Root"

Ditto.

> Source/WebCore/WebCorePrefix.h:129
> +#elif PLATFORM(GTK) || PLATFORM(WPE)

WebCorePrefix.h seems like the perfect location for this. I'm not sure why you think it might not be OK.
Comment 6 Carlos Alberto Lopez Perez 2017-09-01 11:47:15 PDT
(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 319615 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319615&action=review
> 
> Awesome! I did not realize precompiled headers existed on Linux.
> 
> The necessity for the CCACHE_SLOPPINESS environment variable is the main
> blocker here. It has to work without needing any environment variables.
> ccache gets pulled in with the @developer-tools group on Fedora so it's
> expected to be present, but developers should not be expected to know what
> it is.
> 

I strongly disagree with this being a blocker. Really.

Also, note that installing ccache doesn't mean you are using it.

To actually use ccache you have to change your default PATH.

$ which gcc
/usr/lib/ccache/gcc

And if you can change your PATH you can also change an environment variable.
Comment 7 Carlos Alberto Lopez Perez 2017-09-01 11:54:50 PDT
(In reply to Carlos Alberto Lopez Perez from comment #6)
> (In reply to Michael Catanzaro from comment #5)
> > Comment on attachment 319615 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=319615&action=review
> > 
> > Awesome! I did not realize precompiled headers existed on Linux.
> > 
> > The necessity for the CCACHE_SLOPPINESS environment variable is the main
> > blocker here. It has to work without needing any environment variables.
> > ccache gets pulled in with the @developer-tools group on Fedora so it's
> > expected to be present, but developers should not be expected to know what
> > it is.
> > 
> 
> I strongly disagree with this being a blocker. Really.
> 
> Also, note that installing ccache doesn't mean you are using it.
> 
> To actually use ccache you have to change your default PATH.
> 
> $ which gcc
> /usr/lib/ccache/gcc
> 
> And if you can change your PATH you can also change an environment variable.

In any case the script build-webkit maybe can set this environment variable before executing CMake. That way everybody gets the right env var if they are using CMake.
Comment 8 Adrian Perez 2017-09-01 13:03:43 PDT
(In reply to Carlos Alberto Lopez Perez from comment #6)
> (In reply to Michael Catanzaro from comment #5)
> > Comment on attachment 319615 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=319615&action=review
> > 
> > Awesome! I did not realize precompiled headers existed on Linux.
> > 
> > The necessity for the CCACHE_SLOPPINESS environment variable is the main
> > blocker here. It has to work without needing any environment variables.
> > ccache gets pulled in with the @developer-tools group on Fedora so it's
> > expected to be present, but developers should not be expected to know what
> > it is.
> > 
> 
> I strongly disagree with this being a blocker. Really.

Totally not a blocker. If you use CCache and do not set the environment
variable, compilation completes successfully anyway — CCache will just
have cache misses, as explained in the manual: 

   https://ccache.samba.org/manual.html#_precompiled_headers

> Also, note that installing ccache doesn't mean you are using it.
> 
> To actually use ccache you have to change your default PATH.
> 
> $ which gcc
> /usr/lib/ccache/gcc
> 
> And if you can change your PATH you can also change an environment variable.

Yep. CCache usage is optional, and it the user making the build is
responsible to opt-in to use it. The same goes for IceCC. It has always
been like that.
Comment 9 Adrian Perez 2017-09-01 13:10:08 PDT
(In reply to Konstantin Tokarev from comment #4)
> I think we should use https://github.com/larsch/cmake-precompiled-header for
> PCH and static *AllInOne.cpp files for unity builds.

Sorry, but cmake-precompiled-header does not support Clang — Cotire does.
That being said, if there is still a strong preference against Cotire, I can
try and modify cmake-precompiled-header to add Clang support.

At any rate, I think not supporting Clang for precompiled headers would be
bad, provided that some developers prefer it due to the (sometimes) better
error diagnostics and slightly faster build times. Also, some GNU/Linux
distributions which have old versions of GCC as the system compiler are
using Clang to build WebKitGTK+ and it would be nice that they could also
get faster builds by allowing usage of precompiled headers with Clang.
Comment 10 Adrian Perez 2017-09-01 13:14:49 PDT
(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 319615 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319615&action=review
> 
> Awesome! I did not realize precompiled headers existed on Linux.
> 
> The necessity for the CCACHE_SLOPPINESS environment variable is the main
> blocker here. It has to work without needing any environment variables.
> ccache gets pulled in with the @developer-tools group on Fedora so it's
> expected to be present, but developers should not be expected to know what
> it is.
> 
> > Source/JavaScriptCore/CMakeLists.txt:1607
> > +if (COMMAND cotire)
> 
> How does this conditional work? It's testing for what exactly?

This checks whether a CMake function named “cotire” is available. When
Cotire is not loaded, this evaluates to “false” (note that CMake is
confusing: it uses the term “command” for functions that can be used
in CMake files).

> > Source/JavaScriptCore/CMakeLists.txt:1611
> > +        COTIRE_PREFIX_HEADER_INCLUDE_PATH "${CMAKE_SOURCE_DIR}/WebKitBuild/DependenciesGTK/Root;${CMAKE_SOURCE_DIR}/WebKitBuild/DependenciesWPE/Root"
> 
> r- also for the hardcoded paths. This is at least easy to fix; you can
> define some CMake variable differently in OptionsGTK.cmake and
> OptionsWPE.cmake and reuse it here.

Un-hardcoding these is a good suggestion, thanks!

> > Source/WebCore/WebCorePrefix.h:129
> > +#elif PLATFORM(GTK) || PLATFORM(WPE)
> 
> WebCorePrefix.h seems like the perfect location for this. I'm not sure why
> you think it might not be OK.

I also think that the existing prefix headers look like the perfect
place for this, but as I had never touched them before and the GTK+/WPE
ports didn't use them before, I was not 100% sure :-)
Comment 11 Adrian Perez 2017-09-01 13:25:44 PDT
So I am leaving a fresh build overnight in my laptop with the PCH
support added for the WebKit+WebProcess+NetworkProcess targets, and
if all goes well (as I expect!) tomorrow I'll post an updated patch
and new figures for the build times.
Comment 12 Michael Catanzaro 2017-09-01 13:28:08 PDT
(In reply to Carlos Alberto Lopez Perez from comment #6)
> To actually use ccache you have to change your default PATH.
> 
> $ which gcc
> /usr/lib/ccache/gcc
> 
> And if you can change your PATH you can also change an environment variable.

The assumption that the user has intentionally enabled ccache, or even knows what ccache is, is not correct. For instance, on my machine:

$ which gcc
/usr/lib64/ccache/gcc

I didn't add that to my path, or do anything at all to configure ccache. Fedora has /etc/profile.d/ccache.sh to ensure it gets enabled without you having to know that ccache exists. It's not installed by default, but I did not intentionally install it either. At some point I ran the command 'dnf install @c-development' which pulled it in. Safe to assume many, many developers will have done that.

(In reply to Carlos Alberto Lopez Perez from comment #7)
> In any case the script build-webkit maybe can set this environment variable
> before executing CMake. That way everybody gets the right env var if they
> are using CMake.

That's not good enough. We need to set it from the CMake build system. Fortunately that should be very easy. It can be done with something like set(ENV{CCACHE_SLOPPINESS} ...). Let's just add that somewhere and move on.

(In reply to Adrian Perez from comment #8)
> Totally not a blocker. If you use CCache and do not set the environment
> variable, compilation completes successfully anyway — CCache will just
> have cache misses, as explained in the manual: 
> 
>    https://ccache.samba.org/manual.html#_precompiled_headers

You said there would be a warning, and we don't allow build warnings. Anyway, we're lucky that it's easy enough to avoid.
Comment 13 Carlos Alberto Lopez Perez 2017-09-01 13:46:05 PDT
(In reply to Konstantin Tokarev from comment #4)
> I think we should use https://github.com/larsch/cmake-precompiled-header for
> PCH and static *AllInOne.cpp files for unity builds.

Why is that better than Cotire?
Comment 14 Adrian Perez 2017-09-01 13:49:48 PDT
(In reply to Michael Catanzaro from comment #12)

> [...]
>
> You said there would be a warning, and we don't allow build warnings.
> Anyway, we're lucky that it's easy enough to avoid.

It's not a warning while compilation is in progress. CMake prints a
message during configuration telling the user (that is: *warning* the
user) that the environment variable should be defined later on while
building.
Comment 15 Michael Catanzaro 2017-09-01 14:02:12 PDT
Ah, OK then. I think that's OK. It's not so bad.

I guess it's not so easy to get the environment variable set after all. :(
Comment 16 Adrian Perez 2017-09-01 20:30:03 PDT
Created attachment 319687 [details]
Alternative take, reusing existing macro

It turns out, the WebKit CMake files already had support
for doing precompiled headers with MSVC, so the best solution
in the end seemed to be to just hook there the missing CMake
spell to have it working for GCC and Clang. I reached this
solution after a long night trying to understand Cotire and
the suggestion by Konstantin, cmake-precompiled-header. Right
now I left my laptop doing a build, so no timings yet until
I get some hours of sleep -- hopefully the build will complete.

FWIW, personally I prefer this much simpler approach :-D

P.S: The patch will not pass the style checker, it is quite WIP
but I wanted to share it anyway so you can all give feedback.
Comment 17 Adrian Perez 2017-09-01 20:30:52 PDT
(In reply to Adrian Perez from comment #16)

> [...]
> 
> P.S: The patch will not pass the style checker, it is quite WIP
> but I wanted to share it anyway so you can all give feedback.

Also the ChangeLog is not properly formatted O:-)
Comment 18 Build Bot 2017-09-01 20:31:41 PDT
Attachment 319687 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:1:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Adrian Perez 2017-09-02 04:08:25 PDT
Well, the build didn't finish (see log below), but the method from the
simple patch mostly works: the PCH for JavaScriptCore got generated
and used, but then for WebCore it failed because there must be some
“-I...” flag missing when invoking the compiler to build the PCH.

Maybe I'll have some time later today to figure out how to pull the
missing compiler flags.

(BTW, the result is the same with Clang, so the switcheroo for chosing
the .gch/.pch suffix depending on the compiler is working well, too!)

--- Build Output ---

[168/6006] Generating ../../DerivedSources/JavaScriptCore/LLIntDesiredOffsets.h
Only dealing with backends: ["X86_64"]
[1527/6006] Compiling PCH /home/aperez/devel/WebKit/WebKitBuild/Release/Source/WebCore/WebCoreDerivedSourcesPrefix.h.gch
FAILED: Source/WebCore/WebCoreDerivedSourcesPrefix.h.gch
cd /home/aperez/devel/WebKit/WebKitBuild/Release/Source/WebCore && /usr/bin/g++ -x c++-header -o /home/aperez/devel/WebKit/WebKitBuild/Release/Source/WebCore/WebCoreDerivedSourcesPrefix.h.gch /home/aperez/devel/WebKit/Source/WebCore/WebCoreDerivedSourcesPrefix.cpp
In file included from /home/aperez/devel/WebKit/Source/WebCore/WebCoreDerivedSourcesPrefix.cpp:26:0:
/home/aperez/devel/WebKit/Source/WebCore/WebCorePrefix.h:34:10: fatal error: wtf/Platform.h: No such file or directory
 #include <wtf/Platform.h>
          ^~~~~~~~~~~~~~~~
compilation terminated.
Comment 20 Adrian Perez 2017-09-02 07:15:03 PDT
Created attachment 319715 [details]
Simple approach, now with include paths and defines passed when building PCHs


Here goes a new version of the simpler approach, which is not so
simple anymore, but it does not properly add the compiler defines
and include paths when compiling the prefix headers. In order to
get the lists of flags an extra parameter is passed to the macro,
which is the name of the target from which to pick the compiler
flags. The flags are written to a response file because that way
it is done as the last step after all the compilation setup has
been completed -- otherwise not all the flags get picked, in case
the WEBKIT_ADD_PRECOMPILED_HEADER is called *before* additional
options are set, which can happen in port-specific CMake snippets.

This version properly builds the PCHs, and I am waiting for the
builds to finish to be able to provide timings and validate that
WebKit is built correctly.

:-D
Comment 21 Build Bot 2017-09-02 07:16:42 PDT
Attachment 319715 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:1:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Adrian Perez 2017-09-02 08:55:56 PDT
Created attachment 319716 [details]
WIP, fixes WebCoreDerivedSourcesPrefix; the style checker will still fail.
Comment 23 Build Bot 2017-09-02 08:57:48 PDT
Attachment 319716 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:1:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Michael Catanzaro 2017-09-02 10:31:09 PDT
Comment on attachment 319716 [details]
WIP, fixes WebCoreDerivedSourcesPrefix; the style checker will still fail.

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

> Source/WebKit/WebKit2Prefix.h:75
> +#if USE(LIBEPOXY)
> +#include <epoxy.h>
> +#endif

We need to remove this from WebKit2Prefix.h because it's not in the include path. Seems this layer does not use libepoxy. See the build failure on the WPE bot.

> Source/cmake/WebKitMacros.cmake:99
> +            add_custom_command(
> +                OUTPUT "${PrecompiledBinary}"
> +                MAIN_DEPENDENCY "${_cpp}"
> +                DEPENDS "${PrecompiledBinary}.flags"
> +                COMMENT "Compiling PCH ${PrecompiledBinary}"
> +                COMMAND "${CMAKE_CXX_COMPILER}" "@${PrecompiledBinary}.flags"
> +                    -x c++-header ${_pch_cxx_flags} -o "${PrecompiledBinary}"
> +                    "${CMAKE_CURRENT_SOURCE_DIR}/${_cpp}")

So we did not need cotire after all? The bug title and ChangeLog entries need to be updated with the newer name.

> ChangeLog:16
> +2017-09-01  Adrian Perez de Castro  <aperez@igalia.com>
> +
> +        [CMake] Enable pre-compiled headers (PCH) (cotire)
> +        https://bugs.webkit.org/show_bug.cgi?id=139438
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Source/cmake/WebKitMacros.cmake:
> +
> +2017-09-01  Adrian Perez de Castro  <aperez@igalia.com>
> +
> +        [CMake] Enable pre-compiled headers (PCH) (cotire)
> +        https://bugs.webkit.org/show_bug.cgi?id=139438
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Duplicate ChangeLog entry.
Comment 25 Adrian Perez 2017-09-02 11:14:29 PDT
(In reply to Michael Catanzaro from comment #24)
> Comment on attachment 319716 [details]
> WIP, fixes WebCoreDerivedSourcesPrefix; the style checker will still fail.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319716&action=review
> 
> > Source/WebKit/WebKit2Prefix.h:75
> > +#if USE(LIBEPOXY)
> > +#include <epoxy.h>
> > +#endif
> 
> We need to remove this from WebKit2Prefix.h because it's not in the include
> path. Seems this layer does not use libepoxy. See the build failure on the
> WPE bot.

Good catch. I have not tried building WPE yet, so this was an speculative
addition to the precompiled header. Once we have the initial support landed
we can look at what gets often included and try to add more #includes in
the prefix headers to further improve build times — IMHO that would be fine
as a follow-up patch :-)

> > Source/cmake/WebKitMacros.cmake:99
> > +            add_custom_command(
> > +                OUTPUT "${PrecompiledBinary}"
> > +                MAIN_DEPENDENCY "${_cpp}"
> > +                DEPENDS "${PrecompiledBinary}.flags"
> > +                COMMENT "Compiling PCH ${PrecompiledBinary}"
> > +                COMMAND "${CMAKE_CXX_COMPILER}" "@${PrecompiledBinary}.flags"
> > +                    -x c++-header ${_pch_cxx_flags} -o "${PrecompiledBinary}"
> > +                    "${CMAKE_CURRENT_SOURCE_DIR}/${_cpp}")
> 
> So we did not need cotire after all? The bug title and ChangeLog entries
> need to be updated with the newer name.

Noup, the simple approach works fine, thanks to the WebKit source code
being well organized and the usage we make of CMake being consistent.

Cotire does *a lot* of things because it tries to work in all possible
situations (old CMake versions, mixed C/C++ projects, compilers other
than GCC/Clang/MSVC, etc), and also it is made in such a way that it
queries *existing, already configured* CMake targets to allow adding
PCH+unitybuild support for any existing project, which is *very* hard
(I looked at the code, it's scaaary!); therefore it has a lot of code
needed to support all those scenarios.

> > ChangeLog:16
> > +2017-09-01  Adrian Perez de Castro  <aperez@igalia.com>
> > +
> > +        [CMake] Enable pre-compiled headers (PCH) (cotire)
> > +        https://bugs.webkit.org/show_bug.cgi?id=139438
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * Source/cmake/WebKitMacros.cmake:
> > +
> > +2017-09-01  Adrian Perez de Castro  <aperez@igalia.com>
> > +
> > +        [CMake] Enable pre-compiled headers (PCH) (cotire)
> > +        https://bugs.webkit.org/show_bug.cgi?id=139438
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> 
> Duplicate ChangeLog entry.

Yeah, I have been updating the attached patch to this bug often, to
let people see how it is coming along. It will need at least one last
cleanup pass (I have a couple of ideas to simplify macro invocations)
and fixing the ChangeLogs and other style violations.

At any rate, I will be updating with updates along the weekend — I am
determined to have PCH support working ASAP, because it will benefit
infrastructure (faster buildbots), developer experience (shorter
edit-compile-test cycle), and distributors (shorter build times to
update packages).
Comment 26 Michael Catanzaro 2017-09-05 18:05:12 PDT
Comment on attachment 319716 [details]
WIP, fixes WebCoreDerivedSourcesPrefix; the style checker will still fail.

I don't see anything major wrong with this, but you have some comments to address so let's get this out of the request queue for now.
Comment 27 Adrian Perez 2017-09-09 10:15:38 PDT
Created attachment 320349 [details]
Patch

Updated version. Now this picks all the relevant
build flags which affect generation of the PCHs. After much back and forth
reading the CMake documentation, and checking what Cotire does (it turns
out it does many things because it *does* need to do them) I came up with
a not-so-big as Cotire yet not-as-simple as the solution pointed out by
Konstantin (which didn not work becayse it was missing flags).

Still pending to do actual full builds, with this I am getting consistently
between 1 and 2 minutes of saved build time when building only JSC. Also
gotta add the ChageLog entries and all -- but again, sharing early for
feedback. If somebody could run with this patch on MSVC to make sure I
did not break anything, that would be swell.
Comment 28 Build Bot 2017-09-09 10:17:26 PDT
Attachment 320349 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Adrian Perez 2017-09-09 14:26:48 PDT
(In reply to Adrian Perez from comment #27)

> [...] If somebody could run with this patch on MSVC to make sure I
> did not break anything, that would be swell.

...aaand of course the somebody is the Windows EWS. I'll fix it, and
also submit a version of the patch that enables PCHs by default, so
all the EWS run through it enabled.
Comment 30 Adrian Perez 2017-09-09 14:47:00 PDT
Created attachment 320359 [details]
Patch (not for landing)


This is an updated version with PCHs always enabled in order to
test things in all the EWS bots. If this works I will re-upload with the
check for ENABLE_PRECOMPILED_HEADERS in.
Comment 31 Build Bot 2017-09-09 14:49:59 PDT
Attachment 320359 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 Carlos Alberto Lopez Perez 2017-09-14 06:10:18 PDT
Comment on attachment 320359 [details]
Patch (not for landing)

The patch looks good.
I'm willing to do some benchmarks with it once it works ok.
For now, setting r- due to red EWS.
I tried it also locally and failed to build with:
FAILED: Source/WebCore/WebCore_DERIVED_SOURCES/pch.h.gch 
cd /home/clopez/webkit/webkit/WebKitBuild/Release/Source/WebCore && /usr/bin/c++ @/home/clopez/webkit/webkit/WebKitBuild/Release/Source/WebCore/WebCore_DERIVED_SOURCES/flags -x c++-header -o /home/clopez/webkit/webkit/WebKitBuild/Release/Source/WebCore/WebCore_DERIVED_SOURCES/pch.h.gch /home/clopez/webkit/webkit/WebKitBuild/Release/Source/WebCore/WebCore_DERIVED_SOURCES/pch.h
In file included from /usr/include/c++/6/bits/stl_algo.h:59:0,
                 from /usr/include/c++/6/algorithm:62,
                 from /home/clopez/webkit/webkit/Source/WebCore/WebCorePrefix.h:81,
                 from /home/clopez/webkit/webkit/WebKitBuild/Release/Source/WebCore/WebCore_DERIVED_SOURCES/pch.h:1:
/usr/include/c++/6/cstdlib:75:25: fatal error: stdlib.h: No such file or directory
 #include_next <stdlib.h>
                         ^
Which reminds me to bug 161697 (maybe related, take a look)
Comment 33 Adrian Perez 2020-02-21 07:08:09 PST
We probably want to use the built-in support for precompiled
headers which will be available starting with CMake 3.16:

  https://onqtam.com/programming/2019-12-20-pch-unity-cmake-3-16/

Maybe it would be possible to have a macro which actually
calls CMake's target_precompile_headers() when using CMake 3.16
or newer, and which is a no-op otherwise (for older versions).
Comment 34 Konstantin Tokarev 2020-02-22 16:36:16 PST
We could also add cmake to jhbuild so it would be easier for developers to get this feature
Comment 35 Konstantin Tokarev 2020-03-17 03:45:33 PDT
I did some experiments with this feature and I'd like to warn you that 3.16 is buggy, please use 3.17 at least
Comment 36 Adrian Perez 2021-10-18 02:28:09 PDT
Created attachment 441574 [details]
WIP Patch

New version using the CMake builtin PCH support
Comment 37 Adrian Perez 2021-10-18 02:44:42 PDT
Created attachment 441576 [details]
WIP Patch v2
Comment 38 Adrian Perez 2021-10-18 04:44:17 PDT
Created attachment 441585 [details]
WIP Patch v3
Comment 39 Adrian Perez 2021-10-18 05:32:48 PDT
Created attachment 441593 [details]
WIP Patch v4
Comment 40 Adrian Perez 2021-10-18 12:09:20 PDT
Created attachment 441627 [details]
WIP Patch v5
Comment 41 Adrian Perez 2021-10-18 12:11:25 PDT
Created attachment 441628 [details]
WIP Patch v6
Comment 42 Adrian Perez 2021-10-18 13:17:12 PDT
Created attachment 441636 [details]
WIP Patch v7
Comment 43 Adrian Perez 2021-10-18 15:34:20 PDT
Created attachment 441649 [details]
WIP Patch v8
Comment 44 Adrian Perez 2021-10-19 12:27:34 PDT
Created attachment 441769 [details]
WIP Patch v9
Comment 45 Don Olmstead 2021-10-19 13:04:59 PDT
Comment on attachment 441769 [details]
WIP Patch v9

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

Looks like you're super close.

> Source/WebKitLegacy/CMakeLists.txt:50
> +WEBKIT_ADD_PRECOMPILED_HEADER(WebKitLegacy WebKitPrefix.h)

This should be done in the PlatformWin.cmake and the header is win/WebkitPrefix.h For PlatformMac it would be mac/WebKitPrefix.h

> Tools/WebKitTestRunner/PlatformFTW.cmake:-32
> -WEBKIT_ADD_PRECOMPILED_HEADER("WebKitTestRunnerPrefix.h" "win/WebKitTestRunnerPrefix.cpp" WebKitTestRunner_SOURCES)

This again looks like you're going to hit an issue due to the location of the prefix header.
Comment 46 Adrian Perez 2021-10-19 14:02:11 PDT
(In reply to Don Olmstead from comment #45)
> Comment on attachment 441769 [details]
> WIP Patch v9
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441769&action=review
> 
> Looks like you're super close.
> 
> > Source/WebKitLegacy/CMakeLists.txt:50
> > +WEBKIT_ADD_PRECOMPILED_HEADER(WebKitLegacy WebKitPrefix.h)
> 
> This should be done in the PlatformWin.cmake and the header is
> win/WebkitPrefix.h For PlatformMac it would be mac/WebKitPrefix.h
> 
> > Tools/WebKitTestRunner/PlatformFTW.cmake:-32
> > -WEBKIT_ADD_PRECOMPILED_HEADER("WebKitTestRunnerPrefix.h" "win/WebKitTestRunnerPrefix.cpp" WebKitTestRunner_SOURCES)
> 
> This again looks like you're going to hit an issue due to the location of
> the prefix header.

This souldn't be an issue, the header is indeed located at 
“Tools/WebKitTestRunner/WebKitTestRunnerPrefix.h” and with the
CMake PCH support the .cpp file is not really needed.

(Actually, next version of the patch will remove the unneeded
files as well.)
Comment 47 Adrian Perez 2021-10-19 14:04:01 PDT
Created attachment 441792 [details]
WIP Patch v10
Comment 48 Don Olmstead 2021-10-19 14:56:47 PDT
```
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\graphics\cg\ImageBufferCGBitmapBackend.cpp(146,36): error C2672: 'WTF::adoptCF': no matching overloaded function found [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
```

This AppleWin failure is most likely due to WebCorePrefixHeader.h including CoreFoundation files so its a legit failure.
Comment 49 Don Olmstead 2021-10-20 17:00:53 PDT
Created attachment 441957 [details]
Windows fixes

The DumpRenderTree and WebKitTestRunner bits on Windows use WEBKIT_EXECUTABLE_WRAP which creates a DLL that has all the code that the executable then runs. Apply to that wrapper library to prevent the build error seen on the bots.
Comment 50 Don Olmstead 2021-10-20 17:50:18 PDT
Windows is happy. Not sure why WPE decided to stop building.

Anyways I'll leave the rest to you Adrian.
Comment 51 Adrian Perez 2021-10-21 06:17:14 PDT
(In reply to Don Olmstead from comment #50)
> Windows is happy. Not sure why WPE decided to stop building.
> 
> Anyways I'll leave the rest to you Adrian.

Great, thanks for your support! I will integrate your fixes, check
why the WPE builder is choking now on the patch, and make sure this
gets landed in the next days \o/
Comment 52 Don Olmstead 2021-10-21 13:28:05 PDT
Created attachment 442060 [details]
Windows and PlayStation fixes
Comment 53 Don Olmstead 2021-10-21 13:31:12 PDT
Comment on attachment 442060 [details]
Windows and PlayStation fixes

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

Additional changes were required for a local PlayStation build so please use this as your base.

> Source/WebCore/testing/js/WebCoreTestSupportPrefix.h:-48
> -
> -#define WEBCORE_TESTSUPPORT_EXPORT WEBCORE_EXPORT
> -

This should be properly handled in PlatformExportMacros.h 🤞

> Source/WebCore/testing/js/WebCoreTestSupportPrefix.h:130
> +#elif defined(__APPLE__)
> +
>  #if !PLATFORM(IOS_FAMILY)
>  #include <CoreServices/CoreServices.h>
>  #endif // !PLATFORM(IOS_FAMILY)

PlayStation was ending up trying to include CoreServices.h so changed this into one big #if #elif setup to prevent that.
Comment 54 Adrian Perez 2021-10-22 02:54:48 PDT
I left a script running overnight undisturbed in an old laptop, which
did three builds without this patch, and three builds after applying it.
The builds were done as of r284652 with the Flatpak SDK using the
following command:

  WK_USE_CCACHE=NO ./Tools/Scripts/build-webkit --gtk --no-experimental-features

The averaged build times were:

   Before: 2h:22m:07s
    After: 2h:12m:54s

While not much faster, it's still a ~7% speedup, and we get to simplify
and unify the handling of precompiled headers. We can probably tweak the
prefix headers to make a few more big #includes to further improve things,
but I would rather do that in a follow-up =)
Comment 55 Adrian Perez 2021-10-22 05:34:24 PDT
Well, I don't know why the linker is being passed the precompiled
header or why that's an issue, but at least now I can reproduce it
locally: doing a build with the patch applied on top of a dirty
build directory using the Flatpak SDK hits it. Investigating…
Comment 56 Adrian Perez 2021-10-25 15:48:28 PDT
Created attachment 442431 [details]
WIP Patch v11
Comment 57 Adrian Perez 2021-10-25 15:54:14 PDT
Created attachment 442432 [details]
WIP Patch v12
Comment 58 Adrian Perez 2021-10-29 04:48:08 PDT
I have now a version of the patch that also makes the WPE port build
correctly. Just one pass more to clean up a couple of things and I will
post the finished patch to land :-)
Comment 59 Adrian Perez 2021-10-29 05:39:26 PDT
(In reply to Adrian Perez from comment #58)
> I have now a version of the patch that also makes the WPE port build
> correctly. Just one pass more to clean up a couple of things and I will
> post the finished patch to land :-)

Wait! The plot thickens:

  % cmake --version | head -1
  cmake version 3.21.4
  % cmake --version | head -1
  cmake version 3.21.2
  %

System CMake (the newer one) works, but the one from the Flatpak SDK still
doesn't. Checking the diff between both versions I cannot find anything
obvious that would affect how target_precompile_headers() works.
Comment 60 Adrian Perez 2021-10-29 05:41:02 PDT
(In reply to Adrian Perez from comment #59)
> (In reply to Adrian Perez from comment #58)
> > I have now a version of the patch that also makes the WPE port build
> > correctly. Just one pass more to clean up a couple of things and I will
> > post the finished patch to land :-)
> 
> Wait! The plot thickens:
> 
>   % cmake --version | head -1
>   cmake version 3.21.4
>   % cmake --version | head -1
>   cmake version 3.21.2
>   %
> 
> System CMake (the newer one) works, but the one from the Flatpak SDK still
> doesn't. Checking the diff between both versions I cannot find anything
> obvious that would affect how target_precompile_headers() works.

Ah, also my non-Flatpak build dir is configured to use Clang; I have to
try with a configuration more similar to what “build-webkit“ does.

That being said... I am tempted to disable PCH for the JavaScriptCore
target for the WPE port and try to fix it after this patch lands. Opionions?
Comment 61 Adrian Perez 2021-10-29 06:02:13 PDT
(In reply to Adrian Perez from comment #60)
> (In reply to Adrian Perez from comment #59)
> > (In reply to Adrian Perez from comment #58)
> > > I have now a version of the patch that also makes the WPE port build
> > > correctly. Just one pass more to clean up a couple of things and I will
> > > post the finished patch to land :-)
> > 
> > Wait! The plot thickens:
> > 
> >   % cmake --version | head -1
> >   cmake version 3.21.4
> >   % cmake --version | head -1
> >   cmake version 3.21.2
> >   %
> > 
> > System CMake (the newer one) works, but the one from the Flatpak SDK still
> > doesn't. Checking the diff between both versions I cannot find anything
> > obvious that would affect how target_precompile_headers() works.
> 
> Ah, also my non-Flatpak build dir is configured to use Clang; I have to
> try with a configuration more similar to what “build-webkit“ does.

Further narrowing down: This happens only when the compiler is GCC. Aither
CMake generates correct build rules when Clang is the compiler, or Clang is
ignoring passed PCH files at link time.
Comment 62 Adrian Perez 2021-11-03 14:54:28 PDT
(In reply to Adrian Perez from comment #61)
> (In reply to Adrian Perez from comment #60)
> > (In reply to Adrian Perez from comment #59)
> > > (In reply to Adrian Perez from comment #58)
> > > > I have now a version of the patch that also makes the WPE port build
> > > > correctly. Just one pass more to clean up a couple of things and I will
> > > > post the finished patch to land :-)
> > > 
> > > Wait! The plot thickens:
> > > 
> > >   % cmake --version | head -1
> > >   cmake version 3.21.4
> > >   % cmake --version | head -1
> > >   cmake version 3.21.2
> > >   %
> > > 
> > > System CMake (the newer one) works, but the one from the Flatpak SDK still
> > > doesn't. Checking the diff between both versions I cannot find anything
> > > obvious that would affect how target_precompile_headers() works.
> > 
> > Ah, also my non-Flatpak build dir is configured to use Clang; I have to
> > try with a configuration more similar to what “build-webkit“ does.
> 
> Further narrowing down: This happens only when the compiler is GCC. Aither
> CMake generates correct build rules when Clang is the compiler, or Clang is
> ignoring passed PCH files at link time.

Another round of testing later...

 - Both Clang and GCC trip with the .[pg]ch files being added in the
   linker command line.
 - Clang with *LLD* does not trip, no idea if it's LLD ignoring the
   file or the compiler driver removing it from the linker command.
 - In any case, the build.ninja/Makefile files produced by CMake have
   an invalid command line.
 - Seems to be a CMake bug, in the end.

Don has found https://gitlab.kitware.com/cmake/cmake/-/issues/22630 which
seems related but I built a patched CMake 3.21.3 with the associated fix
included and it did not help. At any rate, that is where I got an idea
to avoid adding calling target_precompile_headers() when the target is
of type OBJECT_LIBRARY — this works.
Comment 63 Adrian Perez 2021-11-03 14:57:31 PDT
Created attachment 443247 [details]
WIP Patch v13


Current version, for the sake of running it through the EWS.
Comment 64 Adrian Perez 2021-12-20 11:32:33 PST
Created attachment 447615 [details]
Patch v14

Rebased, let's try and sort out the Windows build issues
Comment 65 Fujii Hironori 2021-12-20 18:17:04 PST
Comment on attachment 447615 [details]
Patch v14

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

> Source/cmake/WebKitMacros.cmake:102
> +        if (NOT type STREQUAL OBJECT_LIBRARY)

This patch doesn't work as expected. type should be ${type}

> if (NOT ${type} STREQUAL OBJECT_LIBRARY)
Comment 66 Fujii Hironori 2021-12-20 18:18:03 PST
Comment on attachment 447615 [details]
Patch v14

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

>> Source/cmake/WebKitMacros.cmake:102
>> +        if (NOT type STREQUAL OBJECT_LIBRARY)
> 
> This patch doesn't work as expected. type should be ${type}

This condition is unfortunate for WinCairo port.
WinCairo is using OBJECT library for WebCore.
Comment 67 Fujii Hironori 2021-12-20 18:36:49 PST
Comment on attachment 447615 [details]
Patch v14

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

> Source/cmake/WebKitMacros.cmake:99
> +if (ENABLE_PRECOMPILED_HEADERS)

ENABLE_PRECOMPILED_HEADERS isn't defined yet in the first run.
I think you should move the condition into the function.

function(WEBKIT_ADD_PRECOMPILED_HEADER _target _header)
    if (ENABLE_PRECOMPILED_HEADERS)
       ...
    endif ()
endfunction()
Comment 68 Adrian Perez 2021-12-22 13:58:12 PST
(In reply to Fujii Hironori from comment #65)
> Comment on attachment 447615 [details]
> Patch v14
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447615&action=review
> 
> > Source/cmake/WebKitMacros.cmake:102
> > +        if (NOT type STREQUAL OBJECT_LIBRARY)
> 
> This patch doesn't work as expected. type should be ${type}
> 
> > if (NOT ${type} STREQUAL OBJECT_LIBRARY)

This works fine as-is, CMake tries to expand variables even if ${}
is not used to explicitly expand them inside “if” conditions (yeah,
CMake is weird, I can use ${type} here for the sake of clarity, though!)
Comment 69 Adrian Perez 2021-12-22 13:59:20 PST
(In reply to Fujii Hironori from comment #66)
> Comment on attachment 447615 [details]
> Patch v14
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447615&action=review
> 
> >> Source/cmake/WebKitMacros.cmake:102
> >> +        if (NOT type STREQUAL OBJECT_LIBRARY)
> > 
> > This patch doesn't work as expected. type should be ${type}
> 
> This condition is unfortunate for WinCairo port.
> WinCairo is using OBJECT library for WebCore.

We also use an object library for JavaScriptCore in the WPE port.
I hope that CMake eventually fixes this bug.
Comment 70 Adrian Perez 2021-12-22 14:00:24 PST
(In reply to Fujii Hironori from comment #67)
> Comment on attachment 447615 [details]
> Patch v14
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447615&action=review
> 
> > Source/cmake/WebKitMacros.cmake:99
> > +if (ENABLE_PRECOMPILED_HEADERS)
> 
> ENABLE_PRECOMPILED_HEADERS isn't defined yet in the first run.
> I think you should move the condition into the function.
> 
> function(WEBKIT_ADD_PRECOMPILED_HEADER _target _header)
>     if (ENABLE_PRECOMPILED_HEADERS)
>        ...
>     endif ()
> endfunction()

Will do, thanks for the suggestion.
Comment 71 Fujii Hironori 2021-12-22 16:45:43 PST
Comment on attachment 447615 [details]
Patch v14

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

>>>>> Source/cmake/WebKitMacros.cmake:102
>>>>> +        if (NOT type STREQUAL OBJECT_LIBRARY)
>>>> 
>>>> This patch doesn't work as expected. type should be ${type}
>>> 
>>> This condition is unfortunate for WinCairo port.
>>> WinCairo is using OBJECT library for WebCore.
>> 
>> This works fine as-is, CMake tries to expand variables even if ${}
>> is not used to explicitly expand them inside “if” conditions (yeah,
>> CMake is weird, I can use ${type} here for the sake of clarity, though!)
> 
> We also use an object library for JavaScriptCore in the WPE port.
> I hope that CMake eventually fixes this bug.

My bad. I tried it again locally. The condition works. Is it a good idea to quote a string?
if (NOT type STREQUAL "OBJECT_LIBRARY")
Comment 72 Fujii Hironori 2021-12-22 16:47:32 PST
Comment on attachment 447615 [details]
Patch v14

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

>>>>>> Source/cmake/WebKitMacros.cmake:102
>>>>>> +        if (NOT type STREQUAL OBJECT_LIBRARY)
>>>>> 
>>>>> This patch doesn't work as expected. type should be ${type}
>>>> 
>>>> This condition is unfortunate for WinCairo port.
>>>> WinCairo is using OBJECT library for WebCore.
>>> 
>>> This works fine as-is, CMake tries to expand variables even if ${}
>>> is not used to explicitly expand them inside “if” conditions (yeah,
>>> CMake is weird, I can use ${type} here for the sake of clarity, though!)
>> 
>> We also use an object library for JavaScriptCore in the WPE port.
>> I hope that CMake eventually fixes this bug.
> 
> My bad. I tried it again locally. The condition works. Is it a good idea to quote a string?
> if (NOT type STREQUAL "OBJECT_LIBRARY")

As far as I tested locally, target_precompile_headers works for OBJECT libraries in WinCairo port.
Can you specify more conditions for the workaround?
Comment 73 Adrian Perez 2022-02-16 14:59:01 PST
(In reply to Fujii Hironori from comment #72)
> Comment on attachment 447615 [details]
> Patch v14
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447615&action=review
> 
> >>>>>> Source/cmake/WebKitMacros.cmake:102
> >>>>>> +        if (NOT type STREQUAL OBJECT_LIBRARY)
> >>>>> 
> >>>>> This patch doesn't work as expected. type should be ${type}
> >>>> 
> >>>> This condition is unfortunate for WinCairo port.
> >>>> WinCairo is using OBJECT library for WebCore.
> >>> 
> >>> This works fine as-is, CMake tries to expand variables even if ${}
> >>> is not used to explicitly expand them inside “if” conditions (yeah,
> >>> CMake is weird, I can use ${type} here for the sake of clarity, though!)
> >> 
> >> We also use an object library for JavaScriptCore in the WPE port.
> >> I hope that CMake eventually fixes this bug.
> > 
> > My bad. I tried it again locally. The condition works. Is it a good idea to quote a string?
> > if (NOT type STREQUAL "OBJECT_LIBRARY")
> 
> As far as I tested locally, target_precompile_headers works for OBJECT
> libraries in WinCairo port.
> Can you specify more conditions for the workaround?

Sure, we could try and enable the workaround only for non-MSVC compilers.

(Also I need to rebase the patch and update it a bit.)