Use bundle_data and create_bundle to add support for iOS app bundle.

Use bundle_data and create_bundle to add support for iOS app bundle.

Add a new script //build/config/ios/ios_gen_plist.py to generate the
Info.plist by merging multiple source files, performing substitutions
of variables and converting to binary1 format.

Expand //build/config/ios/ios_sdk.gni to output variables required
for the substitution of variables in Info.plist.

Refactor ios_app template to use bundle_data and create_bundle to
create the application bundle.

BUG=297668
Committed: https://crrev.com/01285787153c6abbaaa7f86ace3023dd89159476
Cr-Commit-Position: refs/heads/master@{#381429}

[sdefresne, 2016-03-01 19:31:32]

sdefresne@chromium.org changed reviewers:
+ dpranke@chromium.org

[sdefresne, 2016-03-01 19:31:33]

Please take a look.

This depends on my changes to tools/gn landing first and then a new version
being deployed.

https://codereview.chromium.org/1751903003
https://codereview.chromium.org/1752033002

It also depends on a change to third_party/icu

https://codereview.chromium.org/1755473004

[Dirk Pranke, 2016-03-01 23:56:30]

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
File build/config/ios/ios_gen_plist.py (right):

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:30: provide a subclass to have the correct
support for @{{response_file_name}}.
Interesting, I didn't know about fromfile_prefix_chars ...

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:41: value: a list of values
nit: if this is actually a list, I'd call it 'values', not 'value'

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:46: by their value in |substitutions| or None
if any of the variable has no
nit: s/variable/variables/

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:74: # Some value need to be identifier, and
thus the variable references may
Nit: s/value/values/ , s/identifier/identifiers/

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:76: # to identifiers ("identifier" replace all
invalid characters by '-' and
s/replace/replaces

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:77: # "rfc1034identifier" replace them by "_"
to make valid URI too).
s/replace/replaces

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_sdk.gni
File build/config/ios/ios_sdk.gni (right):

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_sdk.gn...
build/config/ios/ios_sdk.gni:45: # This is weird, but Xcode set DTPlatformBuild
to an empty field for
s/set/sets ...

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:21: _create_app_bundle = target_name + ".app"
This is a bit confusing to me. Is _create_app_bundle supposed to refer to the
generated GN label, or the directory name of the bundle, or both? Can we split
these apart? It's strange to have a file extension be in the label name.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:79: # "visibility" and "deps".
I would probably drop this comment as calling things a "circular dependency" may
just create confusion. Declaring variables that are used in multiple targets the
way you are is pretty common and I don't think it needs a comment.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:83: # Generates the application executable.
I would omit this comment.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:105: # application bundle.
I'm not actually sure what you're trying to convey with this comment, but I'd
probably leave it out.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:121: create_bundle(_create_app_bundle) {
Can you just make this be 

  create_bundle(target_name) {
    ...
 
handle the app_name inside, and get rid of the separate group() target?

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:147: # - implements
//testing/iossim(//build/toolchain/mac:clang_x64) and then
Nit: s/implements/implement

[sdefresne, 2016-03-11 17:56:26]

Patchset #2 (id:20001) has been deleted

[sdefresne, 2016-03-11 17:57:12]

dpranke: please take another look

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
File build/config/ios/ios_gen_plist.py (right):

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:30: provide a subclass to have the correct
support for @{{response_file_name}}.
On 2016/03/01 at 23:56:29, Dirk Pranke wrote:
> Interesting, I didn't know about fromfile_prefix_chars ...

Ack.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:41: value: a list of values
On 2016/03/01 at 23:56:29, Dirk Pranke wrote:
> nit: if this is actually a list, I'd call it 'values', not 'value'

Done.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:46: by their value in |substitutions| or None
if any of the variable has no
On 2016/03/01 at 23:56:29, Dirk Pranke wrote:
> nit: s/variable/variables/

Done.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:74: # Some value need to be identifier, and
thus the variable references may
On 2016/03/01 at 23:56:29, Dirk Pranke wrote:
> Nit: s/value/values/ , s/identifier/identifiers/

Done.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:76: # to identifiers ("identifier" replace all
invalid characters by '-' and
On 2016/03/01 at 23:56:29, Dirk Pranke wrote:
> s/replace/replaces

Done.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:77: # "rfc1034identifier" replace them by "_"
to make valid URI too).
On 2016/03/01 at 23:56:30, Dirk Pranke wrote:
> s/replace/replaces

Done.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_sdk.gni
File build/config/ios/ios_sdk.gni (right):

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/ios_sdk.gn...
build/config/ios/ios_sdk.gni:45: # This is weird, but Xcode set DTPlatformBuild
to an empty field for
On 2016/03/01 at 23:56:30, Dirk Pranke wrote:
> s/set/sets ...

Done.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:21: _create_app_bundle = target_name + ".app"
On 2016/03/01 at 23:56:30, Dirk Pranke wrote:
> This is a bit confusing to me. Is _create_app_bundle supposed to refer to the
generated GN label, or the directory name of the bundle, or both? Can we split
these apart? It's strange to have a file extension be in the label name.

Done, but now I get the following error:

$ ninja -C out/Default base_unittest.app
ninja: Entering directory `out/Default'
ninja: error: unknown target 'base_unittest.app'

I've added a comment to find a way to fix this.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:79: # "visibility" and "deps".
On 2016/03/01 at 23:56:30, Dirk Pranke wrote:
> I would probably drop this comment as calling things a "circular dependency"
may just create confusion. Declaring variables that are used in multiple targets
the way you are is pretty common and I don't think it needs a comment.

Done.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:83: # Generates the application executable.
On 2016/03/01 at 23:56:30, Dirk Pranke wrote:
> I would omit this comment.

Done.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:105: # application bundle.
On 2016/03/01 at 23:56:30, Dirk Pranke wrote:
> I'm not actually sure what you're trying to convey with this comment, but I'd
probably leave it out.

Done.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:121: create_bundle(_create_app_bundle) {
On 2016/03/01 at 23:56:30, Dirk Pranke wrote:
> Can you just make this be 
> 
>   create_bundle(target_name) {
>     ...
>  
> handle the app_name inside, and get rid of the separate group() target?

Done, but now I get the following error:

$ ninja -C out/Default base_unittest.app
ninja: Entering directory `out/Default'
ninja: error: unknown target 'base_unittest.app'

I've added a comment to find a way to fix this.

https://codereview.chromium.org/1752873002/diff/1/build/config/ios/rules.gni#...
build/config/ios/rules.gni:147: # - implements
//testing/iossim(//build/toolchain/mac:clang_x64) and then
On 2016/03/01 at 23:56:30, Dirk Pranke wrote:
> Nit: s/implements/implement

Done.

[Dirk Pranke, 2016-03-11 20:56:21]

lgtm w/ one minor typo and a question ...

https://codereview.chromium.org/1752873002/diff/40001/build/config/ios/rules.gni
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1752873002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:156: # - find a way to make "ninja -C out/Default
base_unittest.app" work
This should actually be `base_unittests.app`, right? 

Also `ninja -C out/Debug base_unittests` works today, right, and this TODO is to
make the actual directory/app path also work, like it does today in gyp?

[sdefresne, 2016-03-14 09:52:48]

https://codereview.chromium.org/1752873002/diff/40001/build/config/ios/rules.gni
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1752873002/diff/40001/build/config/ios/rules....
build/config/ios/rules.gni:156: # - find a way to make "ninja -C out/Default
base_unittest.app" work
On 2016/03/11 at 20:56:21, Dirk Pranke wrote:
> This should actually be `base_unittests.app`, right? 

Right.

> Also `ninja -C out/Debug base_unittests` works today, right, and this TODO is
to make the actual directory/app path also work, like it does today in gyp?

Yes, exactly.

[sdefresne, 2016-03-14 10:47:19]

sdefresne@chromium.org changed reviewers:
+ thakis@chromium.org

[sdefresne, 2016-03-14 10:47:20]

thakis: can you review base/BUILD.gn as OWNERS?

You can see the design for create_bundle/bundle_data at
https://docs.google.com/document/d/1bKh57hg6TSBsEmeh0zWpOO5SVGA2INu-D3FGgsyIC...
(was sent to gn-dev, approved by brettw).

If you have no further comments, can you please send to CQ?

[sdefresne, 2016-03-15 14:29:04]

sdefresne@chromium.org changed reviewers:
+ brettw@chromium.org
- thakis@chromium.org

[sdefresne, 2016-03-15 14:29:05]

-thakis (OOO), +brettw: needs OWNERS stamp for base/BUILD.gn

[brettw, 2016-03-15 18:02:30]

lgtm

https://codereview.chromium.org/1752873002/diff/80001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1752873002/diff/80001/base/BUILD.gn#newcode1624
base/BUILD.gn:1624: bundle_data("base_unittests_bundle_data") {
Can bundles be set up in such a way that we don't need the conditional on each
one? If there's no create_bundle target that uses them, I think this would be
benign.

But in this case if we want iOS but not desktop Mac bundle data, we would still
need the conditional.

https://codereview.chromium.org/1752873002/diff/80001/build/config/ios/ios_ge...
File build/config/ios/ios_gen_plist.py (right):

https://codereview.chromium.org/1752873002/diff/80001/build/config/ios/ios_ge...
build/config/ios/ios_gen_plist.py:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
Would this file never be used for desktop Mac?

https://codereview.chromium.org/1752873002/diff/80001/build/config/ios/rules.gni
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1752873002/diff/80001/build/config/ios/rules....
build/config/ios/rules.gni:78: ] + rebase_path(sources, root_build_dir)
This is still probably within the command-line length limit on Mac, but if it's
causing problems don't forget about response_file_contents.

https://codereview.chromium.org/1752873002/diff/80001/build/config/ios/rules....
build/config/ios/rules.gni:101: "app_name",
I don't know if you saw the other code review but I'm thinking we should rename
app_name to be output_name for consistency (not in this patch).

https://codereview.chromium.org/1752873002/diff/80001/build/toolchain/mac/BUI...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/1752873002/diff/80001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:261: description = "COMPILE_XCASSETS {{inputs}}
{{output}}"
I don't think we want to list all inputs in the description name. Just the
output should be fine.

[Dirk Pranke, 2016-03-16 01:19:05]

https://codereview.chromium.org/1752873002/diff/80001/build/config/ios/ios_ge...
File build/config/ios/ios_gen_plist.py (right):

https://codereview.chromium.org/1752873002/diff/80001/build/config/ios/ios_ge...
build/config/ios/ios_gen_plist.py:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
On 2016/03/15 18:02:29, brettw wrote:
> Would this file never be used for desktop Mac?

I believe there's an open bug for unifying the ios versions of these scripts w/
the mac versions of these scripts. I'd view that as a separate task for now
(let's get iOS working and unblocked and then clean up).

[sdefresne, 2016-03-16 09:57:26]

Patchset #5 (id:100001) has been deleted

[sdefresne, 2016-03-16 09:58:49]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-03-16 09:58:49]

Thank you for the reviews.

https://codereview.chromium.org/1752873002/diff/80001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1752873002/diff/80001/base/BUILD.gn#newcode1624
base/BUILD.gn:1624: bundle_data("base_unittests_bundle_data") {
On 2016/03/15 at 18:02:29, brettw wrote:
> Can bundles be set up in such a way that we don't need the conditional on each
one? If there's no create_bundle target that uses them, I think this would be
benign.
> 
> But in this case if we want iOS but not desktop Mac bundle data, we would
still need the conditional.

Removed the conditional (since Mac does not currently uses create_bundle this
does not cause any problem currently).

https://codereview.chromium.org/1752873002/diff/80001/build/config/ios/ios_ge...
File build/config/ios/ios_gen_plist.py (right):

https://codereview.chromium.org/1752873002/diff/80001/build/config/ios/ios_ge...
build/config/ios/ios_gen_plist.py:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
On 2016/03/16 at 01:19:05, Dirk Pranke wrote:
> On 2016/03/15 18:02:29, brettw wrote:
> > Would this file never be used for desktop Mac?
> 
> I believe there's an open bug for unifying the ios versions of these scripts
w/ the mac versions of these scripts. I'd view that as a separate task for now
(let's get iOS working and unblocked and then clean up).

Yeah, the eventual plan is to use the same script on Mac and iOS. I'll
eventually move the file (and if possible change it to use mac_tool.py, but it
is currently not possible as mac_tool.py uses environment variable for input
which cannot be set in an "action" target in gn).

https://codereview.chromium.org/1752873002/diff/80001/build/config/ios/rules.gni
File build/config/ios/rules.gni (right):

https://codereview.chromium.org/1752873002/diff/80001/build/config/ios/rules....
build/config/ios/rules.gni:78: ] + rebase_path(sources, root_build_dir)
On 2016/03/15 at 18:02:29, brettw wrote:
> This is still probably within the command-line length limit on Mac, but if
it's causing problems don't forget about response_file_contents.

I was planning to do it (see custom ArgumentParser in
build/config/ios/ios_gen_plist.py), removed it for debugging purpose and forgot
to put it back. Done.

https://codereview.chromium.org/1752873002/diff/80001/build/config/ios/rules....
build/config/ios/rules.gni:101: "app_name",
On 2016/03/15 at 18:02:29, brettw wrote:
> I don't know if you saw the other code review but I'm thinking we should
rename app_name to be output_name for consistency (not in this patch).

Yeah, I've seen the CL and have a pending CL to use output_name instead of
app_name.

https://codereview.chromium.org/1752873002/diff/80001/build/toolchain/mac/BUI...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/1752873002/diff/80001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:261: description = "COMPILE_XCASSETS {{inputs}}
{{output}}"
On 2016/03/15 at 18:02:29, brettw wrote:
> I don't think we want to list all inputs in the description name. Just the
output should be fine.

Done.

[sdefresne, 2016-03-16 09:58:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1752873002/#ps120001
(title: "Remove conditional around bundle_data, use response_file_contents,
clean description of compile_xca…")

[commit-bot: I haz the power, 2016-03-16 09:59:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752873002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752873002/120001

[commit-bot: I haz the power, 2016-03-16 10:55:50]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-03-16 10:56:51]

Description was changed from

==========
Use bundle_data and create_bundle to add support for iOS app bundle.

Add a new script //build/config/ios/ios_gen_plist.py to generate the
Info.plist by merging multiple source files, performing substitutions
of variables and converting to binary1 format.

Expand //build/config/ios/ios_sdk.gni to output variables required
for the substitution of variables in Info.plist.

Refactor ios_app template to use bundle_data and create_bundle to
create the application bundle.

BUG=297668
==========

to

==========
Use bundle_data and create_bundle to add support for iOS app bundle.

Add a new script //build/config/ios/ios_gen_plist.py to generate the
Info.plist by merging multiple source files, performing substitutions
of variables and converting to binary1 format.

Expand //build/config/ios/ios_sdk.gni to output variables required
for the substitution of variables in Info.plist.

Refactor ios_app template to use bundle_data and create_bundle to
create the application bundle.

BUG=297668

Committed: https://crrev.com/01285787153c6abbaaa7f86ace3023dd89159476
Cr-Commit-Position: refs/heads/master@{#381429}
==========

[commit-bot: I haz the power, 2016-03-16 10:56:52]

Patchset 5 (id:??) landed as
https://crrev.com/01285787153c6abbaaa7f86ace3023dd89159476
Cr-Commit-Position: refs/heads/master@{#381429}

[Nico, 2016-05-06 11:28:11]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-05-06 11:28:11]

xcodebuild is very very slow. Can you use something like this to replace at
least some of the xcodebuild calls with xcrun calls? 'Path' is --show-sdk-path,
'SDKVersion' is --show-sdk-version. I don't see something for
ProductBuildVersion, but gyp doesn't query that value, so maybe we don't need
it.

[Nico, 2016-05-06 13:55:43]

On 2016/05/06 11:28:11, Nico wrote:
> xcodebuild is very very slow. Can you use something like this to replace at
> least some of the xcodebuild calls with xcrun calls? 'Path' is
--show-sdk-path,
> 'SDKVersion' is --show-sdk-version. I don't see something for
> ProductBuildVersion, but gyp doesn't query that value, so maybe we don't need
> it.

"this" above was meant to be a link to 
https://codereview.chromium.org/1521233002/