[Android] Extract signing+zipaligning to python

[Android] Extract signing+zipaligning to python

Continued de-antification. This one is pretty straightforward.

BUG=158821
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193160

[shashi, 2013-04-08 22:39:04]

lgtm

https://codereview.chromium.org/13427020/diff/1/build/android/gyp/finalize_ap...
File build/android/gyp/finalize_apk.py (right):

https://codereview.chromium.org/13427020/diff/1/build/android/gyp/finalize_ap...
build/android/gyp/finalize_apk.py:18: intermediate_path = unsigned_path +
'.copy'
why the extra copy? Can we run it on unsigned path itself?

https://codereview.chromium.org/13427020/diff/1/build/android/gyp/finalize_ap...
build/android/gyp/finalize_apk.py:36: '-f', '4',
Why is it 4? (32 bit?) a clarifying comment will make it easier to understand.

https://codereview.chromium.org/13427020/diff/1/build/android/gyp/finalize_ap...
build/android/gyp/finalize_apk.py:38: final_path,
Do we need a verbosity option here for debugging?

[cjhopman, 2013-04-09 01:58:19]

https://codereview.chromium.org/13427020/diff/1/build/android/gyp/finalize_ap...
File build/android/gyp/finalize_apk.py (right):

https://codereview.chromium.org/13427020/diff/1/build/android/gyp/finalize_ap...
build/android/gyp/finalize_apk.py:18: intermediate_path = unsigned_path +
'.copy'
On 2013/04/08 22:39:04, shashi wrote:
> why the extra copy? Can we run it on unsigned path itself?

unsigned_path is an input to this action and we shouldn't be modifying our
inputs.

we could copy unsigned_path to signed_path and then sign it in-place there, but
that seems wrong in the case where signing fails. i.e. the output(+its
timestamp) will have been updated.

https://codereview.chromium.org/13427020/diff/1/build/android/gyp/finalize_ap...
build/android/gyp/finalize_apk.py:36: '-f', '4',
On 2013/04/08 22:39:04, shashi wrote:
> Why is it 4? (32 bit?) a clarifying comment will make it easier to understand.

Done.

https://codereview.chromium.org/13427020/diff/1/build/android/gyp/finalize_ap...
build/android/gyp/finalize_apk.py:38: final_path,
On 2013/04/08 22:39:04, shashi wrote:
> Do we need a verbosity option here for debugging?

This is already more verbose than in ant (i.e. it's output is printed rather
than suppressed).

[cjhopman, 2013-04-09 01:58:42]

yfriedman@ for OWNERS

[Yaron, 2013-04-09 05:37:16]

lgtm

This has the added benefit of moving the intermediate apks out of the normal
ou/Debug/apks dir, right? Please add to the CL description.

https://codereview.chromium.org/13427020/diff/6001/build/android/ant/apk-pack...
File build/android/ant/apk-package.xml (right):

https://codereview.chromium.org/13427020/diff/6001/build/android/ant/apk-pack...
build/android/ant/apk-package.xml:49: <property name="out.final.file"
location="${FINAL_APK_PATH}" />
Remove

https://codereview.chromium.org/13427020/diff/6001/build/android/ant/apk-pack...
build/android/ant/apk-package.xml:56: <property name="out.unaligned.file"
value="${out.dir}/${APK_NAME}-unaligned.apk" />
Remove.

https://codereview.chromium.org/13427020/diff/6001/build/android/ant/apk-pack...
build/android/ant/apk-package.xml:149: <property name="source.dir"
value="${SOURCE_DIR}" />
Unrelated but these source.dir and source.absolute.dir appear unused. Then you
can also not pass in SOURCE_DIR from gyp, and it's clear that this doesn't do
any compilation

https://codereview.chromium.org/13427020/diff/6001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/13427020/diff/6001/build/java_apk.gypi#newcode53
build/java_apk.gypi:53: 'variables': {
In a follow-up patch, we should split out the expected input variables and the
internal convenience variables. That would hopefully help make it clearer what's
actually expected of a caller.

https://codereview.chromium.org/13427020/diff/6001/build/java_apk.gypi#newcod...
build/java_apk.gypi:566: '-DUNSIGNED_APK_PATH=<(unsigned_apk_path)',
Where is this used? Oh, it looks like out.packaged.file should be updated to
take this in rather than expecting the two values to be in sync.

[cjhopman, 2013-04-09 15:49:46]

The -unsigned and -unaligned have actually been in the intermediate dir for some
time now.

https://codereview.chromium.org/13427020/diff/6001/build/android/ant/apk-pack...
File build/android/ant/apk-package.xml (right):

https://codereview.chromium.org/13427020/diff/6001/build/android/ant/apk-pack...
build/android/ant/apk-package.xml:49: <property name="out.final.file"
location="${FINAL_APK_PATH}" />
On 2013/04/09 05:37:16, Yaron wrote:
> Remove

Done.

https://codereview.chromium.org/13427020/diff/6001/build/android/ant/apk-pack...
build/android/ant/apk-package.xml:56: <property name="out.unaligned.file"
value="${out.dir}/${APK_NAME}-unaligned.apk" />
On 2013/04/09 05:37:16, Yaron wrote:
> Remove.

Done.

https://codereview.chromium.org/13427020/diff/6001/build/android/ant/apk-pack...
build/android/ant/apk-package.xml:149: <property name="source.dir"
value="${SOURCE_DIR}" />
On 2013/04/09 05:37:16, Yaron wrote:
> Unrelated but these source.dir and source.absolute.dir appear unused. Then you
> can also not pass in SOURCE_DIR from gyp, and it's clear that this doesn't do
> any compilation

source.absolute.dir is passed to the apkbuilder task below. I've no idea what it
uses that for (i.e. it shouldn't package anything in there), at some point I
might dig into that task's source to figure out what it's doing.

https://codereview.chromium.org/13427020/diff/6001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/13427020/diff/6001/build/java_apk.gypi#newcode53
build/java_apk.gypi:53: 'variables': {
On 2013/04/09 05:37:16, Yaron wrote:
> In a follow-up patch, we should split out the expected input variables and the
> internal convenience variables. That would hopefully help make it clearer
what's
> actually expected of a caller.

Agreed.

https://codereview.chromium.org/13427020/diff/6001/build/java_apk.gypi#newcod...
build/java_apk.gypi:566: '-DUNSIGNED_APK_PATH=<(unsigned_apk_path)',
On 2013/04/09 05:37:16, Yaron wrote:
> Where is this used? Oh, it looks like out.packaged.file should be updated to
> take this in rather than expecting the two values to be in sync.

Done.

[commit-bot: I haz the power, 2013-04-09 15:50:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/13427020/17001

[commit-bot: I haz the power, 2013-04-09 17:56:29]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-09 17:59:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/13427020/17001

[commit-bot: I haz the power, 2013-04-09 18:39:07]

Change committed as 193160