Bug 143997 – [CMake] Autogenerate cmakeconfig.h.cmake

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Michael Catanzaro]

There are too many features missing from cmakeconfig.h.cmake:

ENABLE_ACCESSIBILITY
ENABLE_LETTERPRESS
ENABLE_MAC_LONG_PRESS
ENABLE_PLUGIN_PROCESS_GTK2
ENABLE_SPEECH_SYNTHESIS
ENABLE_THREADED_COMPOSITOR
USE_REDIRECTED_XCOMPOSITE_WINDOW

We should autogenerate this file so that we don't have to add new options in multiple places. It's too easy to forget.

There are also a few things listed in that file that are not proper WebKit features (but which probably should be), which will be lost if we autogenerate this file:

ENABLE_CREDENTIAL_STORAGE
ENABLE_EVENT_MODE_METATAGS
ENABLE_FIXED_REPORTED_SIZE
ENABLE_FTL_NATIVE_CALL_INLINING
ENABLE_INDEXED_DATABASE_IN_WORKERS
ENABLE_SMOOTH_SCROLLING
ENABLE_VIEWPORT_REFLOW
WTF_USE_GEOCLUE2
HAVE_LLVM
HAVE_GTK_UNIX_PRINTING
WTF_CPU_ARM64_CORTEXA53

We'll need to define those with WEBKIT_OPTION_DEFINE to keep them. But we should do that regardless, as otherwise they're not presented as options to the user in the list of features that prints after running cmake.

[Michael Catanzaro]

(In reply to comment #0)
> ENABLE_EVENT_MODE_METATAGS
> ENABLE_FIXED_REPORTED_SIZE
> ENABLE_VIEWPORT_REFLOW

These don't exist anymore; let's remove them.

> ENABLE_FTL_NATIVE_CALL_INLINING
> ENABLE_INDEXED_DATABASE_IN_WORKERS
> ENABLE_SMOOTH_SCROLLING

These should become options as-is.

> WTF_USE_GEOCLUE2
> WTF_CPU_ARM64_CORTEXA53
> HAVE_LLVM
> HAVE_GTK_UNIX_PRINTING

These should be renamed to USE_* when they become options.

[Michael Catanzaro]

Created attachment 251243 [details]
[CMake] Autogenerate cmakeconfig.h.cmake

[Martin Robinson]

I have a version of this as well that I was playing with. Perhaps we can combine our work?

[Michael Catanzaro]

I figure what I have is probably the simplest way (I'd just like to move the GDK_VERSION_MIN_REQUIRED out of the cross-platform file), but go ahead and post yours. :)

[Martin Robinson]

Created attachment 251352 [details]
Patch

[Michael Catanzaro]

OK, I like this approach a bit better than mine, since it allows exposing arbitrary CMake variables. And you took care of all the variables that would need exposed to the build. That's really awesome, because I was planning to do that, and now I don't have to.

I think I would prefer to change the code to drop the WTF prefix rather than vice-versa, but this way is fine too. We can always change it later: it's certainly less-intrusive to do it this way.

#if PLATFORM(GTK) && !defined(GTK_API_VERSION_2)
#define GDK_VERSION_MIN_REQUIRED GDK_VERSION_3_6
#endif

^ That needs to be dependent on whether we use ENABLE_X11_TARGET or ENABLE_WAYLAND_TARGET: the dependency is GDK 3.6 for X11 and 3.12 for Wayland.

ENABLE_3D_RENDERING and ENABLE_TEXTURE_MAPPER really needs to be set prior to calling WEBKIT_OPTION_END, but it's kind of a mess to fix that (you'd have to move a bunch of code) and it's no worse than what we have now, so we can change that in a later patch.

[Michael Catanzaro]

(In reply to comment #6)
> ^ That needs to be dependent on whether we use ENABLE_X11_TARGET or
> ENABLE_WAYLAND_TARGET: the dependency is GDK 3.6 for X11 and 3.12 for
> Wayland.

No it doesn't; I just misread the existing code. And:

mrobinson:  mcatanzaro: I think GDK_VERSION_MIN_REQUIRED controls what deprecation messages we see.

[Michael Catanzaro]

mcatanzaro:  So your patch looks good to me, no further comments. I think some of those WTF_USE_WHATEVERs we should remove and replace with WEBKIT_OPTION_DEFINE(ENABLE_WHATEVER), in particular the ENABLE_X11_TARGET and ENABLE_WAYLAND_TARGET, but we can do that in a separate future patch.

[Martin Robinson]

Perhaps someone could review this patch which greatly simplifies the CMake build?

[Michael Catanzaro]

Found a couple issues:

* We lost ENABLE_INDEXED_DATABASE_IN_WORKERS with this patch. It's overridden in OptionsEfl.cmake, OptionsGTK.cmake, and OptionsMac.cmake, but it's never defined with WEBKIT_OPTION_DEFINE. So we should add that to WebKitFeatures.cmake in this patch. (See also bug #144069.)
* We should export HAVE_LLVM in OptionsEFL.cmake as well. There's more cleanups we could do there, but as far as I see nothing that would cause breakage for EFL besides that. (I guess EWS does not use FTL, so it didn't catch this.)

[Martin Robinson]

Created attachment 251365 [details]
Patch

[Martin Robinson]

(In reply to comment #10)
> Found a couple issues:
> 
> * We lost ENABLE_INDEXED_DATABASE_IN_WORKERS with this patch. It's
> overridden in OptionsEfl.cmake, OptionsGTK.cmake, and OptionsMac.cmake, but
> it's never defined with WEBKIT_OPTION_DEFINE. So we should add that to
> WebKitFeatures.cmake in this patch. (See also bug #144069.)

Okay. I've added a definition for ENABLE_INDEXED_DATABASE_IN_WORKERS.

[Martin Robinson]

Created attachment 251366 [details]
Patch

[Martin Robinson]

(In reply to comment #12)
> (In reply to comment #10)
> > Found a couple issues:
> > 
> > * We lost ENABLE_INDEXED_DATABASE_IN_WORKERS with this patch. It's
> > overridden in OptionsEfl.cmake, OptionsGTK.cmake, and OptionsMac.cmake, but
> > it's never defined with WEBKIT_OPTION_DEFINE. So we should add that to
> > WebKitFeatures.cmake in this patch. (See also bug #144069.)
> 
> Okay. I've added a definition for ENABLE_INDEXED_DATABASE_IN_WORKERS.

Just to clarify, I think the current patch addresses both of Michael's original concerns.

[Michael Catanzaro]

Nit: ENABLE_SMOOTH_SCROLLING isn't added in the right (alphabetical) place in OptionsGTK.cmake

[Michael Catanzaro]

Hm, one more problem: is it really safe to use SET_AND_EXPOSE_TO_BUILD in a conditional? I'm thinking no, since we usually check values with #if and not #ifdef, we should always define them to 0 if not 1, and therefore only use SET_AND_EXPOSE_TO_BUILD outside of conditionals to ensure the variable is always set to something.

(Of course, if a value is really checked with #ifdef, then that would be wrong....)

[Michael Catanzaro]

mrobinson:  mcatanzaro: Values are check with macros like #if ENABLE(blah) and #if USE(blah) which handle the undefined case.

[Csaba Osztrogonác]

Comment on attachment 251366 [details]
Patch

Nice cleanup, r=me.

[Csaba Osztrogonác]

(In reply to comment #18)
> Comment on attachment 251366 [details]
> Patch
> 
> Nice cleanup, r=me.

But it should be updated before landing due to some recent changes.

[Martin Robinson]

Committed r183416: <http://trac.webkit.org/changeset/183416>

[Martin Robinson]

Comment on attachment 251366 [details]
Patch

Clearing review flag, because this patch has landed.

[Carlos Garcia Campos]

Comment on attachment 251366 [details]
Patch

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

> Source/WTF/wtf/Platform.h:493
> +#if PLATFORM(GTK) && !defined(GTK_API_VERSION_2)
> +#define GDK_VERSION_MIN_REQUIRED GDK_VERSION_3_6
> +#endif

Why was this moved here? This doesn't affect other tools like MiniBrowser, that include cmakeconfig.h. This brought back a lot of GTK+ deprecation warnings when building minibrowser

[Michael Catanzaro]

(In reply to comment #22)
> Comment on attachment 251366 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251366&action=review
> 
> > Source/WTF/wtf/Platform.h:493
> > +#if PLATFORM(GTK) && !defined(GTK_API_VERSION_2)
> > +#define GDK_VERSION_MIN_REQUIRED GDK_VERSION_3_6
> > +#endif
> 
> Why was this moved here? This doesn't affect other tools like MiniBrowser,
> that include cmakeconfig.h. This brought back a lot of GTK+ deprecation
> warnings when building minibrowser

Martin and I were wondering where those were coming from! Good catch. It was moved from cmakeconfig.h.cmake because we deleted that file. It's tricky to set in OptionsGTK.cmake since it has to not be set when compiling the GTK2 plugin process. Probably the best solution is to add it to Tools/MiniBrowser/gtk/CMakeLists.txt.

[Martin Robinson]

(In reply to comment #23)

> Martin and I were wondering where those were coming from! Good catch. It was
> moved from cmakeconfig.h.cmake because we deleted that file. It's tricky to
> set in OptionsGTK.cmake since it has to not be set when compiling the GTK2
> plugin process. Probably the best solution is to add it to
> Tools/MiniBrowser/gtk/CMakeLists.txt.

Michael is absolutely correct. It was moved here because the place it was moved from no longer exists.