ABANDONED [android] Create per-package configuration for multidex.

Abandoned: Superseded by https://codereview.chromium.org/2836723002/

[android] Create per-package configuration for multidex.

This will allow separate multidex configurations for instrumentation
test APKs and the corresponding APKs under test.

BUG=712852

[jbudorick, 2017-04-21 01:04:01]

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

[jbudorick, 2017-04-21 01:50:48]

On 2017/04/21 01:04:02, jbudorick wrote:

Actually, hold off on this. I'm working through a few other issues I hit when
working on the follow-up CL.

[jbudorick, 2017-04-21 14:41:03]

ok, ptal

[agrieve, 2017-04-21 15:43:31]

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/PackageBuildConfig.java (right):

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/PackageBuildConfig.java:27:
loader.loadClass(String.format("%s.PackageConfig", packageName));
Would this technique work with using the existing BuildConfig class? Wondering
if the explicit getClassLoader() is enough without needing to also create
classes with different names.

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/PackageBuildConfig.java:31: Log.e(TAG,
"Unable to determine whether multidex is enabled for %s", packageName, e);
Should this be a fatal error (e.g. don't bother catching it, or rethrow as
RuntimeException). Does it ever happen in practice?

https://codereview.chromium.org/2834603006/diff/20001/build/android/main_dex_...
File build/android/main_dex_classes.flags (right):

https://codereview.chromium.org/2834603006/diff/20001/build/android/main_dex_...
build/android/main_dex_classes.flags:14: -keep class **.PackageConfig {
It's always preferable to use @UsedByReflection rather than adding flags. Would
that work in this case?

[jbudorick, 2017-04-21 16:03:57]

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/PackageBuildConfig.java (right):

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/PackageBuildConfig.java:27:
loader.loadClass(String.format("%s.PackageConfig", packageName));
On 2017/04/21 15:43:31, agrieve wrote:
> Would this technique work with using the existing BuildConfig class? Wondering
> if the explicit getClassLoader() is enough without needing to also create
> classes with different names.

It could. My intention was to keep the two separate for conceptual reasons --
BuildConfig handles build-scope constants, while PackageBuildConfig handles
package-scope constants. Perhaps that's unnecessary, though.

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/PackageBuildConfig.java:31: Log.e(TAG,
"Unable to determine whether multidex is enabled for %s", packageName, e);
On 2017/04/21 15:43:31, agrieve wrote:
> Should this be a fatal error (e.g. don't bother catching it, or rethrow as
> RuntimeException). Does it ever happen in practice?

It shouldn't happen except when making changes for multidex, so I hit it a lot
:)

As to whether or not it should be fatal -- given the current implementation of
PackageConfig, probably, though not being able to find PackageConfig is fine for
single-dex APKs. Unsurprisingly, for multidex APKs, hitting this is very bad and
is absolutely fatal slightly later in the startup process.

https://codereview.chromium.org/2834603006/diff/20001/build/android/main_dex_...
File build/android/main_dex_classes.flags (right):

https://codereview.chromium.org/2834603006/diff/20001/build/android/main_dex_...
build/android/main_dex_classes.flags:14: -keep class **.PackageConfig {
On 2017/04/21 15:43:31, agrieve wrote:
> It's always preferable to use @UsedByReflection rather than adding flags.
Would
> that work in this case?

No, the annotations from org.chromium.base.annotations don't work here. I tried
quite a few ways to use @MainDex s.t. PackageConfig was covered by the -keep
above. The issue is in how and under what circumstances we generate these
per-target files via java_cpp_template in android_apk. We assume that any
android_apk target we generate is, at some level, dependent on //base:base_java.
That isn't necessarily the case -- currently, webapk_javatests has no base_java
dependency -- and when it isn't, the java files we have here break.
(webapk_javatests avoids this w/ NativeLibraries.java because it has no native
library.)

Anyhow, I tried all of the following:
 1. using @MainDex without modifying any dependencies. Fails when base_java
isn't one of the indirect dependencies of the android_apk.
 2. using @MainDex w/ base_java added into the deps written to the build_config
s.t. the resulting java_library_impl target is built against it. Fails when
there's no native library and thus no NativeLibraries.
 3. #2 but always generate NativeLibraries. Fails due to collision when building
test APKs.

I would like to figure out a way to use @MainDex here, but I'm beginning to
think that doing so would require reworking how and when we generate those
per-apk java files. That's a bit bigger scope than I want to take on at the
moment.

[agrieve, 2017-04-21 16:48:42]

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/PackageBuildConfig.java (right):

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/PackageBuildConfig.java:27:
loader.loadClass(String.format("%s.PackageConfig", packageName));
On 2017/04/21 16:03:57, jbudorick wrote:
> On 2017/04/21 15:43:31, agrieve wrote:
> > Would this technique work with using the existing BuildConfig class?
Wondering
> > if the explicit getClassLoader() is enough without needing to also create
> > classes with different names.
> 
> It could. My intention was to keep the two separate for conceptual reasons --
> BuildConfig handles build-scope constants, while PackageBuildConfig handles
> package-scope constants. Perhaps that's unnecessary, though.

I think I'd prefer having just one BuildConfig then. It's already per-package
since it has COMPRESSED_LOCALES as well.

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/PackageBuildConfig.java:31: Log.e(TAG,
"Unable to determine whether multidex is enabled for %s", packageName, e);
On 2017/04/21 16:03:57, jbudorick wrote:
> On 2017/04/21 15:43:31, agrieve wrote:
> > Should this be a fatal error (e.g. don't bother catching it, or rethrow as
> > RuntimeException). Does it ever happen in practice?
> 
> It shouldn't happen except when making changes for multidex, so I hit it a lot
> :)
> 
> As to whether or not it should be fatal -- given the current implementation of
> PackageConfig, probably, though not being able to find PackageConfig is fine
for
> single-dex APKs. Unsurprisingly, for multidex APKs, hitting this is very bad
and
> is absolutely fatal slightly later in the startup process.

Given your explanation, maybe better to make it a Log.d() so that it's stripped
in release binaries?

https://codereview.chromium.org/2834603006/diff/20001/build/android/main_dex_...
File build/android/main_dex_classes.flags (right):

https://codereview.chromium.org/2834603006/diff/20001/build/android/main_dex_...
build/android/main_dex_classes.flags:14: -keep class **.PackageConfig {
On 2017/04/21 16:03:57, jbudorick wrote:
> On 2017/04/21 15:43:31, agrieve wrote:
> > It's always preferable to use @UsedByReflection rather than adding flags.
> Would
> > that work in this case?
> 
> No, the annotations from org.chromium.base.annotations don't work here. I
tried
> quite a few ways to use @MainDex s.t. PackageConfig was covered by the -keep
> above. The issue is in how and under what circumstances we generate these
> per-target files via java_cpp_template in android_apk. We assume that any
> android_apk target we generate is, at some level, dependent on
//base:base_java.
> That isn't necessarily the case -- currently, webapk_javatests has no
base_java
> dependency -- and when it isn't, the java files we have here break.
> (webapk_javatests avoids this w/ NativeLibraries.java because it has no native
> library.)
> 
> Anyhow, I tried all of the following:
>  1. using @MainDex without modifying any dependencies. Fails when base_java
> isn't one of the indirect dependencies of the android_apk.
>  2. using @MainDex w/ base_java added into the deps written to the
build_config
> s.t. the resulting java_library_impl target is built against it. Fails when
> there's no native library and thus no NativeLibraries.
>  3. #2 but always generate NativeLibraries. Fails due to collision when
building
> test APKs.
> 
> I would like to figure out a way to use @MainDex here, but I'm beginning to
> think that doing so would require reworking how and when we generate those
> per-apk java files. That's a bit bigger scope than I want to take on at the
> moment.

Sounds good. Thanks for the details :)

[jbudorick, 2017-04-21 19:03:04]

not yet updated

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/PackageBuildConfig.java (right):

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/PackageBuildConfig.java:27:
loader.loadClass(String.format("%s.PackageConfig", packageName));
On 2017/04/21 16:48:42, agrieve wrote:
> On 2017/04/21 16:03:57, jbudorick wrote:
> > On 2017/04/21 15:43:31, agrieve wrote:
> > > Would this technique work with using the existing BuildConfig class?
> Wondering
> > > if the explicit getClassLoader() is enough without needing to also create
> > > classes with different names.
> > 
> > It could. My intention was to keep the two separate for conceptual reasons
--
> > BuildConfig handles build-scope constants, while PackageBuildConfig handles
> > package-scope constants. Perhaps that's unnecessary, though.
> 
> I think I'd prefer having just one BuildConfig then. It's already per-package
> since it has COMPRESSED_LOCALES as well.

I was wrong -- if we want to keep the Log message at all, this needs to stay out
of template form. Thoughts?

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/PackageBuildConfig.java:31: Log.e(TAG,
"Unable to determine whether multidex is enabled for %s", packageName, e);
On 2017/04/21 16:48:42, agrieve wrote:
> On 2017/04/21 16:03:57, jbudorick wrote:
> > On 2017/04/21 15:43:31, agrieve wrote:
> > > Should this be a fatal error (e.g. don't bother catching it, or rethrow as
> > > RuntimeException). Does it ever happen in practice?
> > 
> > It shouldn't happen except when making changes for multidex, so I hit it a
lot
> > :)
> > 
> > As to whether or not it should be fatal -- given the current implementation
of
> > PackageConfig, probably, though not being able to find PackageConfig is fine
> for
> > single-dex APKs. Unsurprisingly, for multidex APKs, hitting this is very bad
> and
> > is absolutely fatal slightly later in the startup process.
> 
> Given your explanation, maybe better to make it a Log.d() so that it's
stripped
> in release binaries?

Done.

[jbudorick, 2017-04-22 02:51:09]

Description was changed from

==========
[android] Create per-package configuration for multidex.

This will allow separate multidex configurations for instrumentation
test APKs and the corresponding APKs under test.

BUG=712852
==========

to

==========
Abandoned: Superseded by https://codereview.chromium.org/2836723002/

[android] Create per-package configuration for multidex.

This will allow separate multidex configurations for instrumentation
test APKs and the corresponding APKs under test.

BUG=712852
==========