Bug 177338 – Remove build-webkit's notion of feature flags having a default value

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Remove build-webkit's notion of feature flags having a default value

[Tim Horton]

Created attachment 321505 [details]
Patch

[Tim Horton]

As discussed on webkit-dev.

This depends on a change that mcatanzaro needs to make to their buildbots.

[Alex Christensen]

Are we planning to remove the list entirely?

[Tim Horton]

Eventually. Maybe even soon!

[Michael Catanzaro]

(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.

[Carlos Alberto Lopez Perez]

(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

[Carlos Alberto Lopez Perez]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 321555 [details]
Patch

[Michael Catanzaro]

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 [e-mail] to restart the buildbots for the configuration changes to take effect.

[Michael Catanzaro]

Looks like the EWS are broken for an unrelated reason. :/

[Tim Horton]

LGTM but I won't r+ something that is partially my patch :)

[Michael Catanzaro]

I think Carlos Lopez will probably want to review it anyway.

[Alex Christensen]

Comment on attachment 321555 [details]
Patch

r=me
I'd be ok if Carlos looked at it before landing if that happens soonish.

[Carlos Alberto Lopez Perez]

Comment on attachment 321555 [details]
Patch

It looks ok.
Feel free to land. Thanks.

[Michael Catanzaro]

Committed r222394: <http://trac.webkit.org/changeset/222394>

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #10)
> But Carlos Lopez says we need to ask
> [e-mail] to restart the buildbots for the configuration changes to
> take effect.

Request sent.

[Matt Lewis]

Reverted r222394 for reason:

This broke the Windows Debug Build.

Committed r222400: <http://trac.webkit.org/changeset/222400>

[Matt Lewis]

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

[Michael Catanzaro]

Probably nobody ever tested Windows with the CMake featureset.

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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.

[Tim Horton]

(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.

[Tim Horton]

Created attachment 321803 [details]
reews

[Michael Catanzaro]

I'm not sure if EWS will catch this, unfortunately, because only the Windows debug bot was broken, not the release bot.

[Carlos Alberto Lopez Perez]

> 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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Though WebPreferencesDefinitions.h is generated, now, so some changes will be needed there....

[Tim Horton]

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!

[Michael Catanzaro]

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....

[Don Olmstead]

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

[Michael Catanzaro]

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.

[Don Olmstead]

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.

[Don Olmstead]

Created attachment 327435 [details]
Patch

Fix review comments

[Don Olmstead]

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.

[Don Olmstead]

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.

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

(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

[Carlos Alberto Lopez Perez]

Comment on attachment 327435 [details]
Patch

Feel free to land as it is. Just watch the bots after doing that.

[WebKit Commit Bot]

Comment on attachment 327435 [details]
Patch

Clearing flags on attachment: 327435

Committed r225098: <https://trac.webkit.org/changeset/225098>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35669022>

[Michael Catanzaro]

I'll ask for a buildbot reset.

[Michael Catanzaro]

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

[Carlos Alberto Lopez Perez]

(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)

[Michael Catanzaro]

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.

[Tim Horton]

Thanks for relanding this and fixing it up!

[Don Olmstead]

(In reply to Tim Horton from comment #47)
> Thanks for relanding this and fixing it up!

👍 no problem!

[Carlos Alberto Lopez Perez]

Seems this caused some features to change of default value for the GTK+ port. Reported that in bug 179989