Bug 104690 – [EFL] The WebKit2 bots are building and executing WebKit1 tests

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Thiago Marcos P. Santos]

It is a waste of CPU time since we have dedicated WebKit1 bots for that, not to mention that WK1 can make WK2 bots red.

[Laszlo Gombos]

If it is a desire you do not even need to build the WK1 bits now, see

http://trac.webkit.org/changeset/136398
http://trac.webkit.org/changeset/136957

[Thiago Marcos P. Santos]

Created attachment 178865 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 178865 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178865&action=review

> Tools/Scripts/build-webkit:137
>    --only-webkit                     Build only the WebKit project
>    --no-webkit2                      Omit WebKit2 code from the build

What is the difference? only-webkit? no-webkit2 ?

> Tools/Scripts/build-webkit:139
> +  --only-webkit2                    Build only the WebKit2 project (EFL only)

Qt (least used to) support this

[Laszlo Gombos]

Have you considered using --cmakeargs="-DENABLE_WEBKIT=OFF" which would not require changing the build-webkit script) ? 

Introducing a new global flag (--only-webkit2) for all the 9 ports seems a bit of overkill for me as the flag is only available and hooked up for 1 port (EFL).

[Laszlo Gombos]

(In reply to comment #3)
> (From update of attachment 178865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178865&action=review
> 
> > Tools/Scripts/build-webkit:137
> >    --only-webkit                     Build only the WebKit project
> >    --no-webkit2                      Omit WebKit2 code from the build
> 
> What is the difference? only-webkit? no-webkit2 ?

--only-webkit means here the Source/WebKit directory (it does not include WebCore), which is quite different than --no-webkit2 (which includes WebCore).

[Dominik Röttsches (drott)]

Comment on attachment 178865 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178865&action=review

After having a look and discussing offline - it'd be great if we could create a CompileClass and handle the "wk2 bot" distinction by using the existing BuildAndTestWebKit2 factory. Thanks for going the extra mile, I can help testing this on a separate master if needed.

> Tools/BuildSlaveSupport/build.webkit.org-config/config.json:72
> +                    { "name": "efl-linux-slave-2", "platform": "efl-wk2" },

I'd be great if we could avoid introducing a "virtual" platform here.

[Thiago Marcos P. Santos]

(In reply to comment #4)
> Have you considered using --cmakeargs="-DENABLE_WEBKIT=OFF" which would not require changing the build-webkit script) ? 
> 
> Introducing a new global flag (--only-webkit2) for all the 9 ports seems a bit of overkill for me as the flag is only available and hooked up for 1 port (EFL).

Hi Laszlo,

I'm reworking the patch so this --only-webkit2 flag will make more sense.

[Thiago Marcos P. Santos]

Created attachment 179258 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 179258 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179258&action=review

The idea is good.

Let me to think about the master.cfg change a little bit.

> Tools/Scripts/build-webkit:63
> +my $onlyWebKit2 = 0;

noWebKit1 similar to noWebKit2

> Tools/Scripts/build-webkit:140
> +  --only-webkit2                    Build only the WebKit2 project (EFL only)
> +

I prefer --no-webkit1, similar to --no-webkit2.

> Tools/Scripts/build-webkit:155
> +    'only-webkit2' => \$onlyWebKit2,

ditto.

[Csaba Osztrogonác]

Ouch, I shouldn't click on force submit ...

[Csaba Osztrogonác]

Let's see the master.cfg change. Adding the new --no-webkit1 to all slaves can
be confusing and can cause problems on other platforms implements this option.
What if you simple concatenate --no-webkit1 to the build command for only
EFL-WK1 slaves? It would be better if we can determine wk1/wk2 from platform
... but it seems now we can do it from builddir only:

if platform == 'efl' and not builddir.endswith('wk2'):
            self.setCommand(self.command + ['--no-webkit1'])

What do you think about this simpler change now? Or the perfect fix 
would be to change efl platforms efl-wk1, efl-wk2. But I think it
need more changes not to break existing code paths, for example all
platform == 'efl' -> platform.startswith("efl")

[Thiago Marcos P. Santos]

(In reply to comment #11)
> Let's see the master.cfg change. Adding the new --no-webkit1 to all slaves can
> be confusing and can cause problems on other platforms implements this option.
> What if you simple concatenate --no-webkit1 to the build command for only
> EFL-WK1 slaves? It would be better if we can determine wk1/wk2 from platform
> ... but it seems now we can do it from builddir only:
> 
> if platform == 'efl' and not builddir.endswith('wk2'):
>             self.setCommand(self.command + ['--no-webkit1'])
> 
> What do you think about this simpler change now? Or the perfect fix 
> would be to change efl platforms efl-wk1, efl-wk2. But I think it
> need more changes not to break existing code paths, for example all
> platform == 'efl' -> platform.startswith("efl")

It wont be added to all slaves, but only to those using the BuildAndTestWebKit2Factory (only the EFL WK2 bots for instance).

[Csaba Osztrogonác]

(In reply to comment #12)
> It wont be added to all slaves, but only to those using the BuildAndTestWebKit2Factory (only the EFL WK2 bots for instance).

It is true now. But what if X port added a new BuildAndTestWebKit2Factory 
slave? I think these classes should work in general.

I don't want to block your change. But I prefer more general fixes. ;)

What about the two-liner change I proposed?

[Dominik Röttsches (drott)]

(In reply to comment #11)
> Let's see the master.cfg change. Adding the new --no-webkit1 to all slaves can
> be confusing and can cause problems on other platforms implements this option.
> What if you simple concatenate --no-webkit1 to the build command for only
> EFL-WK1 slaves? It would be better if we can determine wk1/wk2 from platform
> ... but it seems now we can do it from builddir only:
> 
> if platform == 'efl' and not builddir.endswith('wk2'):
>             self.setCommand(self.command + ['--no-webkit1'])
> 
> What do you think about this simpler change now? Or the perfect fix 
> would be to change efl platforms efl-wk1, efl-wk2. 

I disagree that this would be the perfect fix. I discussed this with Thiago here: In my opinion we should not create a "virtual" platform efl-wk1 and efl-wk2 to distinguish a builder configuration. A platform variable should just describe the platform, and the builder configuration should be used for telling the builder what to do. We are not making changes to the platform as a whole, we would just like to have certain builders not build webkit 1 any more.

And re your other comments
> It is true now. But what if X port added a new BuildAndTestWebKit2Factory 
> slave? I think these classes should work in general.

They do work in general, right? We can argue the names could be more descriptive but otherwise these recipes just do what the name says. We could rename the CompileWebKit2 class to something like CompileWebKit2Only or so - and that's fine. I prefer this quite much over putting efl-* "virtual" platforms everywhere, including the config.json.

[Csaba Osztrogonác]

As I said I don't want to block your work. I only shared my ideas how can we
make it better. It's all the same for me if you want to use efl / efl-wk1 /
efl-wk2 platform names or not.

I just don't understand why do you want this complex change which can affect
other ports in the future instead of the simple two-liners? I have only one
request: Please don't change BuildAndTestWebKit2Factory to only EFL friendly.
I still think --no-webkit1 shouldn't be passed automatically to a slave typed
BuildAndTestWebKit2, because --no-webkit1 is an EFL only willing for the bots.

[Thiago Marcos P. Santos]

(In reply to comment #15)
> As I said I don't want to block your work. I only shared my ideas how can we
> make it better. It's all the same for me if you want to use efl / efl-wk1 /
> efl-wk2 platform names or not.
> 
> I just don't understand why do you want this complex change which can affect
> other ports in the future instead of the simple two-liners? I have only one
> request: Please don't change BuildAndTestWebKit2Factory to only EFL friendly.
> I still think --no-webkit1 shouldn't be passed automatically to a slave typed
> BuildAndTestWebKit2, because --no-webkit1 is an EFL only willing for the bots.

My first patch on this bug is very similar to what you are suggesting. I tried first to use builddir but I since you don't have it at the CompileWebkit class, I had to use fullPlatform. It was becoming too hackish, that's why I moved to the approach on the second patch.


What about renaming BuildAndTestWebKit2Factory to BuildAndTestWebKit2OnlyFactory. We make it explicit and with no side effects since EFL will be the only customer of this class.

[Thiago Marcos P. Santos]

Created attachment 179281 [details]
Patch

[Thiago Marcos P. Santos]

(In reply to comment #17)
> Created an attachment (id=179281) [details]
> Patch

Ossy, I added a patch implementing what I've just described for your evaluation.

[Csaba Osztrogonác]

Comment on attachment 179281 [details]
Patch

LGTM, r=me.

[WebKit Review Bot]

Comment on attachment 179281 [details]
Patch

Clearing flags on attachment: 179281

Committed r137612: <http://trac.webkit.org/changeset/137612>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

I rebooted the master.  Also debugged why it wasn't auto-restarted and believe we have fixed that problem.  Future changes should automatically see a buildmaster restart.