Bug 107863 - Makefiles should work for arbitrary SDKs and architectures on Apple ports
Summary: Makefiles should work for arbitrary SDKs and architectures on Apple ports
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Other
: P2 Enhancement
Assignee: David Farler
URL:
Keywords:
Depends on:
Blocks: 108978
  Show dependency treegraph
 
Reported: 2013-01-24 14:09 PST by David Farler
Modified: 2013-02-07 16:57 PST (History)
4 users (show)

See Also:


Attachments
Patch (8.69 KB, patch)
2013-01-24 19:10 PST, David Farler
no flags Details | Formatted Diff | Diff
Patch (10.99 KB, patch)
2013-01-25 16:09 PST, David Farler
no flags Details | Formatted Diff | Diff
Patch (5.02 KB, patch)
2013-02-06 11:37 PST, David Farler
no flags Details | Formatted Diff | Diff
Patch (5.03 KB, patch)
2013-02-06 12:38 PST, David Farler
no flags Details | Formatted Diff | Diff
Patch (5.15 KB, patch)
2013-02-06 13:21 PST, David Farler
no flags Details | Formatted Diff | Diff
Patch (5.03 KB, patch)
2013-02-06 13:35 PST, David Farler
no flags Details | Formatted Diff | Diff
Patch (5.03 KB, patch)
2013-02-07 12:27 PST, David Farler
no flags Details | Formatted Diff | Diff
Patch (5.02 KB, patch)
2013-02-07 16:25 PST, David Farler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Farler 2013-01-24 14:09:15 PST
Makefiles should work as expected for all SDKs on Apple ports, not just Mac OS X.
Comment 1 David Farler 2013-01-24 19:10:25 PST
Created attachment 184639 [details]
Patch
Comment 2 David Kilzer (:ddkilzer) 2013-01-25 10:04:02 PST
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.
Comment 3 David Farler 2013-01-25 12:27:59 PST
(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.
Comment 4 David Farler 2013-01-25 16:09:59 PST
Created attachment 184833 [details]
Patch
Comment 5 David Kilzer (:ddkilzer) 2013-01-26 08:27:05 PST
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/.
Comment 6 WebKit Review Bot 2013-01-26 08:36:25 PST
Comment on attachment 184833 [details]
Patch

Clearing flags on attachment: 184833

Committed r140912: <http://trac.webkit.org/changeset/140912>
Comment 7 WebKit Review Bot 2013-01-26 08:36:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Mark Rowe (bdash) 2013-01-27 01:25:28 PST
This broke a couple of aspects of the existing behavior. I wrote up bug 108028.
Comment 9 David Farler 2013-01-28 15:35:45 PST
Reopening - patch reverted in 8da3ed12eecff30e9b8da2412926bf990e802803 (r140939)
Comment 10 David Farler 2013-02-06 11:37:39 PST
Created attachment 186885 [details]
Patch
Comment 11 David Farler 2013-02-06 11:55:04 PST
Comment on attachment 186885 [details]
Patch

For review.
Comment 12 David Kilzer (:ddkilzer) 2013-02-06 12:25:36 PST
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?
Comment 13 David Farler 2013-02-06 12:36:02 PST
(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.
Comment 14 David Farler 2013-02-06 12:38:11 PST
Created attachment 186897 [details]
Patch
Comment 15 David Farler 2013-02-06 13:21:17 PST
Created attachment 186905 [details]
Patch
Comment 16 David Farler 2013-02-06 13:21:37 PST
Comment on attachment 186905 [details]
Patch

Removed eval, only allow SDKROOT, ARCHS, ONLY_ACTIVE_ARCH outside of ARGS
Comment 17 David Farler 2013-02-06 13:35:35 PST
Created attachment 186909 [details]
Patch
Comment 18 Mark Rowe (bdash) 2013-02-07 12:15:03 PST
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?
Comment 19 David Farler 2013-02-07 12:27:32 PST
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.
Comment 20 Mark Rowe (bdash) 2013-02-07 14:51:33 PST
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"
Comment 21 David Farler 2013-02-07 16:25:31 PST
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.
Comment 22 WebKit Review Bot 2013-02-07 16:57:39 PST
Comment on attachment 187196 [details]
Patch

Clearing flags on attachment: 187196

Committed r142207: <http://trac.webkit.org/changeset/142207>
Comment 23 WebKit Review Bot 2013-02-07 16:57:44 PST
All reviewed patches have been landed.  Closing bug.