Makefiles should work for arbitrary SDKs and architectures on Apple ports

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Farler]

Makefiles should work as expected for all SDKs on Apple ports, not just Mac OS X.

[David Farler]

Created attachment 184639 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 184639 [details]
Patch

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.

[David Farler]

(In reply to comment #2)
> (From update of attachment 184639 [details])
> 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

Yeah, I was pondering this inconsistency throughout the code. While this module and scripts that touch it are still in Perl, I'd like to consider developing a consistent CLI with something like Getopt::Long. It supports handling abbreviations and ambiguities. It also supports --option arg and --option=arg formats. I might file a follow-up bug for switching to Getopt::Long before fielding https://bugs.webkit.org/show_bug.cgi?id=107188.

> 
> > 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.)
>

I was mostly just matching the other options' usage. I think it makes sense to use the command line switches whenever possible and let Xcode handle naming/setting of the variables. I only left parsing of ARCHS= to not break anybody currently using that method.
 
> 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.
> 

I agree! I seem to type that every single time I want to build more than once slice. I'll bring it in via a second 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.

This was a paste mistake - fixing.

[David Farler]

Created attachment 184833 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 184833 [details]
Patch

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

r=me

> Source/Makefile:3
> +IOS_DONT_BUILD = WebKit2

Nit: IOS_DO_NOT_BUILD would probably read a bit better.

> Tools/Makefile:4
> +IOS_DONT_BUILD = WebKitTestRunner MiniBrowser TestWebKitAPI
> +IPHONEOS_DONT_BUILD = DumpRenderTree

Ditto on s/DONT/DO_NOT/.

[WebKit Review Bot]

Comment on attachment 184833 [details]
Patch

Clearing flags on attachment: 184833

Committed r140912: <http://trac.webkit.org/changeset/140912>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Rowe (bdash)]

This broke a couple of aspects of the existing behavior. I wrote up bug 108028.

[David Farler]

Reopening - patch reverted in 8da3ed12eecff30e9b8da2412926bf990e802803 (r140939)

[David Farler]

Created attachment 186885 [details]
Patch

[David Farler]

Comment on attachment 186885 [details]
Patch

For review.

[David Kilzer (:ddkilzer)]

Comment on attachment 186885 [details]
Patch

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

Overall looks good.  I'd like Mark Rowe to test the WebKit nightly build with these changes to make sure they work.

> Makefile.shared:3
> +XCODE_OPTIONS_ARGS=

Can we come up with a better name for these?  The --device and --simulator switches are not xcodebuild options; they're build-webkit arguments.  Maybe WEBKITDIRS_OPTIONS?

[David Farler]

(In reply to comment #12)
> (From update of attachment 186885 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186885&action=review
> 
> Overall looks good.  I'd like Mark Rowe to test the WebKit nightly build with these changes to make sure they work.
> 
> > Makefile.shared:3
> > +XCODE_OPTIONS_ARGS=
> 
> Can we come up with a better name for these?  The --device and --simulator switches are not xcodebuild options; they're build-webkit arguments.  Maybe WEBKITDIRS_OPTIONS?

Yeah that's a good point. I'd rather call it BUILD_WEBKIT_OPTIONS though because someday (I can dream) I'd like to rename webkitdirs.pm to reflect its general purposes.

[David Farler]

Created attachment 186897 [details]
Patch

[David Farler]

Created attachment 186905 [details]
Patch

[David Farler]

Comment on attachment 186905 [details]
Patch

Removed eval, only allow SDKROOT, ARCHS, ONLY_ACTIVE_ARCH outside of ARGS

[David Farler]

Created attachment 186909 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 186909 [details]
Patch

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

> Makefile.shared:14
> +	XCODE_OPTIONS += " ONLY_ACTIVE_ARCH=NO"

Ignoring the value of ONLY_ACTIVE_ARCH and always passing NO doesn't seem correct.

Maybe this would be easier to use if we just set ONLY_ACTIVE_ARCH=NO whenever ARCHS is overridden rather than requiring that it be passed separately?

[David Farler]

Created attachment 187147 [details]
Patch

Yeah, good point. I think I meant to set it to the passed variable instead of just NO, but I think most of the time people will want ONLY_ACTIVE_ARCH=NO if they are explicitly setting ARCHS. I updated the patch.

[Mark Rowe (bdash)]

Comment on attachment 187147 [details]
Patch

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

This looks good to go. You should see if it's possible to remove the leading spaces as mentioned above. If it's not, let me know and I can cq+ this patch. If it is, upload a new patch and I'll r+ and cq+ it instead.

> Makefile.shared:11
> +	XCODE_OPTIONS += " ARCHS=$(ARCHS)"
> +	XCODE_OPTIONS += " ONLY_ACTIVE_ARCH=NO"

From looking at the build output with this patch applied, I don't think the leading space inside the double quotes is necessary:

> xcodebuild  `perl -I../../Tools/Scripts -Mwebkitdirs -e 'print XcodeOptionString()' -- `  " ARCHS=x86_64" " ONLY_ACTIVE_ARCH=NO"

[David Farler]

Created attachment 187196 [details]
Patch

Yep, I was able to remove the spaces but I had to enquote ARCHS to allow for using multiple space-delimited archs.

[WebKit Review Bot]

Comment on attachment 187196 [details]
Patch

Clearing flags on attachment: 187196

Committed r142207: <http://trac.webkit.org/changeset/142207>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.