Bug 168531 – Build ImageDiff with host SDK

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

Currently, ImageDiff is built with the target SDK, even for iOS Simulator and device.  ImageDiff should only built with the SDK of the host machine.

[Jonathan Bedard]

Created attachment 301976 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/30583092>

[Jonathan Bedard]

Preemptively cq-ing this particular patch since it will likely break our archives.

[Alexey Proskuryakov]

Comment on attachment 301976 [details]
Patch

iOS EWS is red.

ImageDiff is not standalone, so we'd need to build at least WTF with host SDK, if not more. Or to make it standalone, which seems preferable.

[Jonathan Bedard]

Created attachment 302001 [details]
Patch

[Jonathan Bedard]

(In reply to comment #4)
> Comment on attachment 301976 [details]
> Patch
> 
> iOS EWS is red.
> 
> ImageDiff is not standalone, so we'd need to build at least WTF with host
> SDK, if not more. Or to make it standalone, which seems preferable.

It's dependent on bmalloc and WTF, the updated patch should fix that issue.

I will look into making it a standalone executable, but I'm concerned that this may be a more difficult and less portable solution if we have to do anything like this in the future.

[Jonathan Bedard]

I have some basic metrics for compile time.  A full build of ImageDiff with no bmalloc or WTF already built takes about 15 seconds.  If ImageDiff is built without malloc and WTF dependencies, it takes about a second.  It seems that removing the RetainPtr dependency is going to be the best approach here.

[Jonathan Bedard]

Created attachment 302551 [details]
Patch

[Jonathan Bedard]

Comment on attachment 302551 [details]
Patch

I think this is the way we want to do this.

This approach makes building ImageDiff part of run-webkit-tests.  I suppose this also means we should stop archiving ImageDiff.

[Jonathan Bedard]

Created attachment 302564 [details]
Patch

[WebKit Commit Bot]

Attachment 302564 [details] did not pass style-queue:


ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:154:  Missing space before ( in for(  [whitespace/parens] [5]
ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:163:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

(In reply to comment #11)
> Attachment 302564 [details] did not pass style-queue:
> 
> 
> ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:33:  Alphabetical sorting problem. 
> [build/include_order] [4]
> ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:34:  Alphabetical sorting problem. 
> [build/include_order] [4]
> ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:154:  Missing space before ( in
> for(  [whitespace/parens] [5]
> ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:163:  An else should appear on the
> same line as the preceding }  [whitespace/newline] [4]
> Total errors found: 4 in 5 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

These were all style error present in the original code.  The alphabetical sorting issues should not be fixed, although the other style problems should be.

[Build Bot]

Comment on attachment 302564 [details]
Patch

Attachment 302564 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3181047

New failing tests:
media/modern-media-controls/volume-down-support/volume-down-support.html

[Build Bot]

Created attachment 302574 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Jonathan Bedard]

Created attachment 302669 [details]
Patch

[WebKit Commit Bot]

Attachment 302669 [details] did not pass style-queue:


ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/ImageDiff/mac/ImageDiff.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 302669 [details]
Patch

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

> Tools/ChangeLog:16
> +        (min): Use instead of WTF min.
> +        (max): Use instead of WTF max.

Can we use std::min and std::max?

> Tools/ImageDiff/ImageDiff.xcodeproj/project.pbxproj:184
> +				MACOSX_DEPLOYMENT_TARGET = 10.12;

Hmm.

[Simon Fraser (smfr)]

Comment on attachment 302669 [details]
Patch

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

> Tools/ImageDiff/mac/ImageDiff.cpp:71
> +template<typename T> inline T min(T var1, T var2)
> +{
> +    if (var1 < var2)
> +        return var1;
> +    return var2;
> +}
> +
> +template<typename T> inline T max(T var1, T var2)
> +{
> +    if (var1 > var2)
> +        return var1;
> +    return var2;
> +}

Why can't you use std::min/max?

> Tools/ImageDiff/mac/ImageDiff.cpp:73
> +static CGImageRef createImageFromStdin(int bytesRemaining)

Stdin or StdIn?

> Tools/ImageDiff/mac/ImageDiff.cpp:85
> +    CGImageRef ret = CGImageCreateWithPNGDataProvider(dataProvider, 0, false, kCGRenderingIntentDefault);

We usually call this "result'.

[Jonathan Bedard]

Comment on attachment 302669 [details]
Patch

At the moment, this patch would break archives.  Preparing a new one which will build ImageDiff with the host and target SDK, but not use the host ImageDiff.

[Jonathan Bedard]

Ok, so, to preserve the sanity of this change, I'm going to split it up.

This bug will track building, but not using ImageDiff with the host SDK.  Other bugs will track archiving the host-built ImageDiff, making ImageDiff a stand-alone project and using the host-build ImageDiff.  A patch which does all of these things changes too much at one time.

The incoming patch will look very similar to attachment 20170217154642.

[Jonathan Bedard]

Created attachment 302883 [details]
Patch

[Jonathan Bedard]

The patch 302883 causes ImageDiff to build for both the host and the target.  Next, ImageDiff will be archived and packaged for the host and the target (bug 168944).  Then, the Python scripts will switch to using the ImageDiff built for the host (bug 168945).  Then, we will stop archiving and packaging ImageDiff for the target (bug 168946).  Lastly, we will make ImageDiff a stand-alone application (bug 168939).

[Jonathan Bedard]

Created attachment 302937 [details]
Patch

[Jonathan Bedard]

Created attachment 302980 [details]
Patch

[Jonathan Bedard]

Comment on attachment 302980 [details]
Patch

cq- for the moment.  It contains the Mac 32-bit build fix in build-webkit, but not in the Makefiles.

[Jonathan Bedard]

Created attachment 302990 [details]
Patch

[Jonathan Bedard]

Attachment 302990 [details] shouldn't break any builds.  This patch has mostly been tested locally, although some of the port-specific Makefile bits are still being locally tested, this will take some time and I will update this bug once those tests are complete.

[Jonathan Bedard]

Created attachment 303067 [details]
Patch

[Jonathan Bedard]

Comment on attachment 303067 [details]
Patch

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

> Tools/ImageDiff/Makefile:15
> +	$(SCRIPTS_PATH)/build-imagediff --clean

--clean => clean

[Jonathan Bedard]

Created attachment 305020 [details]
Patch

[Jonathan Bedard]

Created attachment 306096 [details]
Patch

[Jonathan Bedard]

Created attachment 306840 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 306840 [details]
Patch

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

r=me

> Tools/ChangeLog:8
> +        ImageDiff should be built and ran with the host SDK, not the target SDK.

Typo:  ran => run

> Tools/Scripts/build-webkit:302
> +        my @command = (getcwd() . "/Tools/Scripts/build-imagediff");

I prefer to use File::Spec->catfile(getcwd(), "Tools/Scripts/build-imagediff") in these cases, but making this change doesn't block landing.

> Tools/Scripts/webkitdirs.pm:459
> +    my @extract = ('--device', '--efl', '--gtk', '--ios', '--platform', '--sdk', '--simulator', '--wincairo', 'SDKROOT', 'ARCHS', "'ARCHS", '"ARCHS');

Do the single/double quotes around ARCHS really survive here after being processed by the shell?  Just wondering if the last two items can be removed.

> Tools/Scripts/webkitdirs.pm:465
> +            if (length($line) >= length($_) && substr($line, 0, length($_)) eq $_
> +                && index($line, 'i386') == -1 && index($line, 'x86_64') == -1) {

This will break if devices ever existed that used Intel chips but didn't run macOS.

Maybe make the subroutine name more Mac-specific:  extractNonMacOSHostConfiguration()?

> Tools/Scripts/webkitperl/webkitdirs_unittest/extractNonHostConfiguration.pl:1
> +#!/usr/bin/perl -w

Nice!  Love the unit tests!

[Jonathan Bedard]

Created attachment 307000 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 307000 [details]
Patch

Clearing flags on attachment: 307000

Committed r215334: <http://trac.webkit.org/changeset/215334>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.