Remove build-webkit's notion of feature flags having a default value
Created attachment 321505 [details] Patch
As discussed on webkit-dev. This depends on a change that mcatanzaro needs to make to their buildbots.
Are we planning to remove the list entirely?
Eventually. Maybe even soon!
(In reply to Tim Horton from comment #2) > As discussed on webkit-dev. > > This depends on a change that mcatanzaro needs to make to their buildbots. As soon as I figure out where the buildbot configuration lives. :) Carlos Lopez often handles such issues. Hopefully we can get it sorted out today, but if not it should be very soon. Patch looks good.
(In reply to Michael Catanzaro from comment #5) > (In reply to Tim Horton from comment #2) > > As discussed on webkit-dev. > > > > This depends on a change that mcatanzaro needs to make to their buildbots. > > As soon as I figure out where the buildbot configuration lives. :) Carlos > Lopez often handles such issues. Hopefully we can get it sorted out today, > but if not it should be very soon. > > Patch looks good. The buildbot config is all on the repository at Tools/BuildSlaveSupport/build.webkit.org-config After touching anything there be sure to run the unit tests with this: ( cd Tools/BuildSlaveSupport/build.webkit.org-config ; ./mastercfg_unittest.py ) And if you want more testing the script I uploaded to bug 169084 (still waiting for review) allows to run a copy of the buildbot master on your dev machine for testing purposes
Comment on attachment 321505 [details] Patch I think we (GTK/WPE) need a way to enable experimental features in development by default with a common switch for all our bots except a few (the debian and ubuntu ones). Please don't land this still as I think is going to break our ports. Our EWS doesn't run tests. We need to check that we keep enabling the same features after this lands and it doesn't break lot of tests.
After discussing this more, we decided we still need the --default-cmake-features flag so that developers still get the same default feature set as is used by our bots, not the end user feature set. This is important for consistent layout test results. But we can rename it to --experimental-features/--no-experimental-features now that FeatureLists.pm does not have its own defaults. --experimental-features will be the default. Our buildbot config will be modified to pass --no-experimental-features instead of --default-cmake-features for the bots that are currently using --default-cmake-features. Instead of enabling the FeatureList.pm defaults, we'll have --experimental-features tell build-webkit to pass -DENABLE_EXPERIMENTAL_FEATURES down to CMake. OptionsGTK.cmake and OptionsWPE.cmake will be modified to recognize that flag and have it change the defaults for ENABLE_MEDIA_SOURCE and ENABLE_MEDIA_STREAM.
Created attachment 321555 [details] Patch
Tim, Carlos Lopez, could you both review this please? I've tested it to ensure GTK gets the right features enabled both when using (a) build-webkit without --no-experimental-features, (b) build-webkit with --no-experimental-features, and (c) when not using build-webkit. I failed to run the buildbot unittests, but they were already broken ('ImportError: No module named zope.interface') and I've just renamed options so I'm sure that's fine. But Carlos Lopez says we need to ask admin@webkit.org to restart the buildbots for the configuration changes to take effect.
Looks like the EWS are broken for an unrelated reason. :/
LGTM but I won't r+ something that is partially my patch :)
I think Carlos Lopez will probably want to review it anyway.
Comment on attachment 321555 [details] Patch r=me I'd be ok if Carlos looked at it before landing if that happens soonish.
Comment on attachment 321555 [details] Patch It looks ok. Feel free to land. Thanks.
Committed r222394: <http://trac.webkit.org/changeset/222394>
(In reply to Michael Catanzaro from comment #10) > But Carlos Lopez says we need to ask > admin@webkit.org to restart the buildbots for the configuration changes to > take effect. Request sent.
Reverted r222394 for reason: This broke the Windows Debug Build. Committed r222400: <http://trac.webkit.org/changeset/222400>
Build break: https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/4233 https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/4233/steps/compile-webkit/logs/stdio
Probably nobody ever tested Windows with the CMake featureset.
It looks like the Windows build is still failing with the same errors, despite the rollout. I've triggered a clean build so that it should hopefully pick up the features from build-webkit again in the meantime.
(In reply to Michael Catanzaro from comment #21) > It looks like the Windows build is still failing with the same errors, > despite the rollout. I've triggered a clean build so that it should > hopefully pick up the features from build-webkit again in the meantime. The force build worked.
(In reply to Michael Catanzaro from comment #20) > Probably nobody ever tested Windows with the CMake featureset. And this is exactly why it's important to fix this :) I think I didn't notice the EWS fail because the build was already broken (and the second time it didn't get there), but let's run it again and see what's up.
Created attachment 321803 [details] reews
I'm not sure if EWS will catch this, unfortunately, because only the Windows debug bot was broken, not the release bot.
> I'm not sure if EWS will catch this, unfortunately, because only the Windows > debug bot was broken, not the release bot. Is there someone able to test this on windows with a debug build? Otherwise I suggest to land this as it is and try to fix the breakage after it lands by checking the windows bots.
(In reply to Carlos Alberto Lopez Perez from comment #26) > Is there someone able to test this on windows with a debug build? > > Otherwise I suggest to land this as it is and try to fix the breakage after > it lands by checking the windows bots. Today I rediscovered that we still have the defaults in FeatureList.pm. :( Would be great to try landing this again, indeed. I guess Windows debug will probably still be broken, though.
Though WebPreferencesDefinitions.h is generated, now, so some changes will be needed there....
Sadly I don't have time to fiddle with this at the moment, but if one of you wants to take it to the finish line, feel free!
The problem is, I don't think we want to debug the cryptic error on the Windows debug bot. So we'll probably let this sit for a while. I'm sure someone will be interested in picking it up again eventually....
Created attachment 327428 [details] [WIP] Rebased Rebased the patch. Currently throwing it at the bots to see what happens. I'll try and build AppleWin debug locally to see whats going on there. Will fix the changelogs after that to reflect the original authors
Comment on attachment 327428 [details] [WIP] Rebased View in context: https://bugs.webkit.org/attachment.cgi?id=327428&action=review > Source/cmake/OptionsWPE.cmake:16 > -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCELERATED_2D_CANVAS PUBLIC OFF) > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCELERATED_2D_CANVAS PUBLIC ${ENABLE_EXPERIMENTAL_FEATURES}) I just changed this the other day. This one should be always OFF now. > Source/cmake/OptionsWPE.cmake:19 > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MEDIA_SOURCE PUBLIC ${ENABLE_EXPERIMENTAL_FEATURES}) And this one should always be ON. So you can just get rid of the changes in OptionsWPE.cmake; we don't need to use ENABLE_EXPERIMENTAL_FEATURES there. The changes in OptionsGTK.cmake are all still good.
Ok here's an update on AppleWin debug builds. So like before this patch does not build with AppleWin on debug. It appears that https://bugs.webkit.org/show_bug.cgi?id=179867 introduced a warning due to the change of signatures for the methods and warnings are treated as errors in these builds. By disabling the warning I was able to get a build of WebKitLegacy working. This appears to be further than the previous patch got so I THINK we're ok.
Created attachment 327435 [details] Patch Fix review comments
Comment on attachment 327435 [details] Patch Ok I think we're back in business with this patch. It should now match the old one but please double check me here. AppleWin debug build seems to be broken currently so I don't know when that will be resolved. Hopefully before this patch requires another rebase.
I did a local build of wincairo and it built successfully. The last patch also built successfully so I think its a false positive and am ok with fixing in a followup if there is actually an issue.
Comment on attachment 327435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327435&action=review > Source/cmake/OptionsGTK.cmake:161 > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEB_RTC PRIVATE ${ENABLE_EXPERIMENTAL_FEATURES}) I think this line should be removed. We don't longer enable webrtc (not even on experimental builds) after we dropped the openwebrtc backend. The new backend based on libwebrtc is yet not ready to be enabled
(In reply to Carlos Alberto Lopez Perez from comment #37) > Comment on attachment 327435 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=327435&action=review > > > Source/cmake/OptionsGTK.cmake:161 > > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEB_RTC PRIVATE ${ENABLE_EXPERIMENTAL_FEATURES}) > > I think this line should be removed. > We don't longer enable webrtc (not even on experimental builds) after we > dropped the openwebrtc backend. > The new backend based on libwebrtc is yet not ready to be enabled agh.. forget that, i overlooked it is enabled on Tools/Scripts/webkitperl/FeatureList.pm
Comment on attachment 327435 [details] Patch Feel free to land as it is. Just watch the bots after doing that.
Comment on attachment 327435 [details] Patch Clearing flags on attachment: 327435 Committed r225098: <https://trac.webkit.org/changeset/225098>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35669022>
I'll ask for a buildbot reset.
The Debian stable buildbot is broken. It's a minor bot, and it will be fixed by the buildbot reset, so not a big deal. (This bot is now building with ENABLE_MEDIA_SOURCE, which it is not supposed to do, until the next reset.) But I think it is a real MSE bug: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20(Build)/builds/7699/steps/compile-webkit/logs/stdio
(In reply to Michael Catanzaro from comment #44) > The Debian stable buildbot is broken. It's a minor bot, and it will be fixed > by the buildbot reset, so not a big deal. Right, let's wait for the rest. > (This bot is now building with > ENABLE_MEDIA_SOURCE, which it is not supposed to do, until the next reset.) > But I think it is a real MSE bug: > > https://build.webkit.org/builders/GTK%20Linux%2064- > bit%20Release%20Debian%20Stable%20(Build)/builds/7699/steps/compile-webkit/ > logs/stdio GST_BUFFER_DTS_OR_PTS definition appeared on GStreamer 1.8. That bot still runs on Debian jessie (which has GStreamer 1.4)
OK that's fine, ENABLE_MEDIA_SOURCE does not need to work with GStreamer 1.4, and that will get disabled again when the buildbot master is restarted.
Thanks for relanding this and fixing it up!
(In reply to Tim Horton from comment #47) > Thanks for relanding this and fixing it up! 👍 no problem!
Seems this caused some features to change of default value for the GTK+ port. Reported that in bug 179989