Bug 237170 – [CMake] Disabling ENABLE_WEBCORE is ignored when cmake configuration runs again.

• Component: CMake
• Status: RESOLVED FIXED

[Basuke Suzuki]

When I create a project by cmake with -DENABLE_WEBCORE=OFF, it generates a proper project which doesn't build WebCore and above at all. But if I change some CMake related files and it is required to re-run cmake configuration, then it creates a project with the ENABLE_WEBCORE is ON.

> cmake -DCMAKE_TOOLCHAIN_FILE=Platform\PlayStation5 -G Ninja -DPORT="PlayStation" -DENABLE_WEBCORE=OFF -DCMAKE_BUILD_TYPE=Release -B WebKitBuild/jsc
> cmake --build WebKitBuild/jsc
# build is good

Then modify CMake files such as PlatformPlayStation.cmake, and re-run build,
> cmake --build WebKitBuild/jsc
# build is NG, ENABLE_WEBCORE is back to ON

I expected the project should use same variables. Unless re-running configuration, the project is stable. I think this variable should be cached in CMake.

[Basuke Suzuki]

This change seems working as I expected, but I'm not sure this is the correct direction.

diff --git a/Source/cmake/WebKitCommon.cmake b/Source/cmake/WebKitCommon.cmake
index e58c7069abb1..c3389c8e46f7 100644
--- a/Source/cmake/WebKitCommon.cmake
+++ b/Source/cmake/WebKitCommon.cmake
@@ -13,19 +13,19 @@ if (NOT HAS_RUN_WEBKIT_COMMON)
         message(STATUS "The CMake build type is: ${CMAKE_BUILD_TYPE}")
     endif ()

-    set(ENABLE_JAVASCRIPTCORE ON)
-    set(ENABLE_WEBCORE ON)
+    set(ENABLE_JAVASCRIPTCORE ON CACHE STRING "Enable JavaScriptCore build")
+    set(ENABLE_WEBCORE ON ON CACHE STRING "Enable WebCore build")

     if (NOT DEFINED ENABLE_WEBKIT)
-        set(ENABLE_WEBKIT ON)
+        set(ENABLE_WEBKIT ON ON CACHE STRING "Enable WebKit build")
     endif ()
 
     if (NOT DEFINED ENABLE_TOOLS AND EXISTS "${CMAKE_SOURCE_DIR}/Tools")
-        set(ENABLE_TOOLS ON)
+        set(ENABLE_TOOLS ON ON CACHE STRING "Enable Tools build")
     endif ()

     if (NOT DEFINED ENABLE_WEBINSPECTORUI)
-        set(ENABLE_WEBINSPECTORUI ON)
+        set(ENABLE_WEBINSPECTORUI ON ON CACHE STRING "Enable WebInspector UI build")
     endif ()

     # -----------------------------------------------------------------------------

[Fujii Hironori]

If you want to change ENABLE_WEBCORE from outside, using option is the CMake way in such case.
https://cmake.org/cmake/help/latest/command/option.html

[Stephan Szabo]

It seems like it'd work better if those were cache variables rather than normal ones if we're expecting them to have overwritten values from the command line that'll persist.

The others are using an if(NOT DEFINED XXX) block, which may also work if put around the set in WebKitCommon, but seems like it's just doing the same sort of thing that a cache variable would already do. Is there some piece of the cache variable behavior that's problematic for what we're doing (for example maybe around the HAS_RUN_WEBKIT_COMMON bits?)

[Basuke Suzuki]

Created attachment 453425 [details]
PATCH

[Fujii Hironori]

Comment on attachment 453425 [details]
PATCH

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

> Source/cmake/WebKitCommon.cmake:18
> +    option(ENABLE_WEBKIT "Enable building WebKit" ON)

Some ports set these values OFF by default.
It'd be nice if we can change the default value of these options.
I think there two ways to do so.
1. Using option twice, in WebKitCommon.cmake and Options${PORT}.cmake.
2. Adding new variables, for example, ENABLE_WEBCORE_DEFAULT.
But, it seemingly needs some refactoring. I don't object landing this patch if you want.

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #5)
> Comment on attachment 453425 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453425&action=review
> 
> > Source/cmake/WebKitCommon.cmake:18
> > +    option(ENABLE_WEBKIT "Enable building WebKit" ON)
> 
> Some ports set these values OFF by default.
> It'd be nice if we can change the default value of these options.
> I think there two ways to do so.
> 1. Using option twice, in WebKitCommon.cmake and Options${PORT}.cmake.
> 2. Adding new variables, for example, ENABLE_WEBCORE_DEFAULT.
> But, it seemingly needs some refactoring. I don't object landing this patch
> if you want.

Correct. While touching the code, I was fighting with the attempt of refactoring more and more. Then ended up to just doing the straight-forward thing right now.

As you say, I'll land this right now and do more refactoring later.

[Basuke Suzuki]

Created attachment 453478 [details]
PATCH

[EWS]

Committed r290636 (247909@main): <https://commits.webkit.org/247909@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453478 [details].