1291180 - Add Android-Job library in the build.

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[k.krish]

To have a better scheduling solution, We need to integrate the library https://github.com/evernote/android-job in Firefox for android build scripts.

Comment 1

[k.krish]

Attachment: Bug 1291180 - Add Android-Job library in the build

Review commit: https://reviewboard.mozilla.org/r/68648/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68648/

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

https://reviewboard.mozilla.org/r/68648/#review65908

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/68648/#review65910

This just adds the code to gradle based builds. It will not picked up by builds using the 'mach' build tool (build servers, full builds, ..).

You'll have to add the code to the thirdparty folder:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/thirdparty

And then reference it from moz.build:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build#841

Comment 4

[k.krish]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68648/diff/1-2/

Comment 5

[k.krish]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68648/diff/2-3/

Comment 6

[k.krish]

https://reviewboard.mozilla.org/r/68648/#review65910

I have added/changed some files. Now the code for both mach builds and gradle builds are there

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

https://reviewboard.mozilla.org/r/68648/#review67140

::: mobile/android/app/build.gradle:33
(Diff revision 3)
>          targetCompatibility JavaVersion.VERSION_1_7
>      }
> - 
> +
>      dexOptions {
>          javaMaxHeapSize "2g"
> +        jumboMode true

Why is this needed? Do we exceed the method limit after adding this library?

::: mobile/android/base/moz.build:922
(Diff revision 3)
> +    CONFIG['ANDROID_PLAY_SERVICES_GCM_AAR_LIB'],
> +    CONFIG['ANDROID_PLAY_SERVICES_BASEMENT_AAR_LIB'],
> +    CONFIG['ANDROID_PLAY_SERVICES_BASE_AAR_LIB'],

So far those libraries are only added if MOZ_ANDROID_GCM is set. Can you instead of listing them here, remove the dependency on MOZ_ANDROID_GCM?

::: mobile/android/thirdparty/build.gradle:42
(Diff revision 3)
>      }
>  }
>  
>  dependencies {
>      compile "com.android.support:support-v4:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
> +    provided "com.google.android.gms:play-services-gcm:${mozconfig.substs.ANDROID_GOOGLE_PLAY_SERVICES_VERSION}"

Are you sure this is needed?

::: mobile/android/thirdparty/net/vrallev/android/cat/Cat.java:7
(Diff revision 3)
> +
> +/**
> + * @author rwondratschek
> + */
> +@SuppressWarnings("UnusedDeclaration")
> +public final class Cat {

Is this the log dependency from the job class? I wonder why there are so many "unused" warnings suppressed?

Comment 8

[k.krish]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68648/diff/3-4/

Comment 9

[k.krish]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

https://reviewboard.mozilla.org/r/68648/#review67140

> Why is this needed? Do we exceed the method limit after adding this library?

Yes. When I added the library and ran ./gradlew clean app:assembleLocalOldDebug. I got DexException. We are exceeding the limit.

> So far those libraries are only added if MOZ_ANDROID_GCM is set. Can you instead of listing them here, remove the dependency on MOZ_ANDROID_GCM?

I have added the check. if CONFIG['MOZ_ANDROID_GCM']: The scheduler library in the third-party directory needs those libraries as the scheduler needs GCMNetworkManager to schedule jobs

> Are you sure this is needed?

Yes. It is needed :) The scheduler depends on the GCM Library. Without this the Gradle Build (AndroidStudio) will fail.

> Is this the log dependency from the job class? I wonder why there are so many "unused" warnings suppressed?

Yeah this is the log dependency I was telling about. May be we can strip all the log dependancy from the Scheduler Library. In that way we can minimize the number of files added in the third-party directory.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

https://reviewboard.mozilla.org/r/68648/#review68276

::: mobile/android/app/build.gradle:33
(Diff revision 4)
>          targetCompatibility JavaVersion.VERSION_1_7
>      }
> - 
> +
>      dexOptions {
>          javaMaxHeapSize "2g"
> +        jumboMode true

But what happens with mach based builds (which does not read build.gradle)?

We did enable multidex for some gradle based builds (no release versions). Can you explore the differences and whether multidex helps here too?

::: mobile/android/base/moz.build:924
(Diff revision 4)
> +if CONFIG['MOZ_ANDROID_GCM']:
> +    gtjar.extra_jars += [
> +        CONFIG['ANDROID_PLAY_SERVICES_BASE_AAR_LIB'],
> +        CONFIG['ANDROID_PLAY_SERVICES_BASEMENT_AAR_LIB'],
> +        CONFIG['ANDROID_PLAY_SERVICES_GCM_AAR_LIB'],
> +    ]
> +

What I meant was: Now with the library added those play services libraries need to be added to all builds (no matter whether gcm is enabled or not). Therefore we need to remove the MOZ_ANDROID_GCM check when adding the libraries here and in other places in this file.

::: mobile/android/thirdparty/build.gradle:42
(Diff revision 4)
>      }
>  }
>  
>  dependencies {
>      compile "com.android.support:support-v4:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
> +    provided "com.google.android.gms:play-services-gcm:${mozconfig.substs.ANDROID_GOOGLE_PLAY_SERVICES_VERSION}"

This should be 'compile' instead of 'provided', I guess. With 'provided' the code will be compiled against the library but it won't be packed. Is this intended here?

Comment 11

[k.krish]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68648/diff/4-5/

Comment 12

[k.krish]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

https://reviewboard.mozilla.org/r/68648/#review68276

> But what happens with mach based builds (which does not read build.gradle)?
> 
> We did enable multidex for some gradle based builds (no release versions). Can you explore the differences and whether multidex helps here too?

Mach based builds passes without this exception. MultiDex will not help here as this exception is related to the number of strings that can be referenced in a DEX.

> What I meant was: Now with the library added those play services libraries need to be added to all builds (no matter whether gcm is enabled or not). Therefore we need to remove the MOZ_ANDROID_GCM check when adding the libraries here and in other places in this file.

Removed all the MOZ_ANDROID_GCM Check in the moz.build

> This should be 'compile' instead of 'provided', I guess. With 'provided' the code will be compiled against the library but it won't be packed. Is this intended here?

Yes. I changed 'provided' to 'compile' and also explicitly mentioned the transitive dependancy. In mach based build we are adding the GMS-GCM AAR, GMS-BASE AAR and GMS-BASEMENT AAR The similar way of adding the dependancy to Gradle based build is to add GCM, BASE, BASEMENT without any transitive dependancy.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

https://reviewboard.mozilla.org/r/68648/#review70606

::: mobile/android/base/moz.build:713
(Diff revision 5)
> -if CONFIG['MOZ_ANDROID_GCM']:
> +
> -    gbjar.sources += ['java/org/mozilla/gecko/' + x for x in [
> +gbjar.sources += ['java/org/mozilla/gecko/' + x for x in [
> -        'gcm/GcmInstanceIDListenerService.java',
> +    'gcm/GcmInstanceIDListenerService.java',
> -        'gcm/GcmMessageListenerService.java',
> +    'gcm/GcmMessageListenerService.java',
> -        'gcm/GcmTokenClient.java',
> +    'gcm/GcmTokenClient.java',
> -        'push/Fetched.java',
> +    'push/Fetched.java',
> -        'push/PushClient.java',
> +    'push/PushClient.java',
> -        'push/PushManager.java',
> +    'push/PushManager.java',
> -        'push/PushRegistration.java',
> +    'push/PushRegistration.java',
> -        'push/PushService.java',
> +    'push/PushService.java',
> -        'push/PushState.java',
> +    'push/PushState.java',
> -        'push/PushSubscription.java',
> +    'push/PushSubscription.java',
> -    ]]
> +]]

We should still keep the flag for our gcm code. Just GPS dependencies are not depending on MOZ_ANDROID_GCM (only) anymore.

::: mobile/android/thirdparty/build.gradle:43
(Diff revision 5)
>  }
>  
>  dependencies {
>      compile "com.android.support:support-v4:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
> +    compile ("com.google.android.gms:play-services-gcm:${mozconfig.substs.ANDROID_GOOGLE_PLAY_SERVICES_VERSION}") {
> +        transitive = false

You can add them without "transitive=false". Gradle does resolve the transitive dependencies for you, so normally base/basement wouldn't be needed here. However we mirror them in our build.gradle files to just reflect what we listed in the mach based build too (where we have to resolve the dependencies manually).

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

https://reviewboard.mozilla.org/r/68648/#review70610

::: mobile/android/thirdparty/net/vrallev/android/cat/CatGlobal.java:19
(Diff revision 5)
> +
> +/**
> + * @author rwondratschek
> + */
> +@SuppressWarnings("unused")
> +public final class CatGlobal {

Can you file a follow-up bug to investigate if we can remove/replace this logging library?

Comment 15

[k.krish]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68648/diff/5-6/

Comment 16

[k.krish]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

https://reviewboard.mozilla.org/r/68648/#review70610

> Can you file a follow-up bug to investigate if we can remove/replace this logging library?

I have made necessary changes to the library and replaced the logging library with the default android logging. As we do not require the logging library anymore I have removed it from the build. Removing it has not made "DexIndexOverflowException" to go away. So we need to have - "jumboMode true" in Gradle. Now the scheduler is lighter the previous commit :)

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

nalexander: Can you give feedback about the MOZ_ANDROID_GCM bits? If we want to (optionally) use GcmNetworkManager for scheduling (1248901) then we will always need to build against the GCM library and therefore remove some of the MOZ_ANDROID_GCM checks.

Comment 18

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

https://reviewboard.mozilla.org/r/68648/#review75184

Technically, this looks fine.  I have marked r- to ensure I check-back on this ticket to see your answer re `jumboMode true`.

At a high level, Mozilla wants to be able to ship a version of Firefox for Android that does not require (potentially) non-free code.  (I have CCed sebastian onto a MoCo-confidential ticket to talk about this.)  If we consider Google Play Services non-free, this patch will make that impossible.  I would like to understand further what we need GPS/GCM for in this situation before blessing this approach -- perhaps we can keep the noble goal of shipping a fully-free version of Firefox for Android?

That said, technically this looks fine, and I am willing to be convinced that we should just require GPS/GCM at this point.

::: mobile/android/app/build.gradle:33
(Diff revision 6)
>          targetCompatibility JavaVersion.VERSION_1_7
>      }
>  
>      dexOptions {
>          javaMaxHeapSize "2g"
> +        jumboMode true

Is this actually necessary?  You're not adding any strings, and we already successfully build with the additional Google Play libraries...

::: mobile/android/thirdparty/build.gradle:42
(Diff revision 6)
>      }
>  }
>  
>  dependencies {
>      compile "com.android.support:support-v4:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
> +    compile "com.google.android.gms:play-services-gcm:${mozconfig.substs.ANDROID_GOOGLE_PLAY_SERVICES_VERSION}"

Presumably these should be *removed* from a `MOZ_NATIVE_DEVICES`-guarded block as well.  This is not essential, but will help message your intent.

Comment 19

[Gervase Markham [:gerv]]

I'm also interested in understanding why a job scheduler requires Google Play Services :-)

Gerv

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Gervase Markham [:gerv] from comment #19)
> I'm also interested in understanding why a job scheduler requires Google
> Play Services :-)

See bug 1248901 for background. We have different scheduling APIs available on different API levels. We would like to use JobScheduler, but it is only available on Android 5+. AlarmManager is available on all API levels but is pretty bad. On devices with GPS we can use GcmNetworkManager which is as good as JobScheduler but yeah, only available with GPS. So we would like to have a fallback mechanism picking the best available scheduler: AlarmManager < GcmNetworkManager < JobScheduler. This library does exactly this for us.

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8777041 [details]
Bug 1291180 - Add Android-Job library in the build

@Krish: Reading up on the other bugs and discussions I think we should at least make sure that a consumer can build the app without requiring the proprietary bits from Google (There are builds on f-droid without it and the TOR browser based on Firefox probably does not want those libraries included too).

Can you look if it is easy to exclude the files using GCM from the build if MOZ_ANDROID_GCM is not enabled? Basically android-job should only use AlarmManager and JobScheduler if the GCM flag is disabled. If this is not easy because the code is too mingled then stop and we look for a different solution.

Comment 22

[Gervase Markham [:gerv]]

(In reply to Sebastian Kaspari (:sebastian) [PTO 9/12 - 9/21] from comment #21) 
> Can you look if it is easy to exclude the files using GCM from the build if
> MOZ_ANDROID_GCM is not enabled? Basically android-job should only use
> AlarmManager and JobScheduler if the GCM flag is disabled. If this is not
> easy because the code is too mingled then stop and we look for a different
> solution.

That seems like the right answer...

Gerv

Comment 23

[k.krish]

(In reply to Sebastian Kaspari (:sebastian) [PTO 9/12 - 9/21] from comment #21)
> Comment on attachment 8777041 [details]
> Bug 1291180 - Add Android-Job library in the build
> 
> @Krish: Reading up on the other bugs and discussions I think we should at
> least make sure that a consumer can build the app without requiring the
> proprietary bits from Google (There are builds on f-droid without it and the
> TOR browser based on Firefox probably does not want those libraries included
> too).
> 
> Can you look if it is easy to exclude the files using GCM from the build if
> MOZ_ANDROID_GCM is not enabled? Basically android-job should only use
> AlarmManager and JobScheduler if the GCM flag is disabled. If this is not
> easy because the code is too mingled then stop and we look for a different
> solution.

@Sebatian: The library is designed in such a way that if the GCMNetworkManager is not available then automatically it will switch to the other two native Apis. So technically we can remove the GCM bits from the library. I have already started exploring the internals of the library and removed the GCM parts. So when we remove the GCMNetworkManager then the library will choose between AlarmManager and JobScheduler. I will experiment with this approach and make sure that the application doesn't crash. In such case we will not be needing the GPS in third-party libraries in the build.

Comment 24

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.