Consider bundle_data as public_deps of create_bundle targets.

Consider bundle_data as public_deps of create_bundle targets.

The create_bundle and bundle_data targets works as a pair to copy
files into iOS/OS X bundles. The bundle_data target defines the
files to copy and the destination in the bundle, the create_bundle
target generate the copy rule (as the destination cannot be resolved
earlier).

This cause a problem with generated files, as gn prevents a target
using as input a generated file unless there is a direct dependency
or a chain of public dependencies. Since bundle_data/create_bundle
are designed to be resolved recursively, update this check to consider
all bundle_data as public_deps of create_bundle targets.

Convert Target::PullRecursiveBundleData() to O(n) algorithm instead
of a O(n^2) by percolating the dependencies up.

BUG=297668
Committed: https://crrev.com/1cc4b1ca2085776730f92ab41173c6afd9d51d12
Cr-Commit-Position: refs/heads/master@{#383049}

[sdefresne, 2016-03-16 16:37:34]

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

[sdefresne, 2016-03-16 16:37:35]

When trying to get //ios/web/ios_web_shell to build, I found that the following
dependency:

ERROR Input to targets not generated by a dependency.
The file:
  //out/Default/gen/ios/web/window_id.js
is listed as an input or source for the targets:
  //components:components_unittests
  //ios/web:ios_web_unittests
  //ios/web/shell:ios_web_shell
but this file was not generated by any dependencies of the targets. The target
that generates the file is:
  //ios/web:js_resources_compile_js

When printing the dependencies, we have:

$ gn path out/Default //ios/web/shell:ios_web_shell
//ios/web:js_resources_compile_js
//ios/web/shell:ios_web_shell --[private]-->
//ios/web/shell:shell --[private]-->
//ios/web:web --[private]-->
//ios/web:js_resources --[public]-->
//ios/web:js_resources_compile_js

//ios/web:js_resources is a bundle_data target and //ios/web/shell:ios_web_shell
is a create_bundle. With this CL, the dependency is considered legal, without it
causes the error above.

What do you think of this CL? Should I also update "gn path" to add the shortcut
via the bundle_data/create_bundle pair?

[brettw, 2016-03-16 17:27:44]

https://codereview.chromium.org/1804263003/diff/1/tools/gn/target.cc
File tools/gn/target.cc (left):

https://codereview.chromium.org/1804263003/diff/1/tools/gn/target.cc#oldcode511
tools/gn/target.cc:511: deps.push_back(pair.ptr);
I was studying Target::PullRecursiveBundleData more in the context of this patch
and I think we need to change how this works. Currently, this does a dull
dependency walk for every target, which I think is O(n^2).

We should instead track the recursive bundle data for every target and just push
it up the tree as we go, merging the lists at each level. PullRecursiveHardDeps
does this and I think this code can easily be made to work the same way. You can
just use Target* pointers (rather than the LabelPtrPair) which are also faster
to deal with (LabelPtrPair is required for deps because we this is used to track
deps before the labels have been resolved to targets).

This will also give us a list on each target of the recursive bundle data which
I think should solve the problem you're trying to solve by adding deps to bundle
data.

[brettw, 2016-03-16 17:27:44]

https://codereview.chromium.org/1804263003/diff/1/tools/gn/target.cc
File tools/gn/target.cc (left):

https://codereview.chromium.org/1804263003/diff/1/tools/gn/target.cc#oldcode511
tools/gn/target.cc:511: deps.push_back(pair.ptr);
I was studying Target::PullRecursiveBundleData more in the context of this patch
and I think we need to change how this works. Currently, this does a dull
dependency walk for every target, which I think is O(n^2).

We should instead track the recursive bundle data for every target and just push
it up the tree as we go, merging the lists at each level. PullRecursiveHardDeps
does this and I think this code can easily be made to work the same way. You can
just use Target* pointers (rather than the LabelPtrPair) which are also faster
to deal with (LabelPtrPair is required for deps because we this is used to track
deps before the labels have been resolved to targets).

This will also give us a list on each target of the recursive bundle data which
I think should solve the problem you're trying to solve by adding deps to bundle
data.

[sdefresne, 2016-03-17 10:36:22]

Description was changed from

==========
Consider bundle_data as public_deps of create_bundle targets.

The create_bundle and bundle_data targets works as a pair to copy
files into iOS/OS X bundles. The bundle_data target defines the
files to copy and the destination in the bundle, the create_bundle
target generate the copy rule (as the destination cannot be resolved
earlier).

This cause a problem with generated files, as gn prevents a target
using as input a generated file unless there is a direct dependency
or a chain of public dependencies. Since bundle_data/create_bundle
are designed to be resolved recursively, update this check to consider
all bundle_data as public_deps of create_bundle targets.

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

to

==========
Consider bundle_data as public_deps of create_bundle targets.

The create_bundle and bundle_data targets works as a pair to copy
files into iOS/OS X bundles. The bundle_data target defines the
files to copy and the destination in the bundle, the create_bundle
target generate the copy rule (as the destination cannot be resolved
earlier).

This cause a problem with generated files, as gn prevents a target
using as input a generated file unless there is a direct dependency
or a chain of public dependencies. Since bundle_data/create_bundle
are designed to be resolved recursively, update this check to consider
all bundle_data as public_deps of create_bundle targets.

Convert Target::PullRecursiveBundleData() to O(n) algorithm instead
of a O(n^2) by percolating the dependencies up.

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

[sdefresne, 2016-03-17 10:39:58]

Done, please take another look.

[sdefresne, 2016-03-19 14:00:19]

Ping?

[brettw, 2016-03-21 22:49:47]

We should get a unit test for this new behavior.

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

https://codereview.chromium.org/1804263003/diff/40001/tools/gn/bundle_data.h#...
tools/gn/bundle_data.h:80: UniqueVector<const Target*>& deps() { return deps_; }
Can you call this "bundle_deps" and comment that these are the recursive
collection of bundle_data targets that this target depends on?

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

https://codereview.chromium.org/1804263003/diff/40001/tools/gn/target.cc#newc...
tools/gn/target.cc:494: // Stop recursion at create_bundle targets.
This would be more clear to me as something like "Don't propagate bundle data
once they're added to a bundle." (or something). The thing that threw me off
here is that this code isn't actually doing recursion any more.

https://codereview.chromium.org/1804263003/diff/40001/tools/gn/target.cc#newc...
tools/gn/target.cc:501: 
Extra blank line in here.

https://codereview.chromium.org/1804263003/diff/40001/tools/gn/target.cc#newc...
tools/gn/target.cc:512: bundle_data_.AddFileRuleFromTarget(target);
Can this function be changed now? It's weird to pass in a member of a class to a
function of that same class from outside as the only invocation of this
function.

I think this is the only place where bundle_data.deps is written to. It might be
better to make this set read only and have a function
AddBundleData(UniqueVector* swapped_in_bundle_data) or maybe even better would
be to just have AddBundleData(const Target*) that does everything for one
target.

[brettw, 2016-03-21 22:59:08]

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

https://codereview.chromium.org/1804263003/diff/40001/tools/gn/target.cc#newc...
tools/gn/target.cc:512: bundle_data_.AddFileRuleFromTarget(target);
On 2016/03/21 22:49:47, brettw wrote:
> Can this function be changed now? It's weird to pass in a member of a class to
a
> function of that same class from outside as the only invocation of this
> function.
> 
> I think this is the only place where bundle_data.deps is written to. It might
be
> better to make this set read only and have a function
> AddBundleData(UniqueVector* swapped_in_bundle_data) or maybe even better would
> be to just have AddBundleData(const Target*) that does everything for one
> target.

By this I mean append that target to the bundle_deps and if it's new, do the
stuff that AddFileRuleFromTarget did before

[sdefresne, 2016-03-22 12:34:05]

Please take another look.

Regarding unit tests, did you mean check that bundle_deps is correctly
initialized (in that case, I've added the corresponding checks to the existing
Target.PullRecursiveBundleData test) or did you want to me test
Target::CheckSourcesGenerated? In the later case, there is no existing test, so
I'll have to write it from scratch.

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

https://codereview.chromium.org/1804263003/diff/40001/tools/gn/bundle_data.h#...
tools/gn/bundle_data.h:80: UniqueVector<const Target*>& deps() { return deps_; }
On 2016/03/21 at 22:49:47, brettw wrote:
> Can you call this "bundle_deps" and comment that these are the recursive
collection of bundle_data targets that this target depends on?

done.

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

https://codereview.chromium.org/1804263003/diff/40001/tools/gn/target.cc#newc...
tools/gn/target.cc:512: bundle_data_.AddFileRuleFromTarget(target);
On 2016/03/21 at 22:59:08, brettw wrote:
> On 2016/03/21 22:49:47, brettw wrote:
> > Can this function be changed now? It's weird to pass in a member of a class
to a
> > function of that same class from outside as the only invocation of this
> > function.
> > 
> > I think this is the only place where bundle_data.deps is written to. It
might be
> > better to make this set read only and have a function
> > AddBundleData(UniqueVector* swapped_in_bundle_data) or maybe even better
would
> > be to just have AddBundleData(const Target*) that does everything for one
> > target.
> 
> By this I mean append that target to the bundle_deps and if it's new, do the
stuff that AddFileRuleFromTarget did before

There was a small optimisation that this removed (only Target::CREATE_BUNDLE
cares about file_rules_ and asset_catalog_sources_).

To avoid initializing those fields for all target, I've added a separate method
OnTargetResolved(). It also sets the target sources() field for
Target::CREATE_BUNDLE.

WDYT?

[brettw, 2016-03-23 23:18:24]

LGTM. For my previous comment about tests I was mostly concerned about the
propagation of bundle data, but I forgot that's already covered.

[sdefresne, 2016-03-24 09:55:48]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2016-03-24 09:55:56]

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

[commit-bot: I haz the power, 2016-03-24 10:25:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-24 10:27:36]

Description was changed from

==========
Consider bundle_data as public_deps of create_bundle targets.

The create_bundle and bundle_data targets works as a pair to copy
files into iOS/OS X bundles. The bundle_data target defines the
files to copy and the destination in the bundle, the create_bundle
target generate the copy rule (as the destination cannot be resolved
earlier).

This cause a problem with generated files, as gn prevents a target
using as input a generated file unless there is a direct dependency
or a chain of public dependencies. Since bundle_data/create_bundle
are designed to be resolved recursively, update this check to consider
all bundle_data as public_deps of create_bundle targets.

Convert Target::PullRecursiveBundleData() to O(n) algorithm instead
of a O(n^2) by percolating the dependencies up.

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

to

==========
Consider bundle_data as public_deps of create_bundle targets.

The create_bundle and bundle_data targets works as a pair to copy
files into iOS/OS X bundles. The bundle_data target defines the
files to copy and the destination in the bundle, the create_bundle
target generate the copy rule (as the destination cannot be resolved
earlier).

This cause a problem with generated files, as gn prevents a target
using as input a generated file unless there is a direct dependency
or a chain of public dependencies. Since bundle_data/create_bundle
are designed to be resolved recursively, update this check to consider
all bundle_data as public_deps of create_bundle targets.

Convert Target::PullRecursiveBundleData() to O(n) algorithm instead
of a O(n^2) by percolating the dependencies up.

BUG=297668

Committed: https://crrev.com/1cc4b1ca2085776730f92ab41173c6afd9d51d12
Cr-Commit-Position: refs/heads/master@{#383049}
==========

[commit-bot: I haz the power, 2016-03-24 10:27:40]

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