Add support for Mac/iOS application bundles to GN tool.

Add support for Mac/iOS application bundles to GN tool.

Add a new property to all targets named "bundle_data". The values
are propagated down the dependency chain, stopping at executable,
shared_library and loadable_modules.

Add a new target "copy_bundle_data" similar to "copy" except that
the list of files to copy comes from the "bundle_data" property of
its dependencies (it can be empty).

Add a new tool "copy_bundle_data" used by "copy_bundle_data" target.

BUG=546283, 297668

[sdefresne, 2016-01-18 16:02:24]

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

[sdefresne, 2016-01-18 16:08:32]

This is not yet ready for in-depth review, but I'd like high-level comment on
whether this is the correct way to go to add support for bundle data to gn.

It looks like in gyp, the rule was to put a "mac_bundle_resources" in
"link_settings" to have it propagated to the final executable. This change does
something similar by stuffing the "bundle_data" into the "config" objects.
However, due to the dependency rules, this only work if the executable depends
only via "public_deps" on the targets defining the "config", and the config need
to be part of "public_configs".

Is this a good way to do it (putting the data in the config objects)? If so, do
you think the restriction of having only public_deps is problematic? Should I
introduce a new type of deps to fix it? If putting the bundle_data in the
"config" object, were would you recommend putting it?

BTW, to test the following CL, you also need to apply the following patch to
third_party/icu:

diff --git a/BUILD.gn b/BUILD.gn
index 77f8aa6..e845911 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -503,7 +503,7 @@ component("icuuc") {
     "source/common/wintz.c",
   ]
   defines = [ "U_COMMON_IMPLEMENTATION" ]
-  deps = [
+  public_deps = [
     ":icudata",
   ]
 
@@ -538,10 +538,14 @@ if (is_android) {
 # TODO(GYP) support use_system_icu.
 if (icu_use_data_file) {
   if (is_ios) {
+    config("icudata_bundle_data") {
+      visibility = [ ":icudata" ]
+      bundle_data = [
+        "source/data/in/icudtl.dat",
+      ]
+    }
     group("icudata") {
-      # TODO(GYP): Provide an empty group so that this target does not
-      # unconditionally fail. The dat file needs to be bundled separately. In
-      # the longer term, need to figure out how to use system ICU
+      public_configs = [ ":icudata_bundle_data" ]
     }
   } else {
     copy("icudata") {

[brettw, 2016-01-20 18:35:36]

I think this general concept seems fine. Let me know if you want me to look at
any code specifics.

[sdefresne, 2016-01-20 20:58:07]

Description was changed from

==========
Proof-of-Concept: Support for copying files to application bundle.

Add a new field in "config" object named "bundle_data" corresponding
to all the files that need to be copied to the application bundle on
iOS.

Add a new type of target "copy_bundle_data" that is similar to "copy"
but gets the list of source files from the "bundle_data" field of all
its dependent target.

Update build/config/ios/rules.gni to use this new target and update
build/toolchain/mac/BUILD.gn to configure the "copy_bundle_data" tool.

Caveat: since this patchset depends on the config rules, i.e. it will
only collect the "bundle_data" that are defined in "public_configs"
of "public_deps" (fixing this may require adding a new type of deps
or config).

BUG=546283,297668
==========

to

==========
Support for application bundle on iOS.

tools/gn:

  Add a new property to "config" objects named "bundle_data" containing
  a list of files that are necessary at runtime for running the app and
  that need to be copied in application bundle. In general those config
  objects need to be added to "all_dependent_configs" to work properly.

  Add a new target "copy_bundle_data" to copy files into the app bundle.
  The list of file is resolved from the inherited configs' "bundle_data"
  and are copied using the "copy_bundle_data" tool.

build/config/ios:

  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).

build/toolchain/mac/BUILD.gn:

  Configure the "copy_bundle_data" tool on iOS and Mac. The tool uses the
  gyp helper script tools/gyp/pylib/gyp/mac_tool.py to stay as close as
  possible to gyp.

base/BUILD.gn:

  Explicitly list all test files used on iOS in order to get them copied
  into the application bundle. GYP used a script for that step which is
  discouraged when using GN (slow down the GN generation).

ios/web/shell/BUILD.gn:

  Fixes incorrect dependency.

BUG=546283,297668
==========

[sdefresne, 2016-01-20 21:02:25]

I think the CL is now ready for review as I was able to build base_unittests and
have it pass all tests using this CL and its companion
https://codereview.chromium.org/1608343002 (this is using the version of iossim
build using GYP as it does not currently builds with GN).

$ rm -rf out/Default/
$ gn gen --args='target_os="ios"' out/Default
Done. Wrote 1258 targets from 269 files in 2163ms
$ ninja -C out/Default base_unittests
ninja: Entering directory `out/Default'
[1040/1040] STAMP obj/base/base_unittests.stamp
$ out/Default-iphonesimulator/iossim -d 'iPhone 6' -s '9.2'
out/Default/base_unittests.app 
...
[----------] Global test environment tear-down
[==========] 1565 tests from 230 test cases ran. (52958 ms total)
[  PASSED  ] 1565 tests.

  YOU HAVE 27 DISABLED TESTS

brettw:  please look at tools/gn changes
dpranke: please look at *.gn & *.gni changes

[brettw, 2016-01-20 21:31:48]

We need to split apart the tools/gn changes from the build changes. We need to
land the GN tool changes, then do a binary roll, then we can check in the build
changes.

[brettw, 2016-01-20 22:23:17]

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

https://codereview.chromium.org/1606553002/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"
My understanding is that the source will be a single file and this will be
called once for every file in the bundle rule, and output is the directory.

As-written, it looks like this will clear the directory every time we add a new
file. If we want to clear the directory (and I think we do), we should probably
have a command that lists all input files and does the bundle copy in one step.

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/config_values.h
File tools/gn/config_values.h (right):

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/config_values....
tools/gn/config_values.h:33: const std::vector<SourceFile>& name() const {
return name##_; } \
Putting this on config values seems wrong to me. No other source or data is on
here. Configs are really for settings that need to be applied to multiple
targets. I don't think that's the case here, and that bundle data is really
associated with a single target and propagated through the dependency tree.

If my understanding is correct, we should put the bundle data on the Target like
data is, and to collect the sources to it, iterate the dependency tree and
collect the files (as we do for data, see runtime_deps.cc).

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/copy_target_ge...
File tools/gn/copy_target_generator.cc (right):

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/copy_target_ge...
tools/gn/copy_target_generator.cc:46:
CopyBundleDataTargetGenerator::CopyBundleDataTargetGenerator(
I don't like having this one class in the same file as another when nothing else
works this way.

If you feel like a separate file is too stupid, we can pass the target type in
the constructor and have the copy class do both things (the binary target writer
does this).

Actually, since the ninja target writer is shared with the regular copy target
type, the generator should probably be the same class also.

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/functions_targ...
File tools/gn/functions_target.cc (right):

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/functions_targ...
tools/gn/functions_target.cc:310: "More detailed help need to be written\n";
Please do this :)

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/target.h
File tools/gn/target.h (right):

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/target.h#newco...
tools/gn/target.h:139: output_type_ == COPY_BUNDLE_DATA;
Are you sure you need this?

The COPY_FILES rule is here because it may copy a header file that is required
to compile a source file. Unless the copy rule is executed first, compilation
may fail.

But I don't think compilation steps will take bundle data as input. I believe
only an script will, and an action will always require its inputs to be
up-to-date before doing anything.

The reason not to do this is that the COPY_BUNDLE_DATA step will require that
all dependencies be up-to-date, and marking it as hard requires it be done
before any dependencies are compiled, so this will serialize a significant part
of the build.

If not having this gives a problem, we should understand exactly what the issue
is, maybe there's a less destructive way to express what we need.

[sdefresne, 2016-01-21 21:56:48]

Description was changed from

==========
Support for application bundle on iOS.

tools/gn:

  Add a new property to "config" objects named "bundle_data" containing
  a list of files that are necessary at runtime for running the app and
  that need to be copied in application bundle. In general those config
  objects need to be added to "all_dependent_configs" to work properly.

  Add a new target "copy_bundle_data" to copy files into the app bundle.
  The list of file is resolved from the inherited configs' "bundle_data"
  and are copied using the "copy_bundle_data" tool.

build/config/ios:

  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).

build/toolchain/mac/BUILD.gn:

  Configure the "copy_bundle_data" tool on iOS and Mac. The tool uses the
  gyp helper script tools/gyp/pylib/gyp/mac_tool.py to stay as close as
  possible to gyp.

base/BUILD.gn:

  Explicitly list all test files used on iOS in order to get them copied
  into the application bundle. GYP used a script for that step which is
  discouraged when using GN (slow down the GN generation).

ios/web/shell/BUILD.gn:

  Fixes incorrect dependency.

BUG=546283,297668
==========

to

==========
Add support for Mac/iOS application bundles to GN tool.

Add a new property to all targets named "bundle_data". The values
are propagated down the dependency chain, stopping at executable,
shared_library and loadable_modules.

Add a new target "copy_bundle_data" similar to "copy" except that
the list of files to copy comes from the "bundle_data" property of
its dependencies (it can be empty).

Add a new tool "copy_bundle_data" used by "copy_bundle_data" target.

BUG=546283,297668
==========

[sdefresne, 2016-01-21 22:00:55]

sdefresne@chromium.org changed reviewers:
- andybons@chromium.org, dpranke@chromium.org

[sdefresne, 2016-01-21 22:00:56]

Ok, I've split the CL in two as suggested. The other half (.gn/.gni changes) can
be found at https://codereview.chromium.org/1611363003. I guess this CL now only
contains changes to tools/gn.

andybons/dpranke: FYI
brettw: please take another look.

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

https://codereview.chromium.org/1606553002/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/20 at 22:23:17, brettw wrote:
> My understanding is that the source will be a single file and this will be
called once for every file in the bundle rule, and output is the directory.
> 
> As-written, it looks like this will clear the directory every time we add a
new file. If we want to clear the directory (and I think we do), we should
probably have a command that lists all input files and does the bundle copy in
one step.

This is incorrect. This tool will be invoked for each file listed in
"bundle_data" independently, with {{output}} being a single file (as is done
with the "copy" tool). I could not simply re-use the "copy" tool as copying file
in the bundle may require some additional processing (conversion to binary for
.plist, ...).

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/config_values.h
File tools/gn/config_values.h (right):

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/config_values....
tools/gn/config_values.h:33: const std::vector<SourceFile>& name() const {
return name##_; } \
On 2016/01/20 at 22:23:17, brettw wrote:
> Putting this on config values seems wrong to me. No other source or data is on
here. Configs are really for settings that need to be applied to multiple
targets. I don't think that's the case here, and that bundle data is really
associated with a single target and propagated through the dependency tree.
> 
> If my understanding is correct, we should put the bundle data on the Target
like data is, and to collect the sources to it, iterate the dependency tree and
collect the files (as we do for data, see runtime_deps.cc).

Done. Code is much cleaner. Thanks.

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/copy_target_ge...
File tools/gn/copy_target_generator.cc (right):

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/copy_target_ge...
tools/gn/copy_target_generator.cc:46:
CopyBundleDataTargetGenerator::CopyBundleDataTargetGenerator(
On 2016/01/20 at 22:23:17, brettw wrote:
> I don't like having this one class in the same file as another when nothing
else works this way.
> 
> If you feel like a separate file is too stupid, we can pass the target type in
the constructor and have the copy class do both things (the binary target writer
does this).
> 
> Actually, since the ninja target writer is shared with the regular copy target
type, the generator should probably be the same class also.

Merged.

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/functions_targ...
File tools/gn/functions_target.cc (right):

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/functions_targ...
tools/gn/functions_target.cc:310: "More detailed help need to be written\n";
On 2016/01/20 at 22:23:17, brettw wrote:
> Please do this :)

Done.

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/target.h
File tools/gn/target.h (right):

https://codereview.chromium.org/1606553002/diff/20001/tools/gn/target.h#newco...
tools/gn/target.h:139: output_type_ == COPY_BUNDLE_DATA;
On 2016/01/20 at 22:23:17, brettw wrote:
> Are you sure you need this?
> 
> The COPY_FILES rule is here because it may copy a header file that is required
to compile a source file. Unless the copy rule is executed first, compilation
may fail.
> 
> But I don't think compilation steps will take bundle data as input. I believe
only an script will, and an action will always require its inputs to be
up-to-date before doing anything.
> 
> The reason not to do this is that the COPY_BUNDLE_DATA step will require that
all dependencies be up-to-date, and marking it as hard requires it be done
before any dependencies are compiled, so this will serialize a significant part
of the build.
> 
> If not having this gives a problem, we should understand exactly what the
issue is, maybe there's a less destructive way to express what we need.

Good point. Removed.

[brettw, 2016-01-25 21:06:09]

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

https://codereview.chromium.org/1606553002/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"
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.

https://codereview.chromium.org/1606553002/diff/40001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/1606553002/diff/40001/tools/gn/variables.cc#n...
tools/gn/variables.cc:1413: INSERT_VARIABLE(Asmflags)
You need to add BundleData here so it will appear in the help.

[sdefresne, 2016-01-26 12:11:55]

Thank you for the review, please take another look.

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

https://codereview.chromium.org/1606553002/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.

Replied on https://codereview.chromium.org/1611363003.

https://codereview.chromium.org/1606553002/diff/40001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/1606553002/diff/40001/tools/gn/variables.cc#n...
tools/gn/variables.cc:1413: INSERT_VARIABLE(Asmflags)
On 2016/01/25 at 21:06:08, brettw wrote:
> You need to add BundleData here so it will appear in the help.

Done, thank you.

[brettw, 2016-01-26 23:51:41]

https://codereview.chromium.org/1606553002/diff/60001/tools/gn/ninja_copy_tar...
File tools/gn/ninja_copy_target_writer_unittest.cc (right):

https://codereview.chromium.org/1606553002/diff/60001/tools/gn/ninja_copy_tar...
tools/gn/ninja_copy_target_writer_unittest.cc:89: NinjaCopyTargetWriter
writer(&target, out, Toolchain::TYPE_COPY);
Can you add a case for the new type?

https://codereview.chromium.org/1606553002/diff/60001/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/1606553002/diff/60001/tools/gn/target.cc#newc...
tools/gn/target.cc:424: void Target::PullRecursiveBundleData() {
We should have a test in target_unittest.cc that the files get merged properly
across targets, and also not merged when it's an executable (or whatever).

Also: this can make duplicates. Whether we want to make this list unique or not
may depend on how we expect the build to be structured and if a resource might
appear in multiple targets.

[sdefresne, 2016-01-27 13:13:45]

Added unit tests, please take another look.

https://codereview.chromium.org/1606553002/diff/60001/tools/gn/ninja_copy_tar...
File tools/gn/ninja_copy_target_writer_unittest.cc (right):

https://codereview.chromium.org/1606553002/diff/60001/tools/gn/ninja_copy_tar...
tools/gn/ninja_copy_target_writer_unittest.cc:89: NinjaCopyTargetWriter
writer(&target, out, Toolchain::TYPE_COPY);
On 2016/01/26 at 23:51:41, brettw wrote:
> Can you add a case for the new type?

Done.

https://codereview.chromium.org/1606553002/diff/60001/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/1606553002/diff/60001/tools/gn/target.cc#newc...
tools/gn/target.cc:424: void Target::PullRecursiveBundleData() {
On 2016/01/26 at 23:51:41, brettw wrote:
> We should have a test in target_unittest.cc that the files get merged properly
across targets, and also not merged when it's an executable (or whatever).

Done.
 
> Also: this can make duplicates. Whether we want to make this list unique or
not may depend on how we expect the build to be structured and if a resource
might appear in multiple targets.

The bundle_data property is a std::set<SourceFile> so there won't be duplicate
by source path.

[brettw, 2016-02-02 22:44:35]

LGTM.

I'm hazy: were we going to wait on this until we get some higher-level design
figured out, or is this independent of the thing that was about?

[brettw, 2016-02-02 23:28:38]

https://codereview.chromium.org/1606553002/diff/80001/tools/gn/function_toolc...
File tools/gn/function_toolchain.cc (right):

https://codereview.chromium.org/1606553002/diff/80001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:408: "      \"copy\": Tool to copy files.\n"
Please also add your new tool here and check if there's any other places in the
documentation here or above (for toolchain) that need to be updated.

[sdefresne, 2016-02-03 17:43:14]

Sorry, yes, this CL is pending on writing a proper design document to addresses
some issues found while working on Chromium targets. Thank you for the review.

I'll ping once this is ready for further review.

[sdefresne, 2016-03-10 18:22:31]

Obsolete.