267080 2024-01-04 07:35:01 -0800 FEATURE_DEFINES_WITH_SPACE_SEPARATOR is fragile 2024-01-11 07:35:04 -0800 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Unspecified NEW https://bugs.webkit.org/show_bug.cgi?id=267123 P2 Normal --- 1 annevk webkit-unassigned bugs-noreply emw mcatanzaro mike ntim webkit-bug-importer oldest_to_newest 2002959 0 annevk 2024-01-04 07:35:01 -0800 We started auto-generating more selector parsing code and that ends up depending on this define. However, as I found in bug 266947 what this define includes is a subset of all environment variables (which the Cocoa build process provides) and as such subtle bugs can occur where parsing certain pseudo-classes and pseudo-elements won't work on Glib platforms for inexplicable reasons. 2002983 1 annevk 2024-01-04 10:11:15 -0800 In particular GTK enables INPUT_TYPE_DATE but at this point in processing that hasn't been translated to enabling DATE_AND_TIME_INPUT_TYPES it seems, but some pseudo-elements do depend on that. As a result the pseudo-elements that depend on that end up being unsupported. I've hacked around this as this seems like a general flaw in this setup. 2003017 2 mcatanzaro 2024-01-04 14:05:17 -0800 In this comment: https://github.com/WebKit/WebKit/pull/22274#discussion_r1442124307 I found the problem is just that the feature isn't defined properly. 2003018 3 mcatanzaro 2024-01-04 14:07:50 -0800 That is, FEATURE_DEFINES_WITH_SPACE_SEPARATOR only contains features that actually exist. To declare a feature, you have to use WEBKIT_OPTION_DEFINE(). This can be done in WebKitFeatures.cmake for cross-platform features, or Options*.cmake for port-specific features. tbh I don't think there's any real problem with FEATURE_DEFINES_WITH_SPACE_SEPARATOR. And I cannot think of any way to enforce that new feature flags are created properly. If we forget to define the flag for CMake, there's not really anything that CMake can do about that. 2003123 4 annevk 2024-01-04 23:46:30 -0800 I think the problem is that these scripts that generate C++ don't just expect ENABLE() defines, it can be anything that follows `#if`. And that won't work for Glib platforms. 2003145 5 annevk 2024-01-05 03:00:19 -0800 This is also true when they are used in html.css it seems: https://github.com/WebKit/WebKit/pull/22424 2003153 6 mcatanzaro 2024-01-05 04:54:42 -0800 (In reply to Anne van Kesteren from comment #4) > I think the problem is that these scripts that generate C++ don't just > expect ENABLE() defines, it can be anything that follows `#if`. And that > won't work for Glib platforms. There's nothing special about GLib here. Surely it won't work for the macOS/Windows/PlayStation CMake builds either? 2003155 7 mcatanzaro 2024-01-05 04:58:37 -0800 Is it possible to change the script to fail if we use a conditional value and it's not defined to either 0 or 1? 2003178 8 annevk 2024-01-05 09:05:07 -0800 Maybe? That code predated Tim and I touching the script and is also present in other scripts. I'm also not sure how much more time I can put into this issue, especially as vacation time is nearly over. It also won't help with the code introduced in html.css, right? I think you are correct that it's not technically a CMake problem, but CMake builds will continue to run into subtle issues due to this mismatch. 2003195 9 mcatanzaro 2024-01-05 10:54:42 -0800 (In reply to Anne van Kesteren from comment #8) > Maybe? That code predated Tim and I touching the script and is also present > in other scripts. I'm also not sure how much more time I can put into this > issue, especially as vacation time is nearly over. Ah! Well it's certainly not your job to fix every bug, especially if you are doing so during vacation time. > It also won't help with the code introduced in html.css, right? Hmm, I wonder how this file works at all? At first I thought it was a misnamed template file that's used to generate the real html.css and which should be renamed to html.css.in. But checking Source/WebCore/CMakeLists.txt, I see that no, it's just really the actual final html.css that gets processed by WebKit. That seems really weird? I have no clue how this file is supposed to work, because no command removes the build guards. I suspect this is a completely separate problem, actually. This html.css just doesn't make sense. Am I missing something? To make it actually work, we could preprocess html.css using unifdef, passing FEATURE_DEFINES_WITH_SPACE_SEPARATOR to unifdef. > I think you are correct that it's not technically a CMake problem, but CMake > builds will continue to run into subtle issues due to this mismatch. Well it's impossible for CMake to define values that it doesn't know about, so ultimately they're going to have to be defined one way or another if a script wants to use them. WEBKIT_OPTION_DEFINE() is surely the best way to do that. 2003724 10 emw 2024-01-08 16:57:20 -0800 (In reply to Michael Catanzaro from comment #7) > Is it possible to change the script to fail if we use a conditional value > and it's not defined to either 0 or 1? This is what came to mind for me too. I think it would be more intuitive for the script to act like a normal preprocessor with -Wundef enabled. That probably means passing _all_ of the known features into the FEATURE_DEFINES_WITH_SPACE_SEPARATOR string, using `FEATURE_NAME=0` to denote disabled features. 2003815 11 mcatanzaro 2024-01-09 06:21:29 -0800 Agreed. Currently the lists FEATURE_DEFINES and FEATURE_DEFINES_WITH_SPACE_SEPARATOR are populated in WEBKIT_OPTION_END. Unfortunately only values that are enabled are added.