Bug 168531 - Build ImageDiff with host SDK
Summary: Build ImageDiff with host SDK
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks: 168944
  Show dependency treegraph
 
Reported: 2017-02-17 13:12 PST by Jonathan Bedard
Modified: 2017-04-13 13:08 PDT (History)
10 users (show)

See Also:


Attachments
Patch (11.99 KB, patch)
2017-02-17 13:16 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (13.18 KB, patch)
2017-02-17 15:49 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (68.38 KB, patch)
2017-02-23 11:57 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (65.56 KB, patch)
2017-02-23 13:00 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-elcapitan (1.80 MB, application/zip)
2017-02-23 14:19 PST, Build Bot
no flags Details
Patch (65.76 KB, patch)
2017-02-24 08:25 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (10.68 KB, patch)
2017-02-27 16:08 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (10.35 KB, patch)
2017-02-28 09:01 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (10.54 KB, patch)
2017-02-28 13:50 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (10.81 KB, patch)
2017-02-28 14:49 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (10.92 KB, patch)
2017-03-01 08:45 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (11.13 KB, patch)
2017-03-21 13:28 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (11.18 KB, patch)
2017-04-03 13:00 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (11.20 KB, patch)
2017-04-11 12:29 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (11.10 KB, patch)
2017-04-13 11:30 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-02-17 13:12:26 PST
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.
Comment 1 Jonathan Bedard 2017-02-17 13:16:54 PST
Created attachment 301976 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2017-02-17 13:17:26 PST
<rdar://problem/30583092>
Comment 3 Jonathan Bedard 2017-02-17 13:18:11 PST
Preemptively cq-ing this particular patch since it will likely break our archives.
Comment 4 Alexey Proskuryakov 2017-02-17 14:39:09 PST
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.
Comment 5 Jonathan Bedard 2017-02-17 15:49:21 PST
Created attachment 302001 [details]
Patch
Comment 6 Jonathan Bedard 2017-02-17 15:53:54 PST
(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.
Comment 7 Jonathan Bedard 2017-02-22 09:47:22 PST
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.
Comment 8 Jonathan Bedard 2017-02-23 11:57:55 PST
Created attachment 302551 [details]
Patch
Comment 9 Jonathan Bedard 2017-02-23 11:59:49 PST
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.
Comment 10 Jonathan Bedard 2017-02-23 13:00:25 PST
Created attachment 302564 [details]
Patch
Comment 11 WebKit Commit Bot 2017-02-23 13:02:06 PST
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.
Comment 12 Jonathan Bedard 2017-02-23 13:16:19 PST
(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.
Comment 13 Build Bot 2017-02-23 14:19:53 PST
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
Comment 14 Build Bot 2017-02-23 14:19:56 PST
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
Comment 15 Jonathan Bedard 2017-02-24 08:25:28 PST
Created attachment 302669 [details]
Patch
Comment 16 WebKit Commit Bot 2017-02-24 12:18:50 PST
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.
Comment 17 Alexey Proskuryakov 2017-02-27 09:16:12 PST
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.
Comment 18 Simon Fraser (smfr) 2017-02-27 10:53:02 PST
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'.
Comment 19 Jonathan Bedard 2017-02-27 13:08:20 PST
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.
Comment 20 Jonathan Bedard 2017-02-27 15:13:53 PST
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.
Comment 21 Jonathan Bedard 2017-02-27 16:08:58 PST
Created attachment 302883 [details]
Patch
Comment 22 Jonathan Bedard 2017-02-27 16:25:55 PST
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).
Comment 23 Jonathan Bedard 2017-02-28 09:01:17 PST
Created attachment 302937 [details]
Patch
Comment 24 Jonathan Bedard 2017-02-28 13:50:13 PST
Created attachment 302980 [details]
Patch
Comment 25 Jonathan Bedard 2017-02-28 13:51:54 PST
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.
Comment 26 Jonathan Bedard 2017-02-28 14:49:21 PST
Created attachment 302990 [details]
Patch
Comment 27 Jonathan Bedard 2017-02-28 14:55:46 PST
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.
Comment 28 Jonathan Bedard 2017-03-01 08:45:01 PST
Created attachment 303067 [details]
Patch
Comment 29 Jonathan Bedard 2017-03-01 09:43:51 PST
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
Comment 30 Jonathan Bedard 2017-03-21 13:28:45 PDT
Created attachment 305020 [details]
Patch
Comment 31 Jonathan Bedard 2017-04-03 13:00:36 PDT
Created attachment 306096 [details]
Patch
Comment 32 Jonathan Bedard 2017-04-11 12:29:30 PDT
Created attachment 306840 [details]
Patch
Comment 33 David Kilzer (:ddkilzer) 2017-04-13 10:28:09 PDT
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!
Comment 34 Jonathan Bedard 2017-04-13 11:30:39 PDT
Created attachment 307000 [details]
Patch
Comment 35 WebKit Commit Bot 2017-04-13 13:08:39 PDT
Comment on attachment 307000 [details]
Patch

Clearing flags on attachment: 307000

Committed r215334: <http://trac.webkit.org/changeset/215334>
Comment 36 WebKit Commit Bot 2017-04-13 13:08:41 PDT
All reviewed patches have been landed.  Closing bug.