[webkit-reviews] review granted: [Bug 168531] Build ImageDiff with host SDK : [Attachment 306840] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 168531: Build ImageDiff with host SDK
https://bugs.webkit.org/show_bug.cgi?id=168531

Attachment 306840: Patch

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




--- Comment #33 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 306840
  --> https://bugs.webkit.org/attachment.cgi?id=306840
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!