Support target/host architecture with ninja iOS builds

Support target/host architecture with ninja iOS builds

When iOS builds support ninja and enable GYP_CROSSCOMPILE=1, ninja
will build breakpad, protoc and iossim using host arch instead
of the sub-out-ninja workaround.

TBR=mark@chromium.org
BUG=236517
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199827

[justincohen, 2013-05-03 23:16:07]

I'm not 100% sure how target/host is supposed to work, but this gets rid of the
sub-out-to-ninja problem (for ninja builds only, xcode still will sub out)

PTAL!

[stuartmorgan, 2013-05-06 11:56:35]

https://codereview.chromium.org/14597007/diff/1/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/14597007/diff/1/base/base.gypi#newcode700
base/base.gypi:700: ['exclude', 'ios'],
These two rules seem really dangerous. Seems like we should be replicating the
filename_rules rules.

(e.g., hypothetical files machine_info_win.cc and kiosk_utils.cc would fare
badly)

https://codereview.chromium.org/14597007/diff/1/breakpad/breakpad.gyp
File breakpad/breakpad.gyp (right):

https://codereview.chromium.org/14597007/diff/1/breakpad/breakpad.gyp#newcode209
breakpad/breakpad.gyp:209: 'toolsets': ['host','target'],
spaces after ,

https://codereview.chromium.org/14597007/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14597007/diff/1/build/common.gypi#newcode4036
build/common.gypi:4036: 'MACOSX_DEPLOYMENT_TARGET': "10.6",
This is now hard-coded in a bunch of places in this file. Maybe it should be a
shared ios/mac variable?

https://codereview.chromium.org/14597007/diff/1/testing/iossim/iossim.gyp
File testing/iossim/iossim.gyp (right):

https://codereview.chromium.org/14597007/diff/1/testing/iossim/iossim.gyp#new...
testing/iossim/iossim.gyp:54: }, {  # else, OS == "ios"
Fix comment.

https://codereview.chromium.org/14597007/diff/1/third_party/mach_override/mac...
File third_party/mach_override/mach_override.gyp (right):

https://codereview.chromium.org/14597007/diff/1/third_party/mach_override/mac...
third_party/mach_override/mach_override.gyp:9: ['OS=="mac" or OS=="ios"', {
Do we need a generator check here?

https://codereview.chromium.org/14597007/diff/1/third_party/protobuf/protobuf...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/14597007/diff/1/third_party/protobuf/protobuf...
third_party/protobuf/protobuf.gyp:276: }, {  # else, OS=="ios"
Fix comment

[justincohen, 2013-05-06 14:17:36]

darin: ptal third party changes

applied Stuarts changes.  PTAL

https://codereview.chromium.org/14597007/diff/1/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/14597007/diff/1/base/base.gypi#newcode700
base/base.gypi:700: ['exclude', 'ios'],
On 2013/05/06 11:56:35, stuartmorgan wrote:
> These two rules seem really dangerous. Seems like we should be replicating the
> filename_rules rules.
> 
> (e.g., hypothetical files machine_info_win.cc and kiosk_utils.cc would fare
> badly)

Done.

https://codereview.chromium.org/14597007/diff/1/breakpad/breakpad.gyp
File breakpad/breakpad.gyp (right):

https://codereview.chromium.org/14597007/diff/1/breakpad/breakpad.gyp#newcode209
breakpad/breakpad.gyp:209: 'toolsets': ['host','target'],
On 2013/05/06 11:56:35, stuartmorgan wrote:
> spaces after ,

Done.

https://codereview.chromium.org/14597007/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14597007/diff/1/build/common.gypi#newcode4036
build/common.gypi:4036: 'MACOSX_DEPLOYMENT_TARGET': "10.6",
On 2013/05/06 11:56:35, stuartmorgan wrote:
> This is now hard-coded in a bunch of places in this file. Maybe it should be a
> shared ios/mac variable?

Done.

https://codereview.chromium.org/14597007/diff/1/testing/iossim/iossim.gyp
File testing/iossim/iossim.gyp (right):

https://codereview.chromium.org/14597007/diff/1/testing/iossim/iossim.gyp#new...
testing/iossim/iossim.gyp:54: }, {  # else, OS == "ios"
On 2013/05/06 11:56:35, stuartmorgan wrote:
> Fix comment.

Done.

https://codereview.chromium.org/14597007/diff/1/third_party/mach_override/mac...
File third_party/mach_override/mach_override.gyp (right):

https://codereview.chromium.org/14597007/diff/1/third_party/mach_override/mac...
third_party/mach_override/mach_override.gyp:9: ['OS=="mac" or OS=="ios"', {
I don't think so -- the generator check should happen when we decide to include
this file.  I'll add it anyway just in case.

On 2013/05/06 11:56:35, stuartmorgan wrote:
> Do we need a generator check here?

https://codereview.chromium.org/14597007/diff/1/third_party/protobuf/protobuf...
File third_party/protobuf/protobuf.gyp (right):

https://codereview.chromium.org/14597007/diff/1/third_party/protobuf/protobuf...
third_party/protobuf/protobuf.gyp:276: }, {  # else, OS=="ios"
On 2013/05/06 11:56:35, stuartmorgan wrote:
> Fix comment

Done.

[Ryan Sleevi, 2013-05-06 21:38:58]

I've taken a look and everything 'seems' right from a GYP perspective (eg: right
execution timing, no assumptions/'undefined' behaviour reliances, etc). One
small style nit (as proof I reviewed it, I guess), but otherwise this looks good
to me.

https://codereview.chromium.org/14597007/diff/7002/breakpad/breakpad.gyp
File breakpad/breakpad.gyp (right):

https://codereview.chromium.org/14597007/diff/7002/breakpad/breakpad.gyp#newc...
breakpad/breakpad.gyp:211: [ 'OS=="mac"', {
nit: drop the leading whitespace between "[ '" (consistent with line 108 and
694)

[justincohen, 2013-05-06 23:26:22]

https://codereview.chromium.org/14597007/diff/7002/breakpad/breakpad.gyp
File breakpad/breakpad.gyp (right):

https://codereview.chromium.org/14597007/diff/7002/breakpad/breakpad.gyp#newc...
breakpad/breakpad.gyp:211: [ 'OS=="mac"', {
On 2013/05/06 21:38:59, Ryan Sleevi wrote:
> nit: drop the leading whitespace between "[ '" (consistent with line 108 and
> 694)

Done.

[stuartmorgan, 2013-05-07 13:14:46]

https://codereview.chromium.org/14597007/diff/19001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/14597007/diff/19001/base/base.gypi#newcode667
base/base.gypi:667: ['OS == "ios"', {
I think you want a !host on this too

https://codereview.chromium.org/14597007/diff/19001/base/base.gypi#newcode700
base/base.gypi:700: ['include', '^process_util\\.cc'],
... at which point you should be able to remove these three lines.

https://codereview.chromium.org/14597007/diff/19001/base/base.gypi#newcode701
base/base.gypi:701: ['include', '_(cocoa|mac)(_unittest)?\\.(h|cc|mm?)$'],
Can you stick a comment above this line that says "Copied filename_rules to
switch from iOS to Mac inclusions" or something to that effect?

https://codereview.chromium.org/14597007/diff/19001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14597007/diff/19001/build/common.gypi#newcode...
build/common.gypi:1336: 'conditions': [
Probably clearer if you just make the mac section a sibling of the mac || ios
section instead of nesting it like this. Or are there variable scoping issues
with that?

https://codereview.chromium.org/14597007/diff/19001/build/common.gypi#newcode...
build/common.gypi:1373: }],  # OS=="mac"
If you do have to keep the nested structure for variables, fix this comment, and
add a closing comment to the new Mac block.

[justincohen, 2013-05-07 14:50:57]

Thanks Stuart.  Please take another look!

https://codereview.chromium.org/14597007/diff/19001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/14597007/diff/19001/base/base.gypi#newcode667
base/base.gypi:667: ['OS == "ios"', {
On 2013/05/07 13:14:46, stuartmorgan wrote:
> I think you want a !host on this too

Done.

https://codereview.chromium.org/14597007/diff/19001/base/base.gypi#newcode700
base/base.gypi:700: ['include', '^process_util\\.cc'],
On 2013/05/07 13:14:46, stuartmorgan wrote:
> ... at which point you should be able to remove these three lines.

Done.

https://codereview.chromium.org/14597007/diff/19001/base/base.gypi#newcode701
base/base.gypi:701: ['include', '_(cocoa|mac)(_unittest)?\\.(h|cc|mm?)$'],
On 2013/05/07 13:14:46, stuartmorgan wrote:
> Can you stick a comment above this line that says "Copied filename_rules to
> switch from iOS to Mac inclusions" or something to that effect?
> 

Done.

https://codereview.chromium.org/14597007/diff/19001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14597007/diff/19001/build/common.gypi#newcode...
build/common.gypi:1336: 'conditions': [
Yeah, it ended up looking like this because of var scoping.

On 2013/05/07 13:14:46, stuartmorgan wrote:
> Probably clearer if you just make the mac section a sibling of the mac || ios
> section instead of nesting it like this. Or are there variable scoping issues
> with that?

https://codereview.chromium.org/14597007/diff/19001/build/common.gypi#newcode...
build/common.gypi:1373: }],  # OS=="mac"
On 2013/05/07 13:14:46, stuartmorgan wrote:
> If you do have to keep the nested structure for variables, fix this comment,
and
> add a closing comment to the new Mac block.

Done.

[justincohen, 2013-05-07 15:25:44]

brettw, can you look at the thirdparty gyp changes?  Thanks!

[brettw, 2013-05-07 20:59:14]

owners lgtm. I don't really understand what's happening here, so be sure to get
an lgtm from somebody who does.

[Nico, 2013-05-08 22:23:07]

I think this is a good direction. lgtm.

Once this is in, maybe the xcode generator could learn about host/target and
generate these gnarly shell-out-to-something-else targets automatically, so that
you don't need to have to keep them in sync in gyp manually.

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi#newcode...
build/common.gypi:1337: ['OS=="mac"', {
Could you just keep this in the enclosing conditions block instead of nesting it
here? Then this can remain less indented.

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi#newcode...
build/common.gypi:4034: },
is SDKROOT set to the right thing for ios?

[justincohen, 2013-05-08 22:49:56]

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi#newcode...
build/common.gypi:1337: ['OS=="mac"', {
I can't have them as siblings and depend on each other ('mac_sdk_path', etc)

Is there a better way?

On 2013/05/08 22:23:07, Nico wrote:
> Could you just keep this in the enclosing conditions block instead of nesting
it
> here? Then this can remain less indented.

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi#newcode...
build/common.gypi:4034: },
Yes, this section is only supposed to be overrides for host/target, but since I
can't can't negate the IPHONEOS_DEPLOYMENT_TARGET for host, I set it here
instead.

On 2013/05/08 22:23:07, Nico wrote:
> is SDKROOT set to the right thing for ios?

[Nico, 2013-05-08 23:01:00]

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi#newcode...
build/common.gypi:1337: ['OS=="mac"', {
On 2013/05/08 22:49:57, justincohen wrote:
> I can't have them as siblings and depend on each other ('mac_sdk_path', etc)
> 
> Is there a better way?

Ah, hm. Maybe you can move the independent things out at least (mac_keystone,
mac_product_name, mac_breakpad*, clang)?

Don't you need the mac_sdk stuff for ios host builds too?

Which parts of this block do you need for ios target builds? (I'm wondering why
there's no _target==host here)

> 
> On 2013/05/08 22:23:07, Nico wrote:
> > Could you just keep this in the enclosing conditions block instead of
nesting
> it
> > here? Then this can remain less indented.
>

[justincohen, 2013-05-09 00:01:42]

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi#newcode...
build/common.gypi:1337: ['OS=="mac"', {
The mac_sdk part is right in the mac_breakpad, so we'd have to split that up. 
Would that make it more confusing?

iOS host needs mac_sdk_path, but mac_* won't conflict with anything so I didn't
add the target==host here.

On 2013/05/08 23:01:00, Nico wrote:
> On 2013/05/08 22:49:57, justincohen wrote:
> > I can't have them as siblings and depend on each other ('mac_sdk_path', etc)
> > 
> > Is there a better way?
> 
> Ah, hm. Maybe you can move the independent things out at least (mac_keystone,
> mac_product_name, mac_breakpad*, clang)?
> 
> Don't you need the mac_sdk stuff for ios host builds too?
> 
> Which parts of this block do you need for ios target builds? (I'm wondering
why
> there's no _target==host here)
> 
> > 
> > On 2013/05/08 22:23:07, Nico wrote:
> > > Could you just keep this in the enclosing conditions block instead of
> nesting
> > it
> > > here? Then this can remain less indented.
> > 
>

[Nico, 2013-05-09 00:46:17]

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi#newcode...
build/common.gypi:1337: ['OS=="mac"', {
On 2013/05/09 00:01:42, justincohen wrote:
> The mac_sdk part is right in the mac_breakpad, so we'd have to split that up. 
> Would that make it more confusing?
> 
> iOS host needs mac_sdk_path, but mac_* won't conflict with anything so I
didn't
> add the target==host here.

What pieces of the variables in the os=="mac" block cause issues for ios then?
Just 'clang'?

> 
> On 2013/05/08 23:01:00, Nico wrote:
> > On 2013/05/08 22:49:57, justincohen wrote:
> > > I can't have them as siblings and depend on each other ('mac_sdk_path',
etc)
> > > 
> > > Is there a better way?
> > 
> > Ah, hm. Maybe you can move the independent things out at least
(mac_keystone,
> > mac_product_name, mac_breakpad*, clang)?
> > 
> > Don't you need the mac_sdk stuff for ios host builds too?
> > 
> > Which parts of this block do you need for ios target builds? (I'm wondering
> why
> > there's no _target==host here)
> > 
> > > 
> > > On 2013/05/08 22:23:07, Nico wrote:
> > > > Could you just keep this in the enclosing conditions block instead of
> > nesting
> > > it
> > > > here? Then this can remain less indented.
> > > 
> > 
>

[justincohen, 2013-05-09 03:44:02]

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi#newcode...
build/common.gypi:1337: ['OS=="mac"', {
Yeah, just clang.  I pulled out clang:'1' into it's own mac only section and
removed the conditional.

On 2013/05/09 00:46:17, Nico wrote:
> On 2013/05/09 00:01:42, justincohen wrote:
> > The mac_sdk part is right in the mac_breakpad, so we'd have to split that
up. 
> > Would that make it more confusing?
> > 
> > iOS host needs mac_sdk_path, but mac_* won't conflict with anything so I
> didn't
> > add the target==host here.
> 
> What pieces of the variables in the os=="mac" block cause issues for ios then?
> Just 'clang'?
> 
> > 
> > On 2013/05/08 23:01:00, Nico wrote:
> > > On 2013/05/08 22:49:57, justincohen wrote:
> > > > I can't have them as siblings and depend on each other ('mac_sdk_path',
> etc)
> > > > 
> > > > Is there a better way?
> > > 
> > > Ah, hm. Maybe you can move the independent things out at least
> (mac_keystone,
> > > mac_product_name, mac_breakpad*, clang)?
> > > 
> > > Don't you need the mac_sdk stuff for ios host builds too?
> > > 
> > > Which parts of this block do you need for ios target builds? (I'm
wondering
> > why
> > > there's no _target==host here)
> > > 
> > > > 
> > > > On 2013/05/08 22:23:07, Nico wrote:
> > > > > Could you just keep this in the enclosing conditions block instead of
> > > nesting
> > > > it
> > > > > here? Then this can remain less indented.
> > > > 
> > > 
> > 
>

[Nico, 2013-05-09 04:00:10]

Cool, thanks.

Ship it!

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14597007/diff/27001/build/common.gypi#newcode...
build/common.gypi:4034: },
On 2013/05/08 22:49:57, justincohen wrote:
> Yes, this section is only supposed to be overrides for host/target, but since
I
> can't can't negate the IPHONEOS_DEPLOYMENT_TARGET for host, I set it here
> instead.

This could maybe use a comment.

> 
> On 2013/05/08 22:23:07, Nico wrote:
> > is SDKROOT set to the right thing for ios?
>

[stuartmorgan, 2013-05-13 16:16:55]

LGTM assuming we're sure that anything switching on the mac_ variables (e.g.,
mac_breakpad) is in a Mac-only block.

[justincohen, 2013-05-13 16:44:27]

I audited mac_keystone, mac_breakpad_uploads and mac_breakpad.  Most references
are in files only included when OS=="mac", and the remaining references are also
within OS="mac" blocks

[commit-bot: I haz the power, 2013-05-13 16:53:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justincohen@google.com/14597007/65001

[commit-bot: I haz the power, 2013-05-13 19:00:19]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-13 19:07:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justincohen@google.com/14597007/65001

[commit-bot: I haz the power, 2013-05-13 21:29:50]

Change committed as 199827

[Nico, 2013-11-27 00:34:08]

https://chromiumcodereview.appspot.com/14597007/diff/65001/breakpad/breakpad.gyp
File breakpad/breakpad.gyp (right):

https://chromiumcodereview.appspot.com/14597007/diff/65001/breakpad/breakpad....
breakpad/breakpad.gyp:108: ['OS=="mac" or (OS=="ios" and
"<(GENERATOR)"=="ninja")', {
I stumbled across this line today and wondered why the generator check is here,
even though I apparently reviewed the CL that added it. Maybe a short comment
explaining this check would be good.

[justincohen, 2013-11-27 18:42:59]

It's because XCode does the 're_run_targets' thing and stubs out to ninja for
this.  See
https://chromiumcodereview.appspot.com/14597007/diff/65001/breakpad/breakpad....

Is there a better way?

[Nico, 2013-11-27 18:47:03]

Dunno, just saying that a comment in the gyp file explaining why it's there
would've been helpful to me yesterday.


On Wed, Nov 27, 2013 at 10:43 AM, <justincohen@chromium.org> wrote:

> It's because XCode does the 're_run_targets' thing and stubs out to ninja
> for
> this.  See
> https://chromiumcodereview.appspot.com/14597007/diff/
> 65001/breakpad/breakpad.gyp#newcode765
>
> Is there a better way?
>
> https://chromiumcodereview.appspot.com/14597007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.