[Android] Add gn support for multidex.

[Android] Add gn support for multidex.

BUG=272790
Committed: https://crrev.com/e6361975d7f7f4c434dc3380a3824b9d5e323d20
Cr-Commit-Position: refs/heads/master@{#360943}

[jbudorick, 2015-11-15 00:27:09]

jbudorick@chromium.org changed reviewers:
+ agrieve@chromium.org

[jbudorick, 2015-11-15 00:27:09]

✈

[agrieve, 2015-11-16 19:02:34]

Nothing but nits! lgtm :)

https://codereview.chromium.org/1451483002/diff/20001/build/android/gyp/apkbu...
File build/android/gyp/apkbuilder.py (right):

https://codereview.chromium.org/1451483002/diff/20001/build/android/gyp/apkbu...
build/android/gyp/apkbuilder.py:150: and zipfile.is_zipfile(options.dex_file)):
nit: stands out a bit to me. Probably checking the extension is enough.

https://codereview.chromium.org/1451483002/diff/20001/build/android/gyp/dex.py
File build/android/gyp/dex.py (right):

https://codereview.chromium.org/1451483002/diff/20001/build/android/gyp/dex.p...
build/android/gyp/dex.py:63: parser.add_option('--multi-dex', default=False,
action='store_true')
nit: Add help text?

https://codereview.chromium.org/1451483002/diff/20001/build/android/gyp/main_...
File build/android/gyp/main_dex_list.py (right):

https://codereview.chromium.org/1451483002/diff/20001/build/android/gyp/main_...
build/android/gyp/main_dex_list.py:59: shrinked_android_jar = os.path.abspath(
nit: I know this was already here, but "shrinked" isn't a word. Can you change
to shrank or shrunk?

https://codereview.chromium.org/1451483002/diff/20001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1451483002/diff/20001/build/config/android/in...
build/config/android/internal_rules.gni:226: _multidex =
defined(invoker.multidex) && invoker.multidex
nit: It would be more GN'y to call this "enable_multidex" or "use_multidex"

https://codereview.chromium.org/1451483002/diff/20001/build/config/android/in...
build/config/android/internal_rules.gni:230: template("main_dex_list_helper") {
doesn't look like this is used more than once, so no need to use a template...
Unless you're planning on re-using it, in which case it's fine.

https://codereview.chromium.org/1451483002/diff/20001/chrome/android/chrome_p...
File chrome/android/chrome_public_apk_tmpl.gni (right):

https://codereview.chromium.org/1451483002/diff/20001/chrome/android/chrome_p...
chrome/android/chrome_public_apk_tmpl.gni:36: multidex = true
super nit: can you flip the clauses so it reads "if (is_debug) {..."

[jbudorick, 2015-11-16 20:45:56]

https://codereview.chromium.org/1451483002/diff/20001/build/android/gyp/apkbu...
File build/android/gyp/apkbuilder.py (right):

https://codereview.chromium.org/1451483002/diff/20001/build/android/gyp/apkbu...
build/android/gyp/apkbuilder.py:150: and zipfile.is_zipfile(options.dex_file)):
On 2015/11/16 19:02:33, agrieve wrote:
> nit: stands out a bit to me. Probably checking the extension is enough.

Done.

https://codereview.chromium.org/1451483002/diff/20001/build/android/gyp/dex.py
File build/android/gyp/dex.py (right):

https://codereview.chromium.org/1451483002/diff/20001/build/android/gyp/dex.p...
build/android/gyp/dex.py:63: parser.add_option('--multi-dex', default=False,
action='store_true')
On 2015/11/16 19:02:33, agrieve wrote:
> nit: Add help text?

Done.

https://codereview.chromium.org/1451483002/diff/20001/build/android/gyp/main_...
File build/android/gyp/main_dex_list.py (right):

https://codereview.chromium.org/1451483002/diff/20001/build/android/gyp/main_...
build/android/gyp/main_dex_list.py:59: shrinked_android_jar = os.path.abspath(
On 2015/11/16 19:02:33, agrieve wrote:
> nit: I know this was already here, but "shrinked" isn't a word. Can you change
> to shrank or shrunk?

hah, it was already here because I wrote it this way in a previous CL. 

I used shrinked_android_jar because it's the path to shrinkedAndroid.jar. I'd
rather have the variable match the jar than conjugate the verb correctly :)

https://codereview.chromium.org/1451483002/diff/20001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1451483002/diff/20001/build/config/android/in...
build/config/android/internal_rules.gni:226: _multidex =
defined(invoker.multidex) && invoker.multidex
On 2015/11/16 19:02:33, agrieve wrote:
> nit: It would be more GN'y to call this "enable_multidex" or "use_multidex"

I'm still getting a handle on what's GN'y. switched to enable_multidex

https://codereview.chromium.org/1451483002/diff/20001/build/config/android/in...
build/config/android/internal_rules.gni:230: template("main_dex_list_helper") {
On 2015/11/16 19:02:33, agrieve wrote:
> doesn't look like this is used more than once, so no need to use a template...
> Unless you're planning on re-using it, in which case it's fine.

I'm not. I think I patterned this off of something else, but I forget what.
Removed the template.

https://codereview.chromium.org/1451483002/diff/20001/chrome/android/chrome_p...
File chrome/android/chrome_public_apk_tmpl.gni (right):

https://codereview.chromium.org/1451483002/diff/20001/chrome/android/chrome_p...
chrome/android/chrome_public_apk_tmpl.gni:36: multidex = true
On 2015/11/16 19:02:33, agrieve wrote:
> super nit: can you flip the clauses so it reads "if (is_debug) {..."

Done.

[jbudorick, 2015-11-16 20:47:46]

jbudorick@chromium.org changed reviewers:
+ thakis@chromium.org, yfriedman@chromium.org

[jbudorick, 2015-11-16 20:47:47]

+thakis for base/BUILD.gn OWNERS
+yfriedman for chrome/android/chrome_public_apk_tmpl.gni OWNERS

[Yaron, 2015-11-16 21:01:17]

lgtm

[Nico, 2015-11-16 21:46:54]

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn#newcode1736
base/BUILD.gn:1736: }
why isn't this needed for gyp?

[jbudorick, 2015-11-16 21:49:18]

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn#newcode1736
base/BUILD.gn:1736: }
On 2015/11/16 21:46:54, Nico wrote:
> why isn't this needed for gyp?

it is, though it's handled a little differently there because we can't switch on
build type in gyp:

https://codereview.chromium.org/1408163009/diff/110001/base/base.gyp line 1473

[Nico, 2015-11-16 21:57:55]

Sorry, I'm still confused:

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn#newcode1736
base/BUILD.gn:1736: }
On 2015/11/16 21:49:18, jbudorick wrote:
> On 2015/11/16 21:46:54, Nico wrote:
> > why isn't this needed for gyp?
> 
> it is, though it's handled a little differently there because we can't switch
on
> build type in gyp:
> 
> https://codereview.chromium.org/1408163009/diff/110001/base/base.gyp line 1473

Ah, ok. (You can switch on build type in gyp if you want as far as I know, e.g.:
https://code.google.com/p/chromium/codesearch#chromium/src/breakpad/breakpad....)

Why is this needed only for base? (I tried to find ChromiumMultiDex.template,
but cs.chromium.org doesn't seem to know about it. There's a
ChromiumMultiDex.java but it doesn't use anything that looks like
CONFIGURATION_NAME_). This path points to
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...
which doesn't contain the .template file – maybe you forgot to git add it at
some point in the past?

[jbudorick, 2015-11-16 22:00:22]

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn#newcode1736
base/BUILD.gn:1736: }
On 2015/11/16 21:57:55, Nico wrote:
> On 2015/11/16 21:49:18, jbudorick wrote:
> > On 2015/11/16 21:46:54, Nico wrote:
> > > why isn't this needed for gyp?
> > 
> > it is, though it's handled a little differently there because we can't
switch
> on
> > build type in gyp:
> > 
> > https://codereview.chromium.org/1408163009/diff/110001/base/base.gyp line
1473
> 
> Ah, ok. (You can switch on build type in gyp if you want as far as I know,
e.g.:
>
https://code.google.com/p/chromium/codesearch#chromium/src/breakpad/breakpad....)
> 
> Why is this needed only for base? (I tried to find ChromiumMultiDex.template,
> but http://cs.chromium.org doesn't seem to know about it. There's a
> ChromiumMultiDex.java but it doesn't use anything that looks like
> CONFIGURATION_NAME_). This path points to
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...
> which doesn't contain the .template file – maybe you forgot to git add it
at
> some point in the past?

Maybe code search had a bad weekend? It's in there:
https://codereview.chromium.org/1408163009/diff/110001/base/android/java/temp...

[Nico, 2015-11-16 22:04:51]

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn#newcode1736
base/BUILD.gn:1736: }
On 2015/11/16 22:00:21, jbudorick wrote:
> On 2015/11/16 21:57:55, Nico wrote:
> > On 2015/11/16 21:49:18, jbudorick wrote:
> > > On 2015/11/16 21:46:54, Nico wrote:
> > > > why isn't this needed for gyp?
> > > 
> > > it is, though it's handled a little differently there because we can't
> switch
> > on
> > > build type in gyp:
> > > 
> > > https://codereview.chromium.org/1408163009/diff/110001/base/base.gyp line
> 1473
> > 
> > Ah, ok. (You can switch on build type in gyp if you want as far as I know,
> e.g.:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/breakpad/breakpad....)
> > 
> > Why is this needed only for base? (I tried to find
ChromiumMultiDex.template,
> > but http://cs.chromium.org doesn't seem to know about it. There's a
> > ChromiumMultiDex.java but it doesn't use anything that looks like
> > CONFIGURATION_NAME_). This path points to
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...
> > which doesn't contain the .template file – maybe you forgot to git add
it
> at
> > some point in the past?
> 
> Maybe code search had a bad weekend? It's in there:
>
https://codereview.chromium.org/1408163009/diff/110001/base/android/java/temp...

Oh, it's even in this CL, cool, thanks.

…why does the behavior of install() change with build type? Why does
processIsIsolated() only exist in debug builds?

Problems I have with this:

1. CONFIGURATION_NAME_Debug looks like an overly broad name. People might
copy-paste that into their .templates, yet this is only defined correctly for
the base target.

2. Having big behavior differences like this between Release and Debug is
probably not a good idea. For example, the trybots don't run tests in debug but
in release with dchecks enabled – so this should probably be keyed off the "are
dchecks on?" define instead if it needs to be conditional.

[jbudorick, 2015-11-16 22:19:33]

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn#newcode1736
base/BUILD.gn:1736: }
On 2015/11/16 22:04:51, Nico wrote:
> On 2015/11/16 22:00:21, jbudorick wrote:
> > On 2015/11/16 21:57:55, Nico wrote:
> > > On 2015/11/16 21:49:18, jbudorick wrote:
> > > > On 2015/11/16 21:46:54, Nico wrote:
> > > > > why isn't this needed for gyp?
> > > > 
> > > > it is, though it's handled a little differently there because we can't
> > switch
> > > on
> > > > build type in gyp:
> > > > 
> > > > https://codereview.chromium.org/1408163009/diff/110001/base/base.gyp
line
> > 1473
> > > 
> > > Ah, ok. (You can switch on build type in gyp if you want as far as I know,
> > e.g.:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/breakpad/breakpad....)
> > > 
> > > Why is this needed only for base? (I tried to find
> ChromiumMultiDex.template,
> > > but http://cs.chromium.org doesn't seem to know about it. There's a
> > > ChromiumMultiDex.java but it doesn't use anything that looks like
> > > CONFIGURATION_NAME_). This path points to
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...
> > > which doesn't contain the .template file – maybe you forgot to git
add
> it
> > at
> > > some point in the past?
> > 
> > Maybe code search had a bad weekend? It's in there:
> >
>
https://codereview.chromium.org/1408163009/diff/110001/base/android/java/temp...
> 
> Oh, it's even in this CL, cool, thanks.

er... it was in the last one.

> 
> …why does the behavior of install() change with build type? Why does

because we don't currently want to do multidex installation at all in release
builds. Even with a single dex, it caused a pretty big start-up regression:
https://code.google.com/p/chromium/issues/detail?id=525695

It's only necessary in debug builds because we run proguard in release builds,
which puts us comfortably below the dex limit.

> processIsIsolated() only exist in debug builds?
> 
> Problems I have with this:
> 
> 1. CONFIGURATION_NAME_Debug looks like an overly broad name. People might
> copy-paste that into their .templates, yet this is only defined correctly for
> the base target.
> 
> 2. Having big behavior differences like this between Release and Debug is
> probably not a good idea. For example, the trybots don't run tests in debug
but
> in release with dchecks enabled – so this should probably be keyed off
the "are
> dchecks on?" define instead if it needs to be conditional.

[jbudorick, 2015-11-17 16:27:06]

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn#newcode1736
base/BUILD.gn:1736: }
On 2015/11/16 22:00:21, jbudorick wrote:
> On 2015/11/16 21:57:55, Nico wrote:
> > On 2015/11/16 21:49:18, jbudorick wrote:
> > > On 2015/11/16 21:46:54, Nico wrote:
> > > > why isn't this needed for gyp?
> > > 
> > > it is, though it's handled a little differently there because we can't
> switch
> > on
> > > build type in gyp:
> > > 
> > > https://codereview.chromium.org/1408163009/diff/110001/base/base.gyp line
> 1473
> > 
> > Ah, ok. (You can switch on build type in gyp if you want as far as I know,
> e.g.:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/breakpad/breakpad....)
> > 
> > Why is this needed only for base? (I tried to find
ChromiumMultiDex.template,
> > but http://cs.chromium.org doesn't seem to know about it. There's a
> > ChromiumMultiDex.java but it doesn't use anything that looks like
> > CONFIGURATION_NAME_). This path points to
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...
> > which doesn't contain the .template file – maybe you forgot to git add
it
> at
> > some point in the past?
> 
> Maybe code search had a bad weekend? It's in there:
>
https://codereview.chromium.org/1408163009/diff/110001/base/android/java/temp...

(code search did have a bad weekend:
https://build.chromium.org/p/chromium.infra.cron/builders/Chromium%20Linux%20...)

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn#newcode1736
base/BUILD.gn:1736: }
On 2015/11/16 21:57:55, Nico wrote:
> On 2015/11/16 21:49:18, jbudorick wrote:
> > On 2015/11/16 21:46:54, Nico wrote:
> > > why isn't this needed for gyp?
> > 
> > it is, though it's handled a little differently there because we can't
switch
> on
> > build type in gyp:
> > 
> > https://codereview.chromium.org/1408163009/diff/110001/base/base.gyp line
1473
> 
> Ah, ok. (You can switch on build type in gyp if you want as far as I know,
e.g.:
>
https://code.google.com/p/chromium/codesearch#chromium/src/breakpad/breakpad....)

also, I tried these when I was writing the gyp patch but couldn't get it to work
for targets that aren't doing c++ compilation (like this one).

> 
> Why is this needed only for base? (I tried to find ChromiumMultiDex.template,
> but http://cs.chromium.org doesn't seem to know about it. There's a
> ChromiumMultiDex.java but it doesn't use anything that looks like
> CONFIGURATION_NAME_). This path points to
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...
> which doesn't contain the .template file – maybe you forgot to git add it
at
> some point in the past?

https://codereview.chromium.org/1451483002/diff/40001/base/BUILD.gn#newcode1736
base/BUILD.gn:1736: }
On 2015/11/16 22:04:51, Nico wrote:
> On 2015/11/16 22:00:21, jbudorick wrote:
> > On 2015/11/16 21:57:55, Nico wrote:
> > > On 2015/11/16 21:49:18, jbudorick wrote:
> > > > On 2015/11/16 21:46:54, Nico wrote:
> > > > > why isn't this needed for gyp?
> > > > 
> > > > it is, though it's handled a little differently there because we can't
> > switch
> > > on
> > > > build type in gyp:
> > > > 
> > > > https://codereview.chromium.org/1408163009/diff/110001/base/base.gyp
line
> > 1473
> > > 
> > > Ah, ok. (You can switch on build type in gyp if you want as far as I know,
> > e.g.:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/breakpad/breakpad....)
> > > 
> > > Why is this needed only for base? (I tried to find
> ChromiumMultiDex.template,
> > > but http://cs.chromium.org doesn't seem to know about it. There's a
> > > ChromiumMultiDex.java but it doesn't use anything that looks like
> > > CONFIGURATION_NAME_). This path points to
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...
> > > which doesn't contain the .template file – maybe you forgot to git
add
> it
> > at
> > > some point in the past?
> > 
> > Maybe code search had a bad weekend? It's in there:
> >
>
https://codereview.chromium.org/1408163009/diff/110001/base/android/java/temp...
> 
> Oh, it's even in this CL, cool, thanks.
> 
> …why does the behavior of install() change with build type? Why does
> processIsIsolated() only exist in debug builds?
> 
> Problems I have with this:
> 
> 1. CONFIGURATION_NAME_Debug looks like an overly broad name. People might
> copy-paste that into their .templates, yet this is only defined correctly for
> the base target.

made this define clearly multidex-specific.

> 
> 2. Having big behavior differences like this between Release and Debug is
> probably not a good idea. For example, the trybots don't run tests in debug
but
> in release with dchecks enabled – so this should probably be keyed off
the "are
> dchecks on?" define instead if it needs to be conditional.

[Nico, 2015-11-20 18:51:11]

lgtm

Maybe add some of what you said above as a comment in the .template

[jbudorick, 2015-11-20 21:30:01]

On 2015/11/20 18:51:11, Nico wrote:
> lgtm
> 
> Maybe add some of what you said above as a comment in the .template

Done.

[jbudorick, 2015-11-20 21:30:41]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-11-20 21:30:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, yfriedman@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1451483002/#ps80001
(title: "+comment in ChromiumMultiDex.template")

[commit-bot: I haz the power, 2015-11-20 21:33:46]

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

[commit-bot: I haz the power, 2015-11-20 22:19:24]

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

[commit-bot: I haz the power, 2015-11-20 22:19:25]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jbudorick, 2015-11-20 22:20:42]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2015-11-20 22:21:05]

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

[commit-bot: I haz the power, 2015-11-20 23:19:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-20 23:20:41]

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