Fix a dependency issue w/ gn refs for android apks.

Fix a dependency issue w/ gn refs for android apks.

One of the intermediate steps in the build process for
creating Android APKs depends on the output from another
intermediate step, but that dependency was not *also* declared
in the 'deps' target in the GN build file.

As a result, 'gn refs' (and hence 'mb analyze') would not calculate
the right set of dependencies and we would incorrectly conclude that
no compiles were necessary in cases where they really were needed.

More details in the bug.

R=cjhopman@chromium.org, brettw@chromium.org
BUG=487897
Committed: https://crrev.com/0cc8f08b75ed52834b3a8a73bb645865abd9fc55
Cr-Commit-Position: refs/heads/master@{#330152}

[Dirk Pranke, 2015-05-14 03:02:52]

You can see the result of this change in 

http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...

It *almost* worked, but unfortunately I forgot that "chrome_shell_apk" doesn't
exist as an output file and I need to have some way to map "chrome_shell_apk" to
"//chrome/android:chrome_shell_apk" or "chrome_shell_apk.stamp".

Still, I think this change is probably at least part of the solution.

Thoughts?

[brettw, 2015-05-14 16:42:52]

https://codereview.chromium.org/1129773004/diff/1/build/config/android/intern...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1129773004/diff/1/build/config/android/intern...
build/config/android/internal_rules.gni:620: # TODO: crbug.com/487897 -
__finalize needs as input a file created
I don't understand this comment. As we talked about, "gn refs" isn't broken and
we're not working around it, and there's nothing to fix in the future. If you
have a dependency, you should declare it in the deps.

[Dirk Pranke, 2015-05-14 17:27:06]

https://codereview.chromium.org/1129773004/diff/1/build/config/android/intern...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1129773004/diff/1/build/config/android/intern...
build/config/android/internal_rules.gni:620: # TODO: crbug.com/487897 -
__finalize needs as input a file created
On 2015/05/14 16:42:51, brettw wrote:
> I don't understand this comment. As we talked about, "gn refs" isn't broken
and
> we're not working around it, and there's nothing to fix in the future. If you
> have a dependency, you should declare it in the deps.

Yeah, reading it in the morning, I don't think this comment captured the right
thing.

I was trying to capture two different thoughts, I think. 

The first was an explanation of why this change was needed, which probably
could've been a comment on the CL rather than in the code, or it could be a
comment in the code, but just not a TODO.

The second thing was to capture the idea that when 'gn check' will be able to
check that inputs are not declared as dependencies, we should make sure that
this particular one is caught correctly. That was the "TODO" part.

Does that make sense?

[brettw, 2015-05-14 20:02:54]

On 2015/05/14 17:27:06, Dirk Pranke wrote:
>
https://codereview.chromium.org/1129773004/diff/1/build/config/android/intern...
> File build/config/android/internal_rules.gni (right):
> 
>
https://codereview.chromium.org/1129773004/diff/1/build/config/android/intern...
> build/config/android/internal_rules.gni:620: # TODO: crbug.com/487897 -
> __finalize needs as input a file created
> On 2015/05/14 16:42:51, brettw wrote:
> > I don't understand this comment. As we talked about, "gn refs" isn't broken
> and
> > we're not working around it, and there's nothing to fix in the future. If
you
> > have a dependency, you should declare it in the deps.
> 
> Yeah, reading it in the morning, I don't think this comment captured the right
> thing.
> 
> I was trying to capture two different thoughts, I think. 
> 
> The first was an explanation of why this change was needed, which probably
> could've been a comment on the CL rather than in the code, or it could be a
> comment in the code, but just not a TODO.

Generally comments should describe the current state, and information about the
transition of how that got there should be in the commit message. Otherwise in a
year (or more like a week) the comment won't make any sense because nobody will
have context on what it used to do. So I'd move that part out to the commit
message.

> The second thing was to capture the idea that when 'gn check' will be able to
> check that inputs are not declared as dependencies, we should make sure that
> this particular one is caught correctly. That was the "TODO" part.

When I fix the issue (turns out I can make it part of the normal build rather
than just "gn check"), I'm definitely not going to go look here and remove the
TODO so I think adding it for that reason would be better avoided. Maybe add it
on the bug as something to check before closing it.

[Dirk Pranke, 2015-05-14 20:14:39]

On 2015/05/14 20:02:54, brettw wrote:
> On 2015/05/14 17:27:06, Dirk Pranke wrote:
> >
>
https://codereview.chromium.org/1129773004/diff/1/build/config/android/intern...
> > File build/config/android/internal_rules.gni (right):
> > 
> >
>
https://codereview.chromium.org/1129773004/diff/1/build/config/android/intern...
> > build/config/android/internal_rules.gni:620: # TODO: crbug.com/487897 -
> > __finalize needs as input a file created
> > On 2015/05/14 16:42:51, brettw wrote:
> > > I don't understand this comment. As we talked about, "gn refs" isn't
broken
> > and
> > > we're not working around it, and there's nothing to fix in the future. If
> you
> > > have a dependency, you should declare it in the deps.
> > 
> > Yeah, reading it in the morning, I don't think this comment captured the
right
> > thing.
> > 
> > I was trying to capture two different thoughts, I think. 
> > 
> > The first was an explanation of why this change was needed, which probably
> > could've been a comment on the CL rather than in the code, or it could be a
> > comment in the code, but just not a TODO.
> 
> Generally comments should describe the current state, and information about
the
> transition of how that got there should be in the commit message. Otherwise in
a
> year (or more like a week) the comment won't make any sense because nobody
will
> have context on what it used to do. So I'd move that part out to the commit
> message.

Sure, agreed.

> > The second thing was to capture the idea that when 'gn check' will be able
to
> > check that inputs are not declared as dependencies, we should make sure that
> > this particular one is caught correctly. That was the "TODO" part.
> 
> When I fix the issue (turns out I can make it part of the normal build rather
> than just "gn check"), I'm definitely not going to go look here and remove the
> TODO so I think adding it for that reason would be better avoided. Maybe add
it
> on the bug as something to check before closing it.

Ok, I kinda remembered you had wanted me to put in a todo or something for this.

[Dirk Pranke, 2015-05-14 20:53:57]

Okay, given that you've sent me the patch to test, it looks like there's no TODO
here at all. I've removed the comment.

Please take another look?

[brettw, 2015-05-14 22:58:50]

lgtm

lgtm

[brettw, 2015-05-14 22:58:51]

lgtm

[cjhopman, 2015-05-15 18:08:05]

lgtm

[Dirk Pranke, 2015-05-15 18:20:56]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-05-15 18:22:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129773004/20001

[commit-bot: I haz the power, 2015-05-15 18:58:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-15 18:59:47]

Patchset 2 (id:??) landed as
https://crrev.com/0cc8f08b75ed52834b3a8a73bb645865abd9fc55
Cr-Commit-Position: refs/heads/master@{#330152}

[agrieve, 2015-05-15 23:54:56]

On 2015/05/15 18:59:47, I haz the power (commit-bot) wrote:
> Patchset 2 (id:??) landed as
> https://crrev.com/0cc8f08b75ed52834b3a8a73bb645865abd9fc55
> Cr-Commit-Position: refs/heads/master@{#330152}

I'm not sure this captures the dependencies correctly... What I think it should
be:
- package_resources is a dependency of __package
- __package is a dependency of __finalize

So, I think the right fix here is to add:

    deps = [ ":${target_name}__package_resources" ]

to the __package rule

[Dirk Pranke, 2015-05-15 23:59:30]

On 2015/05/15 23:54:56, agrieve wrote:
> I'm not sure this captures the dependencies correctly... What I think it
should
> be:

You're correct that it doesn't capture the dependencies correctly; it's a crude
approximation.

There's actually several missing dependencies. The one you suggest is one of
them. We'll
fix it and the others in a follow-up patch, as part of turning on the actual
checks for this
in GN (you can star the associated bug if you're curious).