Add support for iOS application bundle to GN.

Add support for iOS application bundle to GN.

Add a new script to generate the Info.plist (variable substitution and
merging of multiple Info.plist) and update ios_sdk.py to report extra
informations required for the generation of the application Info.plist.
Most of that code was baked into gyp itself.

Add a new template "ios_test_app" to limit the amount to iOS-specific
code into testing/test.gni. Configure it to use the default signing
identity, default Info.plist, default application background (required
to get correct scaling),

Cleanup ios_app.py as it is only used for signing the application and
rename it to ios_code_sign.py (to better reflect its purpose).

BUG=546283, 297668

[sdefresne, 2016-01-21 22:02:08]

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

[sdefresne, 2016-01-21 22:02:08]

Please take a look.

brettw: for OWNERS of base/BUILD.gn
dpranke: for the rest of the CL

[sdefresne, 2016-01-21 22:02:51]

It is expected that the bots fails since the CL requires a new version of GN. I
should not have sent it to the try bots, sorry for the red ...

[brettw, 2016-01-22 19:00:12]

lgtm

https://codereview.chromium.org/1611363003/diff/1/build/config/ios/ios_code_s...
File build/config/ios/ios_code_sign.py (right):

https://codereview.chromium.org/1611363003/diff/1/build/config/ios/ios_code_s...
build/config/ios/ios_code_sign.py:6: import errno
I think "errno" and "os" are unused.

https://codereview.chromium.org/1611363003/diff/1/build/config/ios/ios_code_s...
build/config/ios/ios_code_sign.py:14: '/usr/bin/env',
Random question: this env call is just to search the path for the app? (I
realize this code is old).

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

https://codereview.chromium.org/1611363003/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:15: IDENT_RE = re.compile(r'[/\s]')
Can you provide documentation for these about what they're trying to do?

https://codereview.chromium.org/1611363003/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:20: 
Delete blank line.

https://codereview.chromium.org/1611363003/diff/1/testing/test.gni
File testing/test.gni (right):

https://codereview.chromium.org/1611363003/diff/1/testing/test.gni#newcode203
testing/test.gni:203: if (!defined(invoker.deps)) {
I'd make this:
  if (!defined(deps)) {
now.

[sdefresne, 2016-01-25 14:01:22]

https://codereview.chromium.org/1611363003/diff/1/build/config/ios/ios_code_s...
File build/config/ios/ios_code_sign.py (right):

https://codereview.chromium.org/1611363003/diff/1/build/config/ios/ios_code_s...
build/config/ios/ios_code_sign.py:6: import errno
On 2016/01/22 at 19:00:12, brettw wrote:
> I think "errno" and "os" are unused.

Removed, thank you.

https://codereview.chromium.org/1611363003/diff/1/build/config/ios/ios_code_s...
build/config/ios/ios_code_sign.py:14: '/usr/bin/env',
On 2016/01/22 at 19:00:12, brettw wrote:
> Random question: this env call is just to search the path for the app? (I
realize this code is old).

I think that is it's purpose, and it is probably useless as
subprocess.check_call do check the PATH. Removed.

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

https://codereview.chromium.org/1611363003/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:15: IDENT_RE = re.compile(r'[/\s]')
On 2016/01/22 at 19:00:12, brettw wrote:
> Can you provide documentation for these about what they're trying to do?

Done.

https://codereview.chromium.org/1611363003/diff/1/build/config/ios/ios_gen_pl...
build/config/ios/ios_gen_plist.py:20: 
On 2016/01/22 at 19:00:12, brettw wrote:
> Delete blank line.

Done.

https://codereview.chromium.org/1611363003/diff/1/testing/test.gni
File testing/test.gni (right):

https://codereview.chromium.org/1611363003/diff/1/testing/test.gni#newcode203
testing/test.gni:203: if (!defined(invoker.deps)) {
On 2016/01/22 at 19:00:12, brettw wrote:
> I'd make this:
>   if (!defined(deps)) {
> now.

Done.

[sdefresne, 2016-01-26 12:12:06]

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

https://codereview.chromium.org/1611363003/diff/20001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:235: command = "rm -rf {{output}} && ./gyp-mac-tool
copy-bundle-resource {{source}} {{output}} True"
On 2016/01/25 at 21:06:08, brettw wrote:
> I see, I see "rm -rf" and I assume the parameter is a directory. It looks like
the rm isn't even necessary? The normal copy command doesn't do this.

Removed the "rm -rf" (but the default "copy" tool does include a "rm -rf" in
case the hardlink fails and it has to fallback to cp).

[Dirk Pranke, 2016-01-29 00:54:25]

lgtm, but see comments below.

For base unittests, the list of files is small enough that I'd be happy to list
them all explicitly, but for the bigger test binaries we should probably support
directories.

https://codereview.chromium.org/1611363003/diff/60001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1611363003/diff/60001/base/BUILD.gn#newcode1881
base/BUILD.gn:1881: bundle_test_name = "baseunittests"
are underscores not allowed in the application ids ?

https://codereview.chromium.org/1611363003/diff/60001/base/BUILD.gn#newcode1882
base/BUILD.gn:1882: test_data_files = [
I think having to specify *every* test file may end up making things hard to
maintain.

In GYP, and in the isolates, we specify whole directories. This has the downside
of implying that ninja (and GYP) can't know when it needs to rebuild targets for
CLs that only change (or add) test files, but it has the upside that the build
files are a lot easier to maintain.

From talking to stuartmorgan@ and maruel@, it sounds like this is rarely a
problem in practice (changes to test files are usually accompanied by changes to
source files), and so from talking to brettw@, I think we agreed that maybe we
should modify the copy code to support test_data_file items that are also
directories (as long as they end in "/" we would do a recursive copy).

That would make this CL simpler and also simplify your net_unittests and
components_unittests CLs significantly. WDYT?

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

https://codereview.chromium.org/1611363003/diff/60001/build/config/ios/rules....
build/config/ios/rules.gni:7: # Use this template to create application bundle
for iOS.
nit: "create an application bundle"

https://codereview.chromium.org/1611363003/diff/60001/build/config/ios/rules....
build/config/ios/rules.gni:206: # Use this template to create test application
bundle for iOS.
nit "create a test" ?

[Dirk Pranke, 2016-01-29 01:11:57]

I don't know if I've missed (or forgotten) some previous discussion of this
design, but looking at it again, I have a couple questions:

First, why do we need a separate 'copy_bundle_data' target, rather than just
having GN generate the rules automatically as part of building shared libraries
and executables, perhaps only if bundle_data is transitively non-empty? Is it
likely (or certain) that the actual settings inside copy_bundle_data will vary
per target? and is that really better than just moving those settings into
shared libraries and executables? 

Second, in earlier discussions on this I had noted the near-complete overlap
between 'bundle_data' and 'data' and 'data_deps'. It doesn't look like this CL
will fold data/data_deps into bundle_data, which means that we'll have to
specify the same things multiple times, in bundle_data and data.

Can we possibly avoid that?

I feel like the current design will end up complicating targets (and adding new
targets) somewhat to add the iOS-isms, and it seems like it might be nice to
avoid that, but maybe I'm missing complications or subtleties that led to this
solution?

[sdefresne, 2016-01-29 11:44:21]

On 2016/01/29 at 01:11:57, dpranke wrote:
> I don't know if I've missed (or forgotten) some previous discussion of this
design, but looking at it again, I have a couple questions:

There was no formal design document. I'll work on it.

This CL and https://codereview.chromium.org/1606553002 were part of a
proof-of-concept with me toying to see how I could implement the support for
bundles.

> First, why do we need a separate 'copy_bundle_data' target, rather than just
having GN generate the rules automatically as part of building shared libraries
and executables, perhaps only if bundle_data is transitively non-empty? Is it
likely (or certain) that the actual settings inside copy_bundle_data will vary
per target? and is that really better than just moving those settings into
shared libraries and executables? 

This is an interesting idea, I'll integrate into the design document. As you
said, the copy_bundle_data step will probably not have much variation. However,
it is not the only step in the creation of an application bundle.

Just for the record, here are the steps for the creation of a bundle, with the
optional steps:

1. create directory with ${target_name}.${bundle_extension},
2. (optional) copy the application binary or shared library there,
3. (optional) recursively copy all the bundle resources, maybe in a
sub-directory (on Mac),
4. (optional) compile .xcassets, creating a temporary .plist with assets
information,
5. copy bundle Info.plist, doing variable substitution, and if .xcasset .plist
is present, merge with it,
6. (optional) sign the bundle

In hindsight, I think a better way would be to add a create_bundle() target that
does all those steps (delegating to an external tool) with all the configurable
bits avaiblable. Then to get an application bundle, it just has to depends on an
executable target.

> Second, in earlier discussions on this I had noted the near-complete overlap
between 'bundle_data' and 'data' and 'data_deps'. It doesn't look like this CL
will fold data/data_deps into bundle_data, which means that we'll have to
specify the same things multiple times, in bundle_data and data.
> 
> Can we possibly avoid that?

I still think we should avoid this, yes. I'll integrate it in my design
document. I'll share it with gn-dev soon.

There are some problems however with "data":
1. it can list directories which may cause problem as ninja will be unable to
track the dependencies,
2. there is indication how the file / directory should be named in the bundle
(this is also a problem with "bundle_data" as implemented).

The first point can be problematic for real application (as opposed to tests).
Something common on iOS is to have .bundle containing just images loaded with
-[NSBundle pathForResource:ofType:]. If a new image is added in a bundle that
Chrome on iOS gets from a bundle we want it to get copied. I still do not know
how to fix that except by forcing the developer to explicitly list that
resource.

Regarding the second point, I just have to point that icu wants to add
"//third_party/icu/icudtl.dat" in "data" and have it copied to
"${target_name}.${bundle_extension}/icudtl.dat" while base_unittests wants to
add "//base/test/data/" and have it copied to
"${target_name}.${bundle_extension}/base/test/data/". We cannot have an
heuristic were we say that files are copied at the bundle root, while
directories have they full path as downstream we have some targets that want to
copy "//ios_internal/third_party/somelib/SomeLib.bundle/" as
"${target_name}.${bundle_extension}/SomeLib.bundle/".

Finally, the documentation will have to be updated as "data" will ends up being
required at compilation time (currently they are not).

> I feel like the current design will end up complicating targets (and adding
new targets) somewhat to add the iOS-isms, and it seems like it might be nice to
avoid that, but maybe I'm missing complications or subtleties that led to this
solution?

Ack, let's discuss this in the design document.

[Dirk Pranke, 2016-01-29 21:09:33]

On 2016/01/29 11:44:21, sdefresne (OOO till Feb 1st) wrote:
> In hindsight, I think a better way would be to add a create_bundle() target
that
> does all those steps (delegating to an external tool) with all the
configurable
> bits avaiblable. Then to get an application bundle, it just has to depends on
an
> executable target.

So, is your thinking that create_bundle() will be a template that invokes
copy_bundle_data() and the other actions as needed, and we will create explicit
create_bundle() targets for chrome and content_shell and other non-test
bundles, and then update the test() template to create bundles automatically
for test binaries?

> > Second, in earlier discussions on this I had noted the near-complete overlap
> between 'bundle_data' and 'data' and 'data_deps'. It doesn't look like this CL
> will fold data/data_deps into bundle_data, which means that we'll have to
> specify the same things multiple times, in bundle_data and data.
> > 
> > Can we possibly avoid that?
> 
> I still think we should avoid this, yes. I'll integrate it in my design
> document. I'll share it with gn-dev soon.

Great.

> 
> There are some problems however with "data":
> 1. it can list directories which may cause problem as ninja will be unable to
> track the dependencies,
> 2. there is indication how the file / directory should be named in the bundle
> (this is also a problem with "bundle_data" as implemented).
> 
> The first point can be problematic for real application (as opposed to tests).
> Something common on iOS is to have .bundle containing just images loaded with
> -[NSBundle pathForResource:ofType:]. If a new image is added in a bundle that
> Chrome on iOS gets from a bundle we want it to get copied. I still do not know
> how to fix that except by forcing the developer to explicitly list that
> resource.

1. is definitely a problem and I would think we would want all of the files to
be
listed explicitly for non-test binaries, just like we do for installers and
other
such things where we want to be very careful when the lists change.

> 
> Regarding the second point, I just have to point that icu wants to add
> "//third_party/icu/icudtl.dat" in "data" and have it copied to
> "${target_name}.${bundle_extension}/icudtl.dat" while base_unittests wants to
> add "//base/test/data/" and have it copied to
> "${target_name}.${bundle_extension}/base/test/data/". We cannot have an
> heuristic were we say that files are copied at the bundle root, while
> directories have they full path as downstream we have some targets that want
to
> copy "//ios_internal/third_party/somelib/SomeLib.bundle/" as
> "${target_name}.${bundle_extension}/SomeLib.bundle/".

Good point. Maybe we need some way to support both.

> Finally, the documentation will have to be updated as "data" will ends up
being
> required at compilation time (currently they are not).

I'm not sure what you mean here?

[sdefresne, 2016-01-30 10:37:26]

On 2016/01/29 at 21:09:33, dpranke wrote:
> On 2016/01/29 11:44:21, sdefresne (OOO till Feb 1st) wrote:
> > In hindsight, I think a better way would be to add a create_bundle() target
that
> > does all those steps (delegating to an external tool) with all the
configurable
> > bits avaiblable. Then to get an application bundle, it just has to depends
on an
> > executable target.
> 
> So, is your thinking that create_bundle() will be a template that invokes
> copy_bundle_data() and the other actions as needed, and we will create
explicit
> create_bundle() targets for chrome and content_shell and other non-test
> bundles, and then update the test() template to create bundles automatically
> for test binaries?

I think we should have a target type in GN to allow creation of bundles (maybe
"create_bundle") that will probably use a configurable tool, and then write
templates around it to create application (i.e. executable) bundles, framework
(i.e. shared library) bundles or plugin (i.e. application extension) bundles.
Then chrome and other app like web_shell or content_shell will use the mac_app()
template or ios_app() template.

> > > Second, in earlier discussions on this I had noted the near-complete
overlap
> > between 'bundle_data' and 'data' and 'data_deps'. It doesn't look like this
CL
> > will fold data/data_deps into bundle_data, which means that we'll have to
> > specify the same things multiple times, in bundle_data and data.
> > > 
> > > Can we possibly avoid that?
> > 
> > I still think we should avoid this, yes. I'll integrate it in my design
> > document. I'll share it with gn-dev soon.
> 
> Great.
> 
> > 
> > There are some problems however with "data":
> > 1. it can list directories which may cause problem as ninja will be unable
to
> > track the dependencies,
> > 2. there is indication how the file / directory should be named in the
bundle
> > (this is also a problem with "bundle_data" as implemented).
> > 
> > The first point can be problematic for real application (as opposed to
tests).
> > Something common on iOS is to have .bundle containing just images loaded
with
> > -[NSBundle pathForResource:ofType:]. If a new image is added in a bundle
that
> > Chrome on iOS gets from a bundle we want it to get copied. I still do not
know
> > how to fix that except by forcing the developer to explicitly list that
> > resource.
> 
> 1. is definitely a problem and I would think we would want all of the files to
be
> listed explicitly for non-test binaries, just like we do for installers and
other
> such things where we want to be very careful when the lists change.
> 
> > 
> > Regarding the second point, I just have to point that icu wants to add
> > "//third_party/icu/icudtl.dat" in "data" and have it copied to
> > "${target_name}.${bundle_extension}/icudtl.dat" while base_unittests wants
to
> > add "//base/test/data/" and have it copied to
> > "${target_name}.${bundle_extension}/base/test/data/". We cannot have an
> > heuristic were we say that files are copied at the bundle root, while
> > directories have they full path as downstream we have some targets that want
to
> > copy "//ios_internal/third_party/somelib/SomeLib.bundle/" as
> > "${target_name}.${bundle_extension}/SomeLib.bundle/".
> 
> Good point. Maybe we need some way to support both.
> 
> > Finally, the documentation will have to be updated as "data" will ends up
being
> > required at compilation time (currently they are not).
> 
> I'm not sure what you mean here?

Here's what "gn help" has to say about "data":

$ gn help data
data: Runtime data file dependencies.

  Lists files or directories required to run the given target. These are
  typically data files or directories of data files. The paths are
  interpreted as being relative to the current build file. Since these
  are runtime dependencies, they do not affect which targets are built
  or when. To declare input files to a script, use "inputs".

  Appearing in the "data" section does not imply any special handling
  such as copying them to the output directory. This is just used for
  declaring runtime dependencies. Runtime dependencies can be queried
  using the "runtime_deps" category of "gn desc" or written during
  build generation via "--runtime-deps-list-file".

  GN doesn't require data files to exist at build-time. So actions that
  produce files that are in turn runtime dependencies can list those
  generated files both in the "outputs" list as well as the "data"
  list.


Note the following that would become false if we start using "data" for bundles:

  Appearing in the "data" section does not imply any special handling
  such as copying them to the output directory.

  GN doesn't require data files to exist at build-time.

Both those statements will become false. This is what I was trying to say.

I've started working on the design document and I think it would be better to
discuss the matter there. Let's consider this CL on hold until the design is
agreed upon.

[sdefresne, 2016-02-05 10:20:00]

https://codereview.chromium.org/1611363003/diff/60001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1611363003/diff/60001/base/BUILD.gn#newcode1881
base/BUILD.gn:1881: bundle_test_name = "baseunittests"
On 2016/01/29 at 00:54:25, Dirk Pranke wrote:
> are underscores not allowed in the application ids ?

They are, but when using gyp, //testing/gtest.gyp went to great length to remove
the underscores from the target name
(https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest.gyp&...):

                'xcode_settings': {
                  'BUNDLE_ID_TEST_NAME':
                    '>!(echo ">(_target_name)" | sed -e "s/_//g")',
                  'INFOPLIST_FILE': '>(ios_unittest_info_plist_path)',
                },

https://codereview.chromium.org/1611363003/diff/60001/base/BUILD.gn#newcode1882
base/BUILD.gn:1882: test_data_files = [
On 2016/01/29 at 00:54:25, Dirk Pranke wrote:
> I think having to specify *every* test file may end up making things hard to
maintain.
> 
> In GYP, and in the isolates, we specify whole directories. This has the
downside of implying that ninja (and GYP) can't know when it needs to rebuild
targets for CLs that only change (or add) test files, but it has the upside that
the build files are a lot easier to maintain.
> 
> From talking to stuartmorgan@ and maruel@, it sounds like this is rarely a
problem in practice (changes to test files are usually accompanied by changes to
source files), and so from talking to brettw@, I think we agreed that maybe we
should modify the copy code to support test_data_file items that are also
directories (as long as they end in "/" we would do a recursive copy).
> 
> That would make this CL simpler and also simplify your net_unittests and
components_unittests CLs significantly. WDYT?

I agree that not listing all the files would make the CL simpler, however,
"copy" targets error out if the find directories in their "sources" argument
(they are of type SourceFile which raise an error if the path ends up with a
"/", see
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gn/source_di...).

This means that I cannot just use "data" as the value of "sources" for the
"copy" target (as it contains "test/data/"). I can add a second variable
"test_data_files" that just contains "test/data", but then it is strange to not
have a trailing slash here, and to duplicate the same information).

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

https://codereview.chromium.org/1611363003/diff/60001/build/config/ios/rules....
build/config/ios/rules.gni:7: # Use this template to create application bundle
for iOS.
On 2016/01/29 at 00:54:25, Dirk Pranke wrote:
> nit: "create an application bundle"

Ack.

https://codereview.chromium.org/1611363003/diff/60001/build/config/ios/rules....
build/config/ios/rules.gni:206: # Use this template to create test application
bundle for iOS.
On 2016/01/29 at 00:54:25, Dirk Pranke wrote:
> nit "create a test" ?

Ack.

[Dirk Pranke, 2016-02-10 01:18:13]

https://codereview.chromium.org/1611363003/diff/60001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1611363003/diff/60001/base/BUILD.gn#newcode1881
base/BUILD.gn:1881: bundle_test_name = "baseunittests"
On 2016/02/05 10:20:00, sdefresne wrote:
> On 2016/01/29 at 00:54:25, Dirk Pranke wrote:
> > are underscores not allowed in the application ids ?
> 
> They are, but when using gyp, //testing/gtest.gyp went to great length to
remove
> the underscores from the target name

Okay. We should probably preserve compatibility, then, but I wonder why they did
that ...

https://codereview.chromium.org/1611363003/diff/60001/base/BUILD.gn#newcode1882
base/BUILD.gn:1882: test_data_files = [
On 2016/02/05 10:20:00, sdefresne wrote:
> On 2016/01/29 at 00:54:25, Dirk Pranke wrote:
> > I think having to specify *every* test file may end up making things hard to
> maintain.
> > 
> > In GYP, and in the isolates, we specify whole directories. This has the
> downside of implying that ninja (and GYP) can't know when it needs to rebuild
> targets for CLs that only change (or add) test files, but it has the upside
that
> the build files are a lot easier to maintain.
> > 
> > From talking to stuartmorgan@ and maruel@, it sounds like this is rarely a
> problem in practice (changes to test files are usually accompanied by changes
to
> source files), and so from talking to brettw@, I think we agreed that maybe we
> should modify the copy code to support test_data_file items that are also
> directories (as long as they end in "/" we would do a recursive copy).
> > 
> > That would make this CL simpler and also simplify your net_unittests and
> components_unittests CLs significantly. WDYT?
> 
> I agree that not listing all the files would make the CL simpler, however,
> "copy" targets error out if the find directories in their "sources" argument
> (they are of type SourceFile which raise an error if the path ends up with a
> "/", see
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gn/source_di...).
> 
> This means that I cannot just use "data" as the value of "sources" for the
> "copy" target (as it contains "test/data/"). I can add a second variable
> "test_data_files" that just contains "test/data", but then it is strange to
not
> have a trailing slash here, and to duplicate the same information).

True. Maybe we should modify copy() to allow a recurse=True third parameter, or
add a copy_recursive function or something.

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

https://codereview.chromium.org/1611363003/diff/60001/build/config/ios/rules....
build/config/ios/rules.gni:255: copy(_target_name + ".testdata") {
we don't usually use periods in target names; copy(_target_name + "__test_data")
would be more conventional.

[sdefresne, 2016-03-10 18:23:13]

Obsolete.