Bug 183422 – webkitpy: Build ImageDiff if it is missing

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

run-webkit-tests should always build ImageDiff if it is requested but does not exist.

[Jonathan Bedard]

Created attachment 335235 [details]
Patch

[Jonathan Bedard]

Created attachment 335242 [details]
Patch

[Daniel Bates]

Comment on attachment 335242 [details]
Patch

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

> Tools/ChangeLog:12
> +        ImageDiff is built with a different SDK than the rest of the WebKit
> +        stack, and this frequently causes infrastructure failures where ImageDiff
> +        is missing on testers. To address this, we should automatically build
> +        ImageDiff if it is missing.
> +

I disagree with this premise. Testers should not be building software because it adds complexity to the build system to ensure that testers build with the same source revisions and toolchain among other issues and breaks with people's intuition on the roles of builders and testers.

[Jonathan Bedard]

(In reply to Daniel Bates from comment #3)
> Comment on attachment 335242 [details]
> Patch
> 
> ...
> 
> I disagree with this premise. Testers should not be building software
> because it adds complexity to the build system to ensure that testers build
> with the same source revisions and toolchain among other issues and breaks
> with people's intuition on the roles of builders and testers.

First, this definitely does not increase complexity. The existing system requires storing ImageDiff in a parallel table to the rest of the built products indexed by the Mac SDK instead of the SDK used to build WebKit. This has been a pretty frequent source of problems for not just the testers, but engineers using archived builds as well.

Second, ImageDiff is a tool used for testing, not a binary to be tested. Given that ImageDiff is called by webkitpy and has no dependencies on the rest of the WebKit stack (unlike the test runners). I would hold that the revision of ImageDiff should be aligned with the revision of webkitpy used, not the revision of WebKit. This would be more consistent with building on the testers.

Lastly, your intuition point. To address this, I would like to note that the alternative approach to this problem is converting ImageDiff to an interpreted language. This is certainly possible, but I'm not sure we have the engineering bandwidth to do so in the near future. I've looked into using Python to replace ImageDiff, but Python does such an abysmal job at optimizing loops that we would almost certainly need to statically compile a few functions to get ImageDiff to be performant. This would bring us back to building on the testers anyways, although perhaps obfuscated behind a Python abstraction.

Ultimately, the hope is that people don't notice this change. ImageDiff only takes a few seconds to build, and rarely changes. If we still want to retain the ability to forbid all building when using run-webkit-tests (including ImageDiff), I can remove the changes in Port.check_build() the current patch contains.

[Jonathan Bedard]

Created attachment 382788 [details]
Patch

[Jonathan Bedard]

We have some pretty compelling reasons to revive this patch due to some binary compatibility issues with simulators and limitations on the OSes we can use in our builders.

[Alexey Proskuryakov]

Comment on attachment 382788 [details]
Patch

OK. Still not excited about this, but these issues have taken too much time.

We should follow up with not building ImageDiff as part of build archive, I think.

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #7)
> Comment on attachment 382788 [details]
> Patch
> 
> OK. Still not excited about this, but these issues have taken too much time.
> 
> We should follow up with not building ImageDiff as part of build archive, I
> think.

I feel like this is the best of our worst options.

This will be better for local testing too.

[WebKit Commit Bot]

Comment on attachment 382788 [details]
Patch

Clearing flags on attachment: 382788

Committed r252031: <https://trac.webkit.org/changeset/252031>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56889868>

[Carlos Garcia Campos]

Comment on attachment 382788 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:287
> -        image_diff_path = self._path_to_image_diff()
> +        image_diff_path = self._build_path('ImageDiff')

This has broken the layout tests run in non apple ports, because it assumes ImageDiff binary is in build_path, while in CMake based ports it's in build_path/bin for example. That's what _path_to_image_diff() returns for those ports.

[Jonathan Bedard]

Committed r252058: <https://trac.webkit.org/changeset/252058>