[webkit-reviews] review denied: [Bug 107863] Makefiles should work for arbitrary SDKs and architectures on Apple ports : [Attachment 184639] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 25 10:02:06 PST 2013


David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has denied David Farler
<dfarler at apple.com>'s request for review:
Bug 107863: Makefiles should work for arbitrary SDKs and architectures on Apple
ports
https://bugs.webkit.org/show_bug.cgi?id=107863

Attachment 184639: Patch
https://bugs.webkit.org/attachment.cgi?id=184639&action=review

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=184639&action=review


r- only for $(XCODE_OPTIONS) not being replaced in the "clean" target.

The other comments can be addressed later in follow-up patches.

> Tools/Scripts/webkitdirs.pm:323
> +	   if ($opt =~ /^--arch(itecture)?$/) {

Should we support --arch=i386 or --architecture=i386 as well?  I guess the code
isn't consistent in doing that.

Don't change this for this patch, but I'd like to see argument parsing
extracted into an internal method so that both styles of arguments are
supported:

--foo bar
--foo=bar

> Tools/Scripts/webkitdirs.pm:338
> +	   $architecture = join(' ', @explicitArchs) if @explicitArchs;

Not for this patch, but I think $architecture should be renamed to
$architectures (or changed to @architectures) in the future since it can hold
more than one now.

> Tools/Scripts/webkitdirs.pm:444
> +	   if ($opt =~ /^--sdk$/i) {
> +	       splice(@ARGV, $i, 1);
> +	       $xcodeSDK = splice(@ARGV, $i, 1);

Ditto above for making --sdk=iphonesimulator work.

> Tools/Scripts/webkitdirs.pm:590
> -    return (@baseProductDirOption, "-configuration", $configuration,
"ARCHS=$architecture", argumentsForXcode());
> +    determineXcodeSDK();
> +    my @archFlags = map { ('-arch', $_) } split(/ /, $architecture);
> +    return (@baseProductDirOption, "-configuration", $configuration, "-sdk",
$xcodeSDK, @archFlags, argumentsForXcode());

Any particular reason you chose to use individual -arch flags instead of a
single ARCHS="$architecture" flag here?  (No need to change this; I'm just
curious.)

Also, we could be helpful here and pass ONLY_ACTIVE_ARCH=NO when there is more
than one architecture defined (since the Debug/Release builds default to
ONLY_ACTIVE_ARCH=YES).	This is fine for a follow-up patch.

> Makefile:8
> +	./WebKitSystemInterface \

This directory will not stay here forever; we will switch to a
WebKitLibaries/libWebKitSystemInterfaceIOS_version_arch.a model at some point
in the future.	It's fine for now, though.

> Makefile.shared:45
> -	( xcodebuild $(OTHER_OPTIONS) -alltargets clean $(XCODE_OPTIONS) |
$(OUTPUT_FILTER) && exit $${PIPESTATUS[0]} )
> +	xcodebuild $(OTHER_OPTIONS) -alltargets clean $(XCODE_OPTIONS) |
$(OUTPUT_FILTER) && exit $${PIPESTATUS[0]}

Any reason why $(XCODE_OPTIONS) wasn't replaced here?  Seems like it should
since XCODE_OPTIONS isn't defined anymore.


More information about the webkit-reviews mailing list