Add "create_bundle" target in order to support bundle with gn.

Add "create_bundle" target in order to support bundle with gn.

The "create_bundle" target recursively collect information from all the
"bundle_data" targets it depends and create a bundle from them. This is
configurable in many way to support all kinds of bundles on OS X or iOS.

Design: https://docs.google.com/document/d/1bKh57hg6TSBsEmeh0zWpOO5SVGA2INu-D3FGgsyICzk/view

BUG=297668
Committed: https://crrev.com/1232aaeb49f5ec11e0e9fcc527bfa96de21904ca
Cr-Commit-Position: refs/heads/master@{#380454}

[sdefresne, 2016-03-01 19:18:04]

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

[sdefresne, 2016-03-01 19:18:05]

Please take a look.

Second CL implementing support for bundle to GN.

[sdefresne, 2016-03-01 19:18:11]

Patchset #1 (id:1) has been deleted

[brettw, 2016-03-02 21:12:42]

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/bundle_data.h
File tools/gn/bundle_data.h (right):

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/bundle_data.h#...
tools/gn/bundle_data.h:65: std::string root_dir_;
Can this have a comment that these values are subdirectories relative to
root_build_dir?

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

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:408: "      \"copy\": Tool to copy files.\n"
The new tools you added, as well as how they work and the new patterns you
added, should be added to this giant documentation block.

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

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/functions_targ...
tools/gn/functions_target.cc:275: "  This target create an OS X / iOS bundle
copying file listed as sources\n"
This sentence doesn't make sense to me. Can you expand on this a lot? (It should
be aimed at people that don't understand Mac bundles at all).

We should also be explicit about what platforms this works on and when it
applies. It looks currently like this does something on all platforms, and
requires that the tool exists, so it would normally be inside a mac/iOS
conditional).

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/functions_targ...
tools/gn/functions_target.cc:277: "  \"create_bundle\" target).\n";
The other target types have a variable section at the bottom for what they read.
Can you add this to here and to bundle_data above and also mark which ones are
required? Like:

  "Variables\n"
  "\n"
  "bundle_root_dir (required), bundle_resources_dir\n" ...

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/ninja_create_b...
File tools/gn/ninja_create_bundle_target_writer.cc (right):

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/ninja_create_b...
tools/gn/ninja_create_bundle_target_writer.cc:38: if
(!target_->toolchain()->GetTool(Toolchain::TYPE_COPY_BUNDLE_DATA)) {
We should have a unit test file for this like we do for the other writer files.

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

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/target.cc#newc...
tools/gn/target.cc:483: void Target::PullRecursiveBundleData() {
We should have a unit test for this showing this getting pulled across some tree
of deps, but not across create_bundle targets. In target_unittest.cc there are
some tests like this you can model yours off of.

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

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/variables.cc#n...
tools/gn/variables.cc:443: "  A string corresponding to a path in
$root_build_dir.\n"
Don't use the $ here since the $ sign just does variable string expansion rather
than referring to variables.

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/variables.cc#n...
tools/gn/variables.cc:447: "  This must correspond to a path under
$root_build_dir.\n"
No $ here and in cases below.

[sdefresne, 2016-03-07 20:47:35]

Please take another look. I think I addressed all comments.

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/bundle_data.h
File tools/gn/bundle_data.h (right):

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/bundle_data.h#...
tools/gn/bundle_data.h:65: std::string root_dir_;
On 2016/03/02 at 21:12:42, brettw wrote:
> Can this have a comment that these values are subdirectories relative to
root_build_dir?

Done.

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

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:408: "      \"copy\": Tool to copy files.\n"
On 2016/03/02 at 21:12:42, brettw wrote:
> The new tools you added, as well as how they work and the new patterns you
added, should be added to this giant documentation block.

Done.

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

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/functions_targ...
tools/gn/functions_target.cc:275: "  This target create an OS X / iOS bundle
copying file listed as sources\n"
On 2016/03/02 at 21:12:42, brettw wrote:
> This sentence doesn't make sense to me. Can you expand on this a lot? (It
should be aimed at people that don't understand Mac bundles at all).
> 
> We should also be explicit about what platforms this works on and when it
applies. It looks currently like this does something on all platforms, and
requires that the tool exists, so it would normally be inside a mac/iOS
conditional).

Done.

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/functions_targ...
tools/gn/functions_target.cc:277: "  \"create_bundle\" target).\n";
On 2016/03/02 at 21:12:42, brettw wrote:
> The other target types have a variable section at the bottom for what they
read. Can you add this to here and to bundle_data above and also mark which ones
are required? Like:
> 
>   "Variables\n"
>   "\n"
>   "bundle_root_dir (required), bundle_resources_dir\n" ...

Done.

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/ninja_create_b...
File tools/gn/ninja_create_bundle_target_writer.cc (right):

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/ninja_create_b...
tools/gn/ninja_create_bundle_target_writer.cc:38: if
(!target_->toolchain()->GetTool(Toolchain::TYPE_COPY_BUNDLE_DATA)) {
On 2016/03/02 at 21:12:42, brettw wrote:
> We should have a unit test file for this like we do for the other writer
files.

Done.

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

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/target.cc#newc...
tools/gn/target.cc:483: void Target::PullRecursiveBundleData() {
On 2016/03/02 at 21:12:42, brettw wrote:
> We should have a unit test for this showing this getting pulled across some
tree of deps, but not across create_bundle targets. In target_unittest.cc there
are some tests like this you can model yours off of.

Done.

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

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/variables.cc#n...
tools/gn/variables.cc:443: "  A string corresponding to a path in
$root_build_dir.\n"
On 2016/03/02 at 21:12:42, brettw wrote:
> Don't use the $ here since the $ sign just does variable string expansion
rather than referring to variables.

Done.

https://codereview.chromium.org/1752033002/diff/40001/tools/gn/variables.cc#n...
tools/gn/variables.cc:447: "  This must correspond to a path under
$root_build_dir.\n"
On 2016/03/02 at 21:12:42, brettw wrote:
> No $ here and in cases below.

Done.

[brettw, 2016-03-07 21:57:30]

I still don't see a test for NinjaCreateBundleTargetWriter. The other target
writers test that the Ninja code looks like some predefined thing. This
especially makes it easier to review what the code is doing (and in the future,
which changes are being made).

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

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/bundle_data.cc...
tools/gn/bundle_data.cc:68: DCHECK(sources);
I don't usually DCHECK these things. In general pointers shouldn't be NULL, and
these are among the easiest types of errors to debug. (same below).

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/bundle_data.cc...
tools/gn/bundle_data.cc:85: for (const SourceFile& source_file :
outputs_as_sources) {
No {}

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/create_bundle_...
File tools/gn/create_bundle_target_generator.cc (right):

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/create_bundle_...
tools/gn/create_bundle_target_generator.cc:56: str, value->origin(), err_))
Did clang indent this? I'd expect this to be moved to the right under the
GetBuildSettings call on the previuos line.

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

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:878: // All tools except the copy and stamp tools
should have outputs. The copy
This comment needs updating.

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

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:259: "  \"outputs\". If there are multiple files,
source patterns can be used.\n"
"If there are multiple files, source patterns can be used" -> "If there are
mutliple sources, source expansions must be used to express the output."

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:260: "  All the output path needs to be relative to
{{bundle_root_dir}}.\n"
This should either say "the output is treated as relative to the
{{bundle_root_dir}}" or "the output must reference a file inside of
{{bundle_root_dir}}" depending on whether you're expecting the path to be
referenced or not (I suspect the latter).

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:291: "     
\"src/MaterialTypography.bundle/Roboto-BlackItalic.ttf\",\n"
I think 2-4 ttf files should be enough for this example to avoid making things
too long.

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:327: "  well know structure). This target does not
define any sources, instead\n"
"well know" -> "well-known"

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:329: "  transitively (the recursion stops at each
\"create_bundle\" targets).\n"
Remove "each"

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:331: "  This target needs to define the value used
for the expansion of the\n"
Can you express this a different way? I found this paragraph confusing.

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:336: "  generate iOS/OS X bundle. It is advised to
put it behind \"is_ios\" or\n"
is_ios and is_mac aren't built-in variables (they're defined by the Chrome
build), so I would say something like "In cross-platform projects, it is advised
to put it behind iOS/Mac conditionals."

[sdefresne, 2016-03-08 14:33:31]

> I still don't see a test for NinjaCreateBundleTargetWriter. The
> other target writers test that the Ninja code looks like some
> predefined thing. This especially makes it easier to review what
> the code is doing (and in the future, which changes are being made).

Sorry about the missing test. I did write them but forgot to run
"git add" before sending the CL for review (I really wish that
"git cl upload" would warn about untracked files).

I've now included them and addressed the comments of that second
round. Would you mind doing another review?

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

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/bundle_data.cc...
tools/gn/bundle_data.cc:68: DCHECK(sources);
On 2016/03/07 at 21:57:30, brettw wrote:
> I don't usually DCHECK these things. In general pointers shouldn't be NULL,
and these are among the easiest types of errors to debug. (same below).

Ack.

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/bundle_data.cc...
tools/gn/bundle_data.cc:85: for (const SourceFile& source_file :
outputs_as_sources) {
On 2016/03/07 at 21:57:30, brettw wrote:
> No {}

Ack.

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/create_bundle_...
File tools/gn/create_bundle_target_generator.cc (right):

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/create_bundle_...
tools/gn/create_bundle_target_generator.cc:56: str, value->origin(), err_))
On 2016/03/07 at 21:57:30, brettw wrote:
> Did clang indent this? I'd expect this to be moved to the right under the
GetBuildSettings call on the previuos line.

No, I forgot to run "git cl format", fixed.

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

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/function_toolc...
tools/gn/function_toolchain.cc:878: // All tools except the copy and stamp tools
should have outputs. The copy
On 2016/03/07 at 21:57:30, brettw wrote:
> This comment needs updating.

Done.

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

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:259: "  \"outputs\". If there are multiple files,
source patterns can be used.\n"
On 2016/03/07 at 21:57:30, brettw wrote:
> "If there are multiple files, source patterns can be used" -> "If there are
mutliple sources, source expansions must be used to express the output."

Done.

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:260: "  All the output path needs to be relative to
{{bundle_root_dir}}.\n"
On 2016/03/07 at 21:57:30, brettw wrote:
> This should either say "the output is treated as relative to the
{{bundle_root_dir}}" or "the output must reference a file inside of
{{bundle_root_dir}}" depending on whether you're expecting the path to be
referenced or not (I suspect the latter).

Done.

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:291: "     
\"src/MaterialTypography.bundle/Roboto-BlackItalic.ttf\",\n"
On 2016/03/07 at 21:57:30, brettw wrote:
> I think 2-4 ttf files should be enough for this example to avoid making things
too long.

Ack.

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:327: "  well know structure). This target does not
define any sources, instead\n"
On 2016/03/07 at 21:57:30, brettw wrote:
> "well know" -> "well-known"

Done.

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:329: "  transitively (the recursion stops at each
\"create_bundle\" targets).\n"
On 2016/03/07 at 21:57:30, brettw wrote:
> Remove "each"

Done.

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:331: "  This target needs to define the value used
for the expansion of the\n"
On 2016/03/07 at 21:57:30, brettw wrote:
> Can you express this a different way? I found this paragraph confusing.

Is it better now?

https://codereview.chromium.org/1752033002/diff/60001/tools/gn/functions_targ...
tools/gn/functions_target.cc:336: "  generate iOS/OS X bundle. It is advised to
put it behind \"is_ios\" or\n"
On 2016/03/07 at 21:57:30, brettw wrote:
> is_ios and is_mac aren't built-in variables (they're defined by the Chrome
build), so I would say something like "In cross-platform projects, it is advised
to put it behind iOS/Mac conditionals."

Done.

[sdefresne, 2016-03-10 03:51:54]

Ping?

[brettw, 2016-03-10 19:09:49]

LGTM (was on vacation for last 2 days)

[sdefresne, 2016-03-10 19:13:56]

On 2016/03/10 at 19:09:49, brettw wrote:
> LGTM (was on vacation for last 2 days)

Ah sorry, I should have checked. Thank you for the review :-)

[sdefresne, 2016-03-10 19:13:59]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2016-03-10 19:15:07]

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

[commit-bot: I haz the power, 2016-03-10 20:16:45]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-03-10 20:17:48]

Description was changed from

==========
Add "create_bundle" target in order to support bundle with gn.

The "create_bundle" target recursively collect information from all the
"bundle_data" targets it depends and create a bundle from them. This is
configurable in many way to support all kinds of bundles on OS X or iOS.

Design:
https://docs.google.com/document/d/1bKh57hg6TSBsEmeh0zWpOO5SVGA2INu-D3FGgsyIC...

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

to

==========
Add "create_bundle" target in order to support bundle with gn.

The "create_bundle" target recursively collect information from all the
"bundle_data" targets it depends and create a bundle from them. This is
configurable in many way to support all kinds of bundles on OS X or iOS.

Design:
https://docs.google.com/document/d/1bKh57hg6TSBsEmeh0zWpOO5SVGA2INu-D3FGgsyIC...

BUG=297668

Committed: https://crrev.com/1232aaeb49f5ec11e0e9fcc527bfa96de21904ca
Cr-Commit-Position: refs/heads/master@{#380454}
==========

[commit-bot: I haz the power, 2016-03-10 20:17:49]

Patchset 4 (id:??) landed as
https://crrev.com/1232aaeb49f5ec11e0e9fcc527bfa96de21904ca
Cr-Commit-Position: refs/heads/master@{#380454}