[webkit-reviews] review denied: [Bug 139438] [CMake] Enable pre-compiled headers (PCH) (cotire) : [Attachment 319615] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has denied Adrian Perez
<aperez at igalia.com>'s request for review:
Bug 139438: [CMake] Enable pre-compiled headers (PCH) (cotire)
https://bugs.webkit.org/show_bug.cgi?id=139438

Attachment 319615: Patch

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




--- Comment #5 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 319615
  --> https://bugs.webkit.org/attachment.cgi?id=319615
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}/WebKi
tBuild/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}/WebKi
tBuild/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.