Turn Chrome Shell's AndroidManifest into a jinja2 template.

Turn Chrome Shell's AndroidManifest into a jinja2 template.

This turns Chrome Shell's AndroidManifest.xml into a template which gets
processed at build time to produce the final AndroidManifest.xml that's
used to build the APK.

BUG=163751
NOTRY=true

Committed: https://crrev.com/d5366cb1ad27e6bf1ae659f374a9b9dd3186761d
Cr-Commit-Position: refs/heads/master@{#300409}

[newt (away), 2014-10-14 03:33:53]

newt@chromium.org changed reviewers:
+ cjhopman@chromium.org

[newt (away), 2014-10-14 03:33:54]

WDYT?

The other option, as I mentioned in the bug, is to make template processing a
step inside of java_apk.gypi

If we go with this solution, then I should create a process_template.gypi file,
which can be included here and in other places when template processing is
needed.

[cjhopman, 2014-10-14 18:36:35]

On 2014/10/14 03:33:54, newt wrote:
> WDYT?
> 
> The other option, as I mentioned in the bug, is to make template processing a
> step inside of java_apk.gypi
> 
> If we go with this solution, then I should create a process_template.gypi
file,
> which can be included here and in other places when template processing is
> needed.

This'll be great. I'd stick with the processing outside of java_apk.gypi.

[newt (away), 2014-10-17 15:54:53]

PTAL

[cjhopman, 2014-10-17 18:54:26]

https://codereview.chromium.org/657443002/diff/20001/build/android/gyp/jinja_...
File build/android/gyp/jinja_template.py (right):

https://codereview.chromium.org/657443002/diff/20001/build/android/gyp/jinja_...
build/android/gyp/jinja_template.py:36: if relpath.startswith(os.pardir):
Because you asked before, here's another approach:

os.path.abspath(input_filename).startswith(os.path.abspath(inputs_base_dir)

https://codereview.chromium.org/657443002/diff/20001/build/config/android/rul...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/657443002/diff/20001/build/config/android/rul...
build/config/android/rules.gni:414: base_path = "$target_gen_dir/$target_name"
for these variables set at the top level of the template, prefix them with _ and
then explicitly provide them for the invocations below that use them.

https://codereview.chromium.org/657443002/diff/20001/build/config/android/rul...
build/config/android/rules.gni:420: # Why?? I just copy-pasted this
You shouldn't need to forward the deps. Since deps have no affect on how this is
built, just not supporting deps seems fine to me.

https://codereview.chromium.org/657443002/diff/20001/build/config/android/rul...
build/config/android/rules.gni:801: ":${target_name}__dex",
sort.

https://codereview.chromium.org/657443002/diff/20001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/657443002/diff/20001/chrome/android/BUILD.gn#...
chrome/android/BUILD.gn:10: chrome_shell_manifest =
"$target_gen_dir/chrome_shell_manifest/AndroidManifest.xml"
Move this down near where it's used.

https://codereview.chromium.org/657443002/diff/20001/chrome/android/BUILD.gn#...
chrome/android/BUILD.gn:355: testonly = true  # is this needed?
this isn't needed (but it doesn't hurt).

gn will throw an error if any testonly=false thing depends on a testonly=true
thing. I'm not sure why chrome_shell_apk has testonly at all (content_shell_apk
has it because content_shell is testonly). I don't know if I consider
chrome_shell_apk a test-thing or not (but since content_shell is considered a
test thing I guess chrome shell should be too).

since this doesn't depend on anything testonly isn't required, adding it here
will just enforce that everything that depends on it is testonly (and thus
ensure that it won't be included in a production build of anything).

[newt (away), 2014-10-20 22:45:12]

https://codereview.chromium.org/657443002/diff/20001/build/android/gyp/jinja_...
File build/android/gyp/jinja_template.py (right):

https://codereview.chromium.org/657443002/diff/20001/build/android/gyp/jinja_...
build/android/gyp/jinja_template.py:36: if relpath.startswith(os.pardir):
On 2014/10/17 18:54:25, cjhopman wrote:
> Because you asked before, here's another approach:
> 
> os.path.abspath(input_filename).startswith(os.path.abspath(inputs_base_dir)

Good point. Though I'd need to ensure that os.path.abspath(inputs_base_dir) ends
with "/"

https://codereview.chromium.org/657443002/diff/20001/build/config/android/rul...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/657443002/diff/20001/build/config/android/rul...
build/config/android/rules.gni:414: base_path = "$target_gen_dir/$target_name"
On 2014/10/17 18:54:25, cjhopman wrote:
> for these variables set at the top level of the template, prefix them with _
and
> then explicitly provide them for the invocations below that use them.

Done.

https://codereview.chromium.org/657443002/diff/20001/build/config/android/rul...
build/config/android/rules.gni:420: # Why?? I just copy-pasted this
On 2014/10/17 18:54:25, cjhopman wrote:
> You shouldn't need to forward the deps. Since deps have no affect on how this
is
> built, just not supporting deps seems fine to me.

Done.

https://codereview.chromium.org/657443002/diff/20001/build/config/android/rul...
build/config/android/rules.gni:801: ":${target_name}__dex",
On 2014/10/17 18:54:25, cjhopman wrote:
> sort.

Done.

https://codereview.chromium.org/657443002/diff/20001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/657443002/diff/20001/chrome/android/BUILD.gn#...
chrome/android/BUILD.gn:10: chrome_shell_manifest =
"$target_gen_dir/chrome_shell_manifest/AndroidManifest.xml"
On 2014/10/17 18:54:25, cjhopman wrote:
> Move this down near where it's used.

Done.

https://codereview.chromium.org/657443002/diff/20001/chrome/android/BUILD.gn#...
chrome/android/BUILD.gn:355: testonly = true  # is this needed?
On 2014/10/17 18:54:25, cjhopman wrote:
> this isn't needed (but it doesn't hurt).
> 
> gn will throw an error if any testonly=false thing depends on a testonly=true
> thing. I'm not sure why chrome_shell_apk has testonly at all
(content_shell_apk
> has it because content_shell is testonly). I don't know if I consider
> chrome_shell_apk a test-thing or not (but since content_shell is considered a
> test thing I guess chrome shell should be too).
> 
> since this doesn't depend on anything testonly isn't required, adding it here
> will just enforce that everything that depends on it is testonly (and thus
> ensure that it won't be included in a production build of anything).

Ok, keeping testonly

[newt (away), 2014-10-20 22:46:43]

newt@chromium.org changed reviewers:
+ dtrainor@chromium.org

[newt (away), 2014-10-20 22:46:45]

dtrainor: chrome/android/ (if you trust cjhopman)

[David Trainor- moved to gerrit, 2014-10-20 23:00:25]

lgtm

[cjhopman, 2014-10-20 23:11:57]

lgtm

https://chromiumcodereview.appspot.com/657443002/diff/40001/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/657443002/diff/40001/build/common.gypi...
build/common.gypi:152: 'android_channel%': 'default',
This doesn't seem to be used anywhere. If it's just used downstream, let's
declare it there.

[newt (away), 2014-10-20 23:23:44]

On 2014/10/20 23:11:57, cjhopman wrote:
> lgtm
> 
> https://chromiumcodereview.appspot.com/657443002/diff/40001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://chromiumcodereview.appspot.com/657443002/diff/40001/build/common.gypi...
> build/common.gypi:152: 'android_channel%': 'default',
> This doesn't seem to be used anywhere. If it's just used downstream, let's
> declare it there.

I have machinations to use it upstream...

[newt (away), 2014-10-20 23:23:52]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-10-20 23:26:35]

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

[commit-bot: I haz the power, 2014-10-20 23:38:09]

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

[commit-bot: I haz the power, 2014-10-20 23:38:09]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[newt (away), 2014-10-21 00:33:10]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-10-21 00:34:32]

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

[newt (away), 2014-10-21 00:49:23]

The CQ bit was unchecked by newt@chromium.org

[newt (away), 2014-10-21 00:52:05]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-10-21 00:52:59]

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

[commit-bot: I haz the power, 2014-10-21 02:56:18]

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

[commit-bot: I haz the power, 2014-10-21 02:56:19]

Exceeded time limit waiting for builds to trigger.

[newt (away), 2014-10-21 03:23:52]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-10-21 03:24:23]

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

[commit-bot: I haz the power, 2014-10-21 03:27:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-21 03:30:13]

Patchset 5 (id:??) landed as
https://crrev.com/d5366cb1ad27e6bf1ae659f374a9b9dd3186761d
Cr-Commit-Position: refs/heads/master@{#300409}