Re: ninja/mac: Create -iphoneos device builds for iOS ninja generator. (issue 25355002)

[tha...@chromium.org]

I think I like this approach.

Does this means that the <(GENERATOR) checks you have in your gyp files can
go
away?


https://codereview.chromium.org/25355002/diff/32001/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/25355002/diff/32001/pylib/gyp/generator/ninja.py#newcode2118
pylib/gyp/generator/ninja.py:2118: for config_name in
target_dict['configurations'].keys():
Omit ".keys()", iterating a dict iterates the keys.

…but you only use the key to get at the value, so maybe iterate over
.values()?

https://codereview.chromium.org/25355002/diff/32001/pylib/gyp/generator/ninja.py#newcode2120
pylib/gyp/generator/ninja.py:2120: settings =
config.get('xcode_settings', None)
None is the default for get, you can omit it

– but if you use {} as default you don't need the "if settings" check in
the next line

https://codereview.chromium.org/25355002/diff/32001/pylib/gyp/generator/ninja.py#newcode2130
pylib/gyp/generator/ninja.py:2130: new_config_name =
config_name+'-iphoneos'
spaces around +

https://codereview.chromium.org/25355002/diff/32001/pylib/gyp/generator/ninja.py#newcode2131
pylib/gyp/generator/ninja.py:2131: new_config_dict =
copy.deepcopy(config)
How much does this slow down gyp?

https://codereview.chromium.org/25355002/diff/32001/pylib/gyp/generator/ninja.py#newcode2138
pylib/gyp/generator/ninja.py:2138: def GenerateOutput(target_list,
target_dicts, data, params):
call xcode_emulation.CloneConfigurationsForDeviceAndEmulator(targets)
here, and move the remaining code in this cl into xcode_emulation. Add
docstrings to your new functions.

(the if statement should be in CloneConfigurationsForDeviceAndEmulator
too)

https://codereview.chromium.org/25355002/

[fisc...@chromium.org]

<driveby>

Can you update the CL description with what .gyp authors should do to
accomodate
this change? I can't tell e.g. what will need to change in my workflow
(https://code.google.com/p/webrtc/source/browse/trunk/talk/app/webrtc/objc/README)
as a result of this CL.

https://codereview.chromium.org/25355002/

[fisc...@chromium.org]

Thanks Justin. The key I missed before is that is creating new
configurations,
not new targets (i.e. out/Debug-iphoneos/MyTarget, not
out/Debug/MyTarget-iphoneos).
Meaningless LGTM :)

https://codereview.chromium.org/25355002/

[tha...@chromium.org]

lgtm


https://codereview.chromium.org/25355002/diff/45001/pylib/gyp/xcode_emulation.py
File pylib/gyp/xcode_emulation.py (right):

https://codereview.chromium.org/25355002/diff/45001/pylib/gyp/xcode_emulation.py#newcode1194
pylib/gyp/xcode_emulation.py:1194:
python style guide says two blank lines between functions

https://codereview.chromium.org/25355002/diff/45001/pylib/gyp/xcode_emulation.py#newcode1195
pylib/gyp/xcode_emulation.py:1195: def HasIOSTarget(targets):
this is module-private, start name with a _

https://codereview.chromium.org/25355002/diff/45001/pylib/gyp/xcode_emulation.py#newcode1201
pylib/gyp/xcode_emulation.py:1201: if
settings.get('IPHONEOS_DEPLOYMENT_TARGET'):
If it fits in one line, go with

if config.get('xcode_settings', {}).get('IPHONEOS_DEPLOYMENT_TARGET'):

https://codereview.chromium.org/25355002/diff/45001/pylib/gyp/xcode_emulation.py#newcode1206
pylib/gyp/xcode_emulation.py:1206: def
AddIOSDeviceConfigurations(targets):
Also add a _ in front of the name

https://codereview.chromium.org/25355002/