RESOLVED FIXED 143997
[CMake] Autogenerate cmakeconfig.h.cmake
https://bugs.webkit.org/show_bug.cgi?id=143997
Summary [CMake] Autogenerate cmakeconfig.h.cmake
Michael Catanzaro
Reported 2015-04-21 09:52:42 PDT
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.
Attachments
[CMake] Autogenerate cmakeconfig.h.cmake (8.92 KB, patch)
2015-04-21 11:09 PDT, Michael Catanzaro
no flags
Patch (22.55 KB, patch)
2015-04-22 12:10 PDT, Martin Robinson
no flags
Patch (23.29 KB, patch)
2015-04-22 14:17 PDT, Martin Robinson
no flags
Patch (23.96 KB, patch)
2015-04-22 14:19 PDT, Martin Robinson
no flags
Michael Catanzaro
Comment 1 2015-04-21 10:11:10 PDT
(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
Comment 2 2015-04-21 11:09:00 PDT
Created attachment 251243 [details] [CMake] Autogenerate cmakeconfig.h.cmake
Martin Robinson
Comment 3 2015-04-21 16:38:58 PDT
I have a version of this as well that I was playing with. Perhaps we can combine our work?
Michael Catanzaro
Comment 4 2015-04-21 17:53:50 PDT
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
Comment 5 2015-04-22 12:10:59 PDT
Michael Catanzaro
Comment 6 2015-04-22 12:38:51 PDT
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
Comment 7 2015-04-22 12:43:54 PDT
(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
Comment 8 2015-04-22 12:46:50 PDT
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
Comment 9 2015-04-22 13:04:24 PDT
Perhaps someone could review this patch which greatly simplifies the CMake build?
Michael Catanzaro
Comment 10 2015-04-22 13:48:31 PDT
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
Comment 11 2015-04-22 14:17:18 PDT
Martin Robinson
Comment 12 2015-04-22 14:17:49 PDT
(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
Comment 13 2015-04-22 14:19:41 PDT
Martin Robinson
Comment 14 2015-04-22 19:56:25 PDT
(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
Comment 15 2015-04-23 12:51:35 PDT
Nit: ENABLE_SMOOTH_SCROLLING isn't added in the right (alphabetical) place in OptionsGTK.cmake
Michael Catanzaro
Comment 16 2015-04-23 16:18:31 PDT
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
Comment 17 2015-04-23 16:19:10 PDT
mrobinson: mcatanzaro: Values are check with macros like #if ENABLE(blah) and #if USE(blah) which handle the undefined case.
Csaba Osztrogonác
Comment 18 2015-04-27 09:31:54 PDT
Comment on attachment 251366 [details] Patch Nice cleanup, r=me.
Csaba Osztrogonác
Comment 19 2015-04-27 09:33:31 PDT
(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
Comment 20 2015-04-27 13:31:28 PDT
Martin Robinson
Comment 21 2015-04-27 13:37:09 PDT
Comment on attachment 251366 [details] Patch Clearing review flag, because this patch has landed.
Carlos Garcia Campos
Comment 22 2015-05-08 01:09:53 PDT
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
Comment 23 2015-05-08 07:44:29 PDT
(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
Comment 24 2015-05-08 07:52:17 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.