[remoting android] Make Android credits a GN template.

[remoting android] Make Android credits a GN template.

BUG=697128
Review-Url: https://codereview.chromium.org/2763623003
Cr-Commit-Position: refs/heads/master@{#459335}
Committed: https://chromium.googlesource.com/chromium/src/+/6d34234ffabc40b20a7e7e82794d3f7dc06f13b4

[Lambros, 2017-03-20 23:53:40]

Patchset #1 (id:1) has been deleted

[Lambros, 2017-03-21 00:58:11]

lambroslambrou@chromium.org changed reviewers:
+ agrieve@chromium.org, jamiewalch@chromium.org

[Lambros, 2017-03-21 00:58:11]

jamiewalch: PTAL
agrieve: internal_rules.gni rubberstamp, maybe also take a quick look at my GN
code, see if it can be simplified?

I'm not sure the templating is strictly necessary (because we might be able to
get away with switching the target name on is_chrome_branded instead). But I
think it's tidier, and lays some groundwork in case we wanted to re-use the
credits template elsewhere.

[agrieve, 2017-03-21 02:07:14]

lgtm for internal_rules.gni (thanks!)

https://codereview.chromium.org/2763623003/diff/80001/remoting/android/BUILD.gn
File remoting/android/BUILD.gn (right):

https://codereview.chromium.org/2763623003/diff/80001/remoting/android/BUILD....
remoting/android/BUILD.gn:89: ":credits_resources_raw",
It's a little bit bad form to depend on a generated target name from outside of
the template that defines it. Instead, I'd recommend:

template("remoting_android_credits") {
  action("${target_name}__generate") {
    ...
  }
  copy(target_name) {
    deps = [ ":${target_name}__generate" ]
  }
}

That said... I think it would be cleaner to define this android_resources()
target in the same place as the copy (since they both point to the same
directory & list of files). So, I'd either move the copy out, or move the
android_resources() in (and convert this target to a java_group() that just
depends on it, if you need to maintain it)

https://codereview.chromium.org/2763623003/diff/80001/remoting/android/credit...
File remoting/android/credits_tmpl.gni (right):

https://codereview.chromium.org/2763623003/diff/80001/remoting/android/credit...
remoting/android/credits_tmpl.gni:11: _credits_html_file =
"$target_gen_dir/credits.html"
unintuitively, $target_gen_dir == "gen/remoting/android", so what you have here
would cause errors if two remoting_android_credits() existed in the same
BUILD.gn file.

Instead, do: 
_credits_html_file = "$target_gen_dir/$target_name/credits.html"

or:
_credits_html_file = "$target_gen_dir/$target_name-credits.html"

https://codereview.chromium.org/2763623003/diff/80001/remoting/android/credit...
remoting/android/credits_tmpl.gni:45: _copy_target_name = target_name +
"_resources_raw"
I think this will evaluate to "credits_resources_raw_resources_raw".

target_name changes within the scope of the copy target to be the copy target's
name.

https://codereview.chromium.org/2763623003/diff/80001/remoting/android/credit...
remoting/android/credits_tmpl.gni:49: ] + get_target_outputs(":$target_name")
Might likewise be an issue here.

[Lambros, 2017-03-22 21:31:00]

PTAL

agrieve: I kept the 'copy' target inside the template, but I've added a template
parameter to specify the location of the generated files. I think this addresses
your concern, and it seems cleaner to me.

jamiewalch: There isn't really anything specific to Android in the credits
template, so maybe we could move it to remoting/base. WDYT?

My plan is to use the new credits template in the Official build to generate
separate credits for remoting_google_apk (the official build target not present
in open-source checkouts). Then use a trick that agrieve@ told me about to
override the resources (a trick used by Android Chrome to replace icons with
official versions). This would mean that the credits get generated for both
remoting_apk and remoting_google_apk, but I think this is much cleaner than
alternative approaches.

https://codereview.chromium.org/2763623003/diff/80001/remoting/android/BUILD.gn
File remoting/android/BUILD.gn (right):

https://codereview.chromium.org/2763623003/diff/80001/remoting/android/BUILD....
remoting/android/BUILD.gn:89: ":credits_resources_raw",
On 2017/03/21 02:07:13, agrieve wrote:
> It's a little bit bad form to depend on a generated target name from outside
of
> the template that defines it. Instead, I'd recommend:
> 
> template("remoting_android_credits") {
>   action("${target_name}__generate") {
>     ...
>   }
>   copy(target_name) {
>     deps = [ ":${target_name}__generate" ]
>   }
> }
> 
> That said... I think it would be cleaner to define this android_resources()
> target in the same place as the copy (since they both point to the same
> directory & list of files). So, I'd either move the copy out, or move the
> android_resources() in (and convert this target to a java_group() that just
> depends on it, if you need to maintain it)

Done (added __generate to the action target)

https://codereview.chromium.org/2763623003/diff/80001/remoting/android/credit...
File remoting/android/credits_tmpl.gni (right):

https://codereview.chromium.org/2763623003/diff/80001/remoting/android/credit...
remoting/android/credits_tmpl.gni:11: _credits_html_file =
"$target_gen_dir/credits.html"
On 2017/03/21 02:07:13, agrieve wrote:
> unintuitively, $target_gen_dir == "gen/remoting/android", so what you have
here
> would cause errors if two remoting_android_credits() existed in the same
> BUILD.gn file.
> 
> Instead, do: 
> _credits_html_file = "$target_gen_dir/$target_name/credits.html"
> 
> or:
> _credits_html_file = "$target_gen_dir/$target_name-credits.html"

Done.

https://codereview.chromium.org/2763623003/diff/80001/remoting/android/credit...
remoting/android/credits_tmpl.gni:45: _copy_target_name = target_name +
"_resources_raw"
On 2017/03/21 02:07:13, agrieve wrote:
> I think this will evaluate to "credits_resources_raw_resources_raw".
> 
> target_name changes within the scope of the copy target to be the copy
target's
> name.

In my testing, this doesn't seem to be the case. target_name retains the
original name of the template instance, even inside a target scope inside the
template.

https://codereview.chromium.org/2763623003/diff/80001/remoting/android/credit...
remoting/android/credits_tmpl.gni:49: ] + get_target_outputs(":$target_name")
On 2017/03/21 02:07:13, agrieve wrote:
> Might likewise be an issue here.

Same here.

[Lambros, 2017-03-22 23:27:16]

I've moved the template to remoting/credits, since it will also be useful for
the iOS build, at least.

[agrieve, 2017-03-23 00:11:09]

On 2017/03/22 23:27:16, Lambros wrote:
> I've moved the template to remoting/credits, since it will also be useful for
> the iOS build, at least.

A mighty fine looking template :)

[Jamie, 2017-03-23 17:46:38]

lgtm

[Lambros, 2017-03-24 00:15:15]

The CQ bit was checked by lambroslambrou@chromium.org

[Lambros, 2017-03-24 00:15:16]

The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2763623003/#ps120001
(title: "Move credits template to remoting/credits")

[commit-bot: I haz the power, 2017-03-24 00:15:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 00:18:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 00:19:00]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Lambros, 2017-03-24 00:51:08]

The CQ bit was checked by lambroslambrou@chromium.org

[Lambros, 2017-03-24 00:51:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2763623003/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-24 00:51:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 01:58:07]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490316668902360,
"parent_rev": "2615ab3076703e2d4e409370ba32c46ab0aa170c", "commit_rev":
"6d34234ffabc40b20a7e7e82794d3f7dc06f13b4"}

[commit-bot: I haz the power, 2017-03-24 01:58:53]

Description was changed from

==========
[remoting android] Make Android credits a GN template.

BUG=697128
==========

to

==========
[remoting android] Make Android credits a GN template.

BUG=697128

Review-Url: https://codereview.chromium.org/2763623003
Cr-Commit-Position: refs/heads/master@{#459335}
Committed:
https://chromium.googlesource.com/chromium/src/+/6d34234ffabc40b20a7e7e82794d...
==========

[commit-bot: I haz the power, 2017-03-24 01:58:53]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/6d34234ffabc40b20a7e7e82794d...