When launching build-webkit, compile only WebKit2 by default (without WebKit1) Add a new flag to enable WebKit1 compilation.
Created attachment 199665 [details] Patch
diff --git a/Source/autotools/ReadCommandLineArguments.m4 b/Source/autotools/ReadCommandLineArguments.m4 index ce40895956f975ff5c71f682d47a397d3540f8e2..90384558f5cdd278e34ce62eadcc5eeeed390585 100644 --- a/Source/autotools/ReadCommandLineArguments.m4 +++ b/Source/autotools/ReadCommandLineArguments.m4 @@ -1,19 +1,10 @@ AC_MSG_CHECKING([whether to build WebKit1]) AC_ARG_ENABLE(webkit1, - AC_HELP_STRING([--enable-webkit1], [build WebKit1 [default=yes]]), + AC_HELP_STRING([--enable-webkit1], [build WebKit1 [default=no]]), [], [enable_webkit1="yes"]) I guess you should change [enable_webkit1="yes"] by [enable_webkit1="no"] here.
Created attachment 199669 [details] Patch
By default, when launching 'build-webkit' both WebKit1 and WebKit2 are built. This patch makes the default option to be WebKit2 compilation but no WebKit1 compilation. In addition, it adds a new arg --webkit1 (gtk only) to allow compiling WebKit1. Summarizing: * ./Tools/Scripts/build-webkit (WK2 but no WK1) * ./Tools/Scripts/build-webkit --webkit1 (WK2 and WK1) * ./Tools/Scripts/build-webkit --webkit1 --no-webkit2 (Only WK1) * ./Tools/Scripts/build-webkit --no-webkit2 (nothing)
What is the motivation of that change? I know the focus is on wk2 now but still. What about the buildbots? Right we have better test coverage on the wk1 bot AFAIK. EWS also uses build-webkit, the wk1 EWS would need adaptations to keep building.
(In reply to comment #5) > What about the buildbots? Right we have better test coverage on the wk1 bot AFAIK. To be honest, I think this is a pretty big issue. We should change the bots to be testing WebKit2.
Xan suggested this to be the default behaviour as now the focus is on WK2 and building both by default is more time consuming. I have doubts too as there is already a setting that allows not to compile webkit1 (--no-webkit1). I don't fully understand the impact of this change (how it affects to bots, ews, etc) so I let you discuss the convenience of setting this as the default behaviour.
I also think we should make sure the WebKit2 tests coverage is as good as the WebKit1 too before switching the bots and disabling wk1 build. I would focus on stabilizing the WebKit2 bot and fixing the tests that pass in wk1 but not in wk2.
(In reply to comment #6) > (In reply to comment #5) > > > What about the buildbots? Right we have better test coverage on the wk1 bot AFAIK. > > To be honest, I think this is a pretty big issue. We should change the bots to be testing WebKit2. We have a WebKit2 test bot, but it's still behind the WebKit1 bot in terms of coverage :(
I agree with the WK2 testing coverage on all points, it's basically just a matter of determining which underlying hardware should be tasked with testing what configuration, and executing that. Of course it's important to get this done quick enough as the focus is on WK2 now. Also related, it should be discussed what amount of focus should be left on WebKit1 - API additions are already not welcomed there anymore, though basically most of the testing coverage is still on that variant. Putting that aside and focusing on the scope of this patch, I'd recommend disabling WebKit1 builds by default only in the Autotools build system. Developers and Buildbot builders should still rely on build-webkit's --no-webkit1 and --no-webkit2 flags to get the desired configuration. Similarly, I don't think the additional --webkit1 flag is needed. In short - release builds (straight via configure) should disable WK1 by default, developer builds (through build-webkit) should still enable WK1 unless specifically disabled (via --no-webkit1), at most until support for WK1GTK+ and all of its related code is thrown out of the source tree.
(In reply to comment #10) > Putting that aside and focusing on the scope of this patch, I'd recommend disabling WebKit1 builds by default only in the Autotools build system. Developers and Buildbot builders should still rely on build-webkit's --no-webkit1 and --no-webkit2 flags to get the desired configuration. Similarly, I don't think the additional --webkit1 flag is needed. > > In short - release builds (straight via configure) should disable WK1 by default, developer builds (through build-webkit) should still enable WK1 unless specifically disabled (via --no-webkit1), at most until support for WK1GTK+ and all of its related code is thrown out of the source tree. Another approach: Disable WebKit1 by default in Automake configuration, as it's done in the proposed patch now. For build-webkit, no new options would be required, but --no-webkit1 and --no-webkit2 options' support would be removed for the Autotools build system. This would mean that runnning `build-webkit --gtk` would fall back to only building WK2. One could still build WK1 by running `build-webkit --gtk --enable-webkit1` as the unrecognized arguments are added to the arguments list that's used when running autogen.sh. The related code that does that is located here: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitdirs.pm#L1980
(In reply to comment #11) > Another approach: > Disable WebKit1 by default in Automake configuration, as it's done in the proposed patch now. For build-webkit, no new options would be required, but --no-webkit1 and --no-webkit2 options' support would be removed for the Autotools build system. This would mean that runnning `build-webkit --gtk` would fall back to only building WK2. One could still build WK1 by running `build-webkit --gtk --enable-webkit1` as the unrecognized arguments are added to the arguments list that's used when running autogen.sh. > I like this
(In reply to comment #12) > (In reply to comment #11) > > Another approach: > > Disable WebKit1 by default in Automake configuration, as it's done in the proposed patch now. For build-webkit, no new options would be required, but --no-webkit1 and --no-webkit2 options' support would be removed for the Autotools build system. This would mean that runnning `build-webkit --gtk` would fall back to only building WK2. One could still build WK1 by running `build-webkit --gtk --enable-webkit1` as the unrecognized arguments are added to the arguments list that's used when running autogen.sh. > > > I like this I like it as well, though I have some small reservations about disabling WebKit1 by default for the Automake build. The primary consumers of the Automake path are downstream builders and they usually need WebKit1.
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > Another approach: > > > Disable WebKit1 by default in Automake configuration, as it's done in the proposed patch now. For build-webkit, no new options would be required, but --no-webkit1 and --no-webkit2 options' support would be removed for the Autotools build system. This would mean that runnning `build-webkit --gtk` would fall back to only building WK2. One could still build WK1 by running `build-webkit --gtk --enable-webkit1` as the unrecognized arguments are added to the arguments list that's used when running autogen.sh. > > > > > I like this > > I like it as well, though I have some small reservations about disabling WebKit1 by default for the Automake build. The primary consumers of the Automake path are downstream builders and they usually need WebKit1. Same concern here.
We'll be removing WebKit1 entirely soon, so we can close this in favor of that.
Comment on attachment 199669 [details] Patch Clearing review flag so it no longer shows up in the review queue.