Bug 100844 – [Qt] Reduce the number of clean builds

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

[Csaba Osztrogonác]

After http://trac.webkit.org/changeset/131445 build-webkit always trigger 
clean build if any Qt project file changed after the latest successful build.

I checked the history on one of our buildbot. There were 622 builds in this
2 weeks and 60 was clean build because of this change. I think clean build in
~10% of cases is too high and unnecessary.

I suggest we should do clean build if any project file changed _and_ incremental build failed.
Patch is coming soon.

[Csaba Osztrogonác]

Created attachment 171627 [details]
Patch

[Csaba Osztrogonác]

I checked the "config change triggers clean build" case too, it was only 2 
clean build because of this reason during 622 builds. And it is still needed,
because a config change can easily cause false positive build and/or test fail.

[Simon Hausmann]

Comment on attachment 171627 [details]
Patch

LGTM, but it would be good to get an opinion from Tor Arne on this, too.

[Tor Arne Vestbø]

Comment on attachment 171627 [details]
Patch

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

This opens us up to false positive incremental builds. If you're fine with that, then let's try it. A few nitpicks:

> Tools/Scripts/webkitdirs.pm:2337
> +                print "Change to $_ detected, maybe clean build needed.\n";

"clean build may be needed"

> Tools/Scripts/webkitdirs.pm:2390
> +        $result = system "$command wipeclean incremental";

What's the behaviour of make when passing multiple targets? Is there a chance it will try to parallelise them, or is this equivalent to make foo && make bar?

[Csaba Osztrogonác]

Created attachment 171646 [details]
Patch

[Csaba Osztrogonác]

(In reply to comment #4)
> (From update of attachment 171627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171627&action=review
> 
> This opens us up to false positive incremental builds. If you're fine with that, then let's try it. A few nitpicks:
> 
> > Tools/Scripts/webkitdirs.pm:2337
> > +                print "Change to $_ detected, maybe clean build needed.\n";
> 
> "clean build may be needed"

Fixed.
 
> > Tools/Scripts/webkitdirs.pm:2390
> > +        $result = system "$command wipeclean incremental";
> 
> What's the behaviour of make when passing multiple targets? Is there a chance it will try to parallelise them, or is this equivalent to make foo && make bar?

Good point, it can be parallelizable. Fixed.

[Csaba Osztrogonác]

(In reply to comment #4)
> (From update of attachment 171627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171627&action=review
> 
> This opens us up to false positive incremental builds. If you're fine with that, then let's try it.

You're right. But as far as I remember there were much more 
false negative incremental build issue that false positive.
On the whole I'm fine with it, let's see what happens. ;-)

[Tor Arne Vestbø]

Comment on attachment 171646 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:2349
> +    if ($needsIncrementalBuild && !$maybeNeedsCleanBuild) {

Doesn't this mean that when a file changes, we won't do an incremental build, only a regular build? I think we can leave it like it was.

> Tools/Scripts/webkitdirs.pm:2388
> +        print "Calling '$command wipeclean' in " . $dir . "\n\n";

And then you need to reconstruct command here, since it will have "incremental" in it at this point.

[Csaba Osztrogonác]

Created attachment 172579 [details]
Patch

[Csaba Osztrogonác]

(In reply to comment #8)
> (From update of attachment 171646 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171646&action=review
> 
> > Tools/Scripts/webkitdirs.pm:2349
> > +    if ($needsIncrementalBuild && !$maybeNeedsCleanBuild) {
> 
> Doesn't this mean that when a file changes, we won't do an incremental build, only a regular build? I think we can leave it like it was.

Good point. Fixed.
 
> > Tools/Scripts/webkitdirs.pm:2388
> > +        print "Calling '$command wipeclean' in " . $dir . "\n\n";
> 
> And then you need to reconstruct command here, since it will have "incremental" in it at this point.

It is simpler using $make instead of $command, because 
we don't need incremental option after the wipeclean.

[Csaba Osztrogonác]

Comment on attachment 172579 [details]
Patch

ping?

[Csaba Osztrogonác]

Comment on attachment 172579 [details]
Patch

Clearing flags on attachment: 172579

Committed r133873: <http://trac.webkit.org/changeset/133873>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.