Bug 171788 - REGRESSION: ImageDiff not building with make
Summary: REGRESSION: ImageDiff not building with make
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-07 00:15 PDT by Alexey Proskuryakov
Modified: 2017-05-09 09:24 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.48 KB, patch)
2017-05-08 10:22 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (1.79 KB, patch)
2017-05-08 15:12 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (1008 bytes, patch)
2017-05-08 16:10 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 Alexey Proskuryakov 2017-05-07 00:15:02 PDT
I built WebKit with make, and tried to run tests:

ap$ run-webkit-tests --no-build
Using port 'mac-sierra-wk2'
Test configuration: <sierra, x86_64, debug>
...

ImageDiff was not found at /Users/ap/Safari/OpenSource/WebKitBuild/Debug/ImageDiff
Build check failed
Comment 1 Alexey Proskuryakov 2017-05-07 00:16:44 PDT
I get this message even when not passing --no-build, which is a separate issue, but may be easiest to address at the same time.

ImageDiff was not found at /Users/ap/Safari/OpenSource/WebKitBuild/Debug/ImageDiff
Comment 2 Jonathan Bedard 2017-05-08 08:07:38 PDT
(In reply to Alexey Proskuryakov from comment #1)
> I get this message even when not passing --no-build, which is a separate
> issue, but may be easiest to address at the same time.
> 
> ImageDiff was not found at
> /Users/ap/Safari/OpenSource/WebKitBuild/Debug/ImageDiff

Is this an iOS Simulator build?  Or Mac?

It's definitely looking in the correct spot, it's just failing to actually build ImageDiff.
Comment 3 Jonathan Bedard 2017-05-08 10:22:27 PDT
Created attachment 309375 [details]
Patch
Comment 4 Jonathan Bedard 2017-05-08 10:23:28 PDT
attachment 309375 [details] should do the trick.

Actually, no Apple platforms were building the correct ImageDiff through Make.
Comment 5 Alexey Proskuryakov 2017-05-08 10:58:39 PDT
Comment on attachment 309375 [details]
Patch

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

> Tools/ChangeLog:8
> +        * ImageDiff/Makefile: Use Mac SDK when building for iOS Simulator or iOS device.

Please explain that this is fixing a separate issue that you found while looking into this.
Comment 6 Jonathan Bedard 2017-05-08 15:12:41 PDT
Created attachment 309420 [details]
Patch
Comment 7 Alexey Proskuryakov 2017-05-08 15:32:24 PDT
Comment on attachment 309420 [details]
Patch

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

> Tools/ChangeLog:3
> +        REGRESSION: ImageDiff not building with make

I'm not sure if this is even a bug. In the past, we explicitly disabled LayoutTestRelay building with DO_NOT_BUILD_LAYOUT_TEST_RELAY variable (it's actually still in place in trunk). The rationale was that we wanted to maintain symmetry with production builds.

But then I'm not sure about how testers would work. Really confused about the overall plan.

> Tools/ChangeLog:14
> +        * ImageDiff/Makefile: Use Mac SDK when building for iOS Simulator or iOS device.

It's probably best to file a new bug about this, as it really seems like a separate issue.

> Tools/ImageDiff/Makefile:5
> +	OVERRIDE_SDKROOT = macosx
> +	OVERRIDE_ARCHS = "i386 x86_64"

Here is what we used to have in LayoutTestRelay/Makefile, added in <https://trac.webkit.org/r192106>:

OVERRIDE_ARCHS = default
OVERRIDE_SDKROOT = default
Comment 8 Jonathan Bedard 2017-05-08 15:57:02 PDT
(In reply to Alexey Proskuryakov from comment #7)
> Comment on attachment 309420 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309420&action=review
> 
> > Tools/ChangeLog:3
> > +        REGRESSION: ImageDiff not building with make
> 
> I'm not sure if this is even a bug. In the past, we explicitly disabled
> LayoutTestRelay building with DO_NOT_BUILD_LAYOUT_TEST_RELAY variable (it's
> actually still in place in trunk). The rationale was that we wanted to
> maintain symmetry with production builds.

I think the difference with LayoutTestRelay and ImageDiff is that every platform needs an ImageDiff, it's just not always the one built with the target SDK.
 
> It's probably best to file a new bug about this, as it really seems like a
> separate issue.

I can file a separate bug about the iOS Simulator/Device case
Comment 9 Jonathan Bedard 2017-05-08 16:06:33 PDT
<https://bugs.webkit.org/show_bug.cgi?id=171835> is tracking the iOS device and iOS simulator bits.
Comment 10 Jonathan Bedard 2017-05-08 16:10:18 PDT
Created attachment 309433 [details]
Patch
Comment 11 Radar WebKit Bug Importer 2017-05-08 16:18:45 PDT
<rdar://problem/32062947>
Comment 12 Alex Christensen 2017-05-09 07:25:41 PDT
https://bugs.webkit.org/show_bug.cgi?id=171847 fixes a similar problem on Linux and Windows.
Comment 13 WebKit Commit Bot 2017-05-09 07:53:05 PDT
Comment on attachment 309433 [details]
Patch

Clearing flags on attachment: 309433

Committed r216505: <http://trac.webkit.org/changeset/216505>
Comment 14 WebKit Commit Bot 2017-05-09 07:53:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Alexey Proskuryakov 2017-05-09 09:24:57 PDT
Talking about this more, Jonathan and I think that this should be conditional, to match production builds. It's not right that release/debug builds ImageDiff, but production does not.