Closed Bug 1463376 Opened 2 years ago Closed 2 years ago

Update to latest Google Play Services version

Categories

(Firefox for Android :: General, enhancement, P1)

All
Android
enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: petru, Assigned: andrei.a.lazar)

References

(Blocks 3 open bugs)

Details

(Whiteboard: --do_not_change--[priority:high])

Attachments

(2 files)

In the process of testing the update to support library 26 (bug 1385464) I found that the sign-in process crashes the app, probably because of incompatibilities of the currently play-services version used(8.4.0) and the newer support lib, with the following stacktrace - https://pastebin.mozilla.org/9086010

There might be other incompatibilities that follow the support lib update and because the current version of play-services used is now 2.5 years old I think an upgrade would be welcomed.

Because there are many libraries that needs to be updated and their implementation might have changed in this 2 years period it's probable that this update process will take some time.
Blocks: 1303393, 1286912
Blocks: android-o
Assignee: nobody → andrei.a.lazar
Blocks: 1385464
Priority: -- → P1
Whiteboard: --do_not_change--[priority:high]
Status: NEW → ASSIGNED
No longer blocks: 1385464
Depends on: 1438716, 1385464
Comment on attachment 8987103 [details]
Bug 1463376 - Update to latest Google Play Services version

https://reviewboard.mozilla.org/r/252328/#review258786

I think it very unlikely that this will work in the wild.  Mixing-and-matching versions of the Google Play Services suite is, to the best of my knowledge, unsupported.  Unless we have a reference to a Google-provided source saying "you can do this" we should not do this.

Explain to me why upgrading `-measurement` is not done in this patch?

::: commit-message-6b6f3:3
(Diff revision 1)
> +Bug 1463376 - Update to latest Google Play Services version r?sdaswani
> +
> +Updated google play services version as part of work of the Oreo migration and removed unused libraries from gradle.

Nit: Google Play Services.

It doesn't look like you removed any libraries, either -- just some of our own files.
Attachment #8987103 - Flags: review-
Attachment #8987103 - Flags: review?(sdaswani)
Hello, unfortunately, for analytics, I'm using a different version because google seems to have skipped 15.0.1 for it - https://mvnrepository.com/artifact/com.google.android.gms/play-services-analytics where as for the measurement library I'm using 8.4.0 because it seems that it is the last version released - https://mvnrepository.com/artifact/com.google.android.gms/play-services-measurement.
Flags: needinfo?(nalexander)
(In reply to Andrei Lazar from comment #3)
> Hello, unfortunately, for analytics, I'm using a different version because
> google seems to have skipped 15.0.1 for it -
> https://mvnrepository.com/artifact/com.google.android.gms/play-services-
> analytics where as for the measurement library I'm using 8.4.0 because it
> seems that it is the last version released -
> https://mvnrepository.com/artifact/com.google.android.gms/play-services-
> measurement.

Sorry to let this drop.  Mixing-and-matching bits of GPS doesn't make sense.  Why do we need measurement at all?  I see code to _remove_ measurement stuff around https://searchfox.org/mozilla-central/source/mobile/android/app/build.gradle#462.  I vaguely remember this had to do with Leanplum, which is itself slated for upgrade (right?) in https://bugzilla.mozilla.org/show_bug.cgi?id=1438716.  We should upgrade these things in lockstep, not try to glue old Google code into new Google code.
Flags: needinfo?(nalexander)
Hello Nick, regarding the workaround for bypassing a warning from a google play services base, please bump me if you can find a better solution. Thank you!
Flags: needinfo?(nalexander)
Depends on: 1468487
Comment on attachment 8987103 [details]
Bug 1463376 - Update to latest Google Play Services version

https://reviewboard.mozilla.org/r/252328/#review261690

::: mobile/android/thirdparty/build.gradle:52
(Diff revision 3)
>      if (mozconfig.substs.MOZ_ANDROID_MMA) {
>          implementation "com.android.support:appcompat-v7:$support_library_version"
>          implementation "com.android.support:support-annotations:$support_library_version"
>          implementation "com.google.android.gms:play-services-gcm:$google_play_services_version"
> +        implementation "com.google.android.gms:play-services-basement:$google_play_services_version"
> +        implementation "com.google.android.gms:play-services-ads:$google_play_services_version"

Why do we need the ads library? In view of bug 1420886 this would seem rather unfortunate.
Hi Jan, we needed ads library because Leanplum is using AdvertisingIdClient.class, which, I suppose, on 8.4.0 GPS it was embedded in analytics, but upon updating to 15.0.0 they migrated it to ads.
After further investigations, I decided to use play-services-ads-identifier library, which is the lightest library from ads, and it doesn't include the doubleclick package as specified in bug 1420886. What you think?
Flags: needinfo?(jh+bugzilla)
Attachment #8987103 - Flags: review?(jh+bugzilla)
Comment on attachment 8987103 [details]
Bug 1463376 - Update to latest Google Play Services version

I guess there's no choice if we want to use Leanplum, but thanks for identifying a more lightweight approach. Leaving the rest to Nick, though...
Flags: needinfo?(jh+bugzilla)
Attachment #8987103 - Flags: review?(nalexander)
Attachment #8987103 - Flags: review?(jh+bugzilla)
Attachment #8987103 - Flags: review?(nalexander)
Comment on attachment 8987103 [details]
Bug 1463376 - Update to latest Google Play Services version

https://reviewboard.mozilla.org/r/252328/#review261914

This looks basically okay to me.  Green on try and doesn't break noGPS builds (also on try) then I'm happy.

Thanks for pushing this across the line!

::: build.gradle:119
(Diff revision 5)
>                      "-Xlint:-serial",
>                      // Turn all remaining warnings into errors,
>                      // unless marked by @SuppressWarnings.
>                      "-Werror"]
>              }
> +            if (project.name == 'app') {

Is this still needed?  (I hope not, it looks very hacky.)
Attachment #8987103 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #13)
> Comment on attachment 8987103 [details]
> Bug 1463376 - Update to latest Google Play Services version
> 
> https://reviewboard.mozilla.org/r/252328/#review261914
> 
> This looks basically okay to me.  Green on try and doesn't break noGPS
> builds (also on try) then I'm happy.
> 
> Thanks for pushing this across the line!
> 
> ::: build.gradle:119
> (Diff revision 5)
> >                      "-Xlint:-serial",
> >                      // Turn all remaining warnings into errors,
> >                      // unless marked by @SuppressWarnings.
> >                      "-Werror"]
> >              }
> > +            if (project.name == 'app') {
> 
> Is this still needed?  (I hope not, it looks very hacky.)

Hello Nick, unfortunately yes, because I want to ignore that classfile warning only on app module.
I was actually looking for a better approach to constrain it even more, but until then, I find this approach harmless.
Comment on attachment 8987103 [details]
Bug 1463376 - Update to latest Google Play Services version

Can you please make sure that the correct reviewer(s) is/are set in the commit message and/or Mozreview before publishing an updated version?
Attachment #8987103 - Flags: review?(jh+bugzilla)
Comment on attachment 8987103 [details]
Bug 1463376 - Update to latest Google Play Services version

https://reviewboard.mozilla.org/r/252328/#review262604

This looks about right to me.  Bonus points for not hacking up the dependency resolution process but no need to block if you can't achieve that.

::: mobile/android/app/build.gradle:440
(Diff revision 6)
>  // by moz.build.  Per https://bugzilla.mozilla.org/show_bug.cgi?id=1320310#c14,
>  // this breaks launching in Android Studio; therefore, we only do this for
>  // official automation builds and not for local developer builds.
>  import groovy.xml.XmlUtil
>  
> +// Workaround for fixing sub-dependencies upon gradle error:

Can we avoid this if we pin to 15.0.1 instead of 15.0.0?  Needing this worries me.

Also, please use `ext.google_play_services_version`, not the hard-coded "15.0.0" here.
Attachment #8987103 - Flags: review+
(In reply to Jan Henning [:JanH] from comment #16)
> Comment on attachment 8987103 [details]
> Bug 1463376 - Update to latest Google Play Services version
> 
> Can you please make sure that the correct reviewer(s) is/are set in the
> commit message and/or Mozreview before publishing an updated version?
Flags: needinfo?(andrei.a.lazar)
Attachment #8987103 - Flags: review?(jh+bugzilla)
I am sorry, I cleared you from the commit message, but somehow, it keeps asking for your review T_T
Flags: needinfo?(andrei.a.lazar)
There have been issues building due to dependencies missing.  I think https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8f95a595d88b850e4c5a964b2b43f813dcc52d8 should address them, but we'll see.
Flags: needinfo?(nalexander)
(In reply to Nick Alexander :nalexander from comment #21)
> There have been issues building due to dependencies missing.  I think
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c8f95a595d88b850e4c5a964b2b43f813dcc52d8 should
> address them, but we'll see.

This addresses the Gradle dependency fetching issue, but there are real lint problems here: https://treeherder.mozilla.org/#/jobs?repo=try&author=nalexander@mozilla.com&selectedJob=187684217.  And that lint problem isn't "just lint" -- something is wrong; we're mixing library versions in a way we never should.  Back to you, Andrei, to investigate.
Flags: needinfo?(andrei.a.lazar)
The lint issues are because this patch depends on the 26 support library patch - bug 1385464 and should be applied on top of that.
I tried to reproduce your patch (which that MOZ_ANDROID_MMA flag) and pushed to try with fuzzy [1].

I see that it now fails with [2]:
> A problem occurred configuring project ':geckoview'.
> > Failed to notify project evaluation listener.
>    > Could not resolve all files for configuration ':geckoview:officialWithGeckoBinariesMinApi21DebugCompileClasspath'.
>       > Could not resolve com.android.support:support-v4:26.1.0.
>         Required by:
>             project :geckoview
>          > No cached version of com.android.support:support-v4:26.1.0 available for offline mode.
>          > No cached version of com.android.support:support-v4:26.1.0 available for offline mode.
>       > Could not resolve com.android.support:palette-v7:26.1.0.
>         Required by:
>             project :geckoview
>          > No cached version of com.android.support:palette-v7:26.1.0 available for offline mode.
>          > No cached version of com.android.support:palette-v7:26.1.0 available for offline mode.
>    > Could not get unknown property 'javadocJarOfficialWithGeckoBinariesNoMinApiRelease' for object of type org.gradle.api.internal.artifacts.dsl.DefaultArtifactHandler.

Although when building with the 26 support library alone no such errors occur - [3]
  

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=27c645c2e9b26aeeaa02c1fa8ca75d1114101cb7
[2] https://taskcluster-artifacts.net/HS10iABoR8-RBGwWNdJxjw/0/public/logs/live_backing.log
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fc4a56002b7208fd62ff031859212ad8827b658
Flags: needinfo?(andrei.a.lazar) → needinfo?(nalexander)
(In reply to Petru-Mugurel Lingurar[:petru] from comment #23)
> The lint issues are because this patch depends on the 26 support library
> patch - bug 1385464 and should be applied on top of that.
> I tried to reproduce your patch (which that MOZ_ANDROID_MMA flag) and pushed
> to try with fuzzy [1].
> 
> I see that it now fails with [2]:
> > A problem occurred configuring project ':geckoview'.
> > > Failed to notify project evaluation listener.
> >    > Could not resolve all files for configuration ':geckoview:officialWithGeckoBinariesMinApi21DebugCompileClasspath'.
> >       > Could not resolve com.android.support:support-v4:26.1.0.
> >         Required by:
> >             project :geckoview
> >          > No cached version of com.android.support:support-v4:26.1.0 available for offline mode.
> >          > No cached version of com.android.support:support-v4:26.1.0 available for offline mode.
> >       > Could not resolve com.android.support:palette-v7:26.1.0.
> >         Required by:
> >             project :geckoview
> >          > No cached version of com.android.support:palette-v7:26.1.0 available for offline mode.
> >          > No cached version of com.android.support:palette-v7:26.1.0 available for offline mode.
> >    > Could not get unknown property 'javadocJarOfficialWithGeckoBinariesNoMinApiRelease' for object of type org.gradle.api.internal.artifacts.dsl.DefaultArtifactHandler.
> 
> Although when building with the 26 support library alone no such errors
> occur - [3]
>   
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=27c645c2e9b26aeeaa02c1fa8ca75d1114101cb7

Your try build doesn't include all of my changes, some of which remove the `google()` repository.  I believe that's breaking dependency fetching.  There's a mechanism in place to populate the Maven repositories _differently_ for dependency fetching and for regular builds.  See https://searchfox.org/mozilla-central/search?q=GRADLE_MAVEN_REPOSITORIES&path= -- you can see that the Maven repositories come from disk for regular builds:

	mobile/android/config/mozconfigs/common
24	export GRADLE_MAVEN_REPOSITORIES="file://$topsrcdir/android-gradle-dependencies/jcenter","file://$topsrcdir/android-gradle-dependencies/google"
	
and from the network for dependency builds:

	mobile/android/config/mozconfigs/android-api-16-gradle-dependencies/nightly
18	export GRADLE_MAVEN_REPOSITORIES="http://localhost:8081/nexus/content/repositories/jcenter/","http://localhost:8081/nexus/content/repositories/google/"

Putting in google() is going around that, as I can see in the build logs from fetches like

[task 2018-07-12T08:27:41.110Z]  1:45.57 Download https://dl.google.com/dl/android/maven2/com/android/support/palette-v7/26.1.0/palette-v7-26.1.0.pom
[task 2018-07-12T08:27:41.110Z]  1:45.57 Download https://dl.google.com/dl/android/maven2/com/android/support/support-v4/26.1.0/support-v4-26.1.0.pom

(i.e., not like)

[task 2018-07-12T08:27:41.110Z]  1:45.57 Download http://localhost:8081/nexus/content/repositories/jcenter/com/android/support/support-core-utils/26.1.0/support-core-utils-26.1.0.pom
[task 2018-07-12T08:27:41.111Z]  1:45.57 Download http://localhost:8081/nexus/content/repositories/jcenter/com/android/support/support-media-compat/26.1.0/support-media-compat-26.1.0.pom
[task 2018-07-12T08:27:41.111Z]  1:45.57 Download http://localhost:8081/nexus/content/repositories/jcenter/com/android/support/support-compat/26.1.0/support-compat-26.1.0.pom

which will get added to the fetched dependencies

in the android-gradle-dependencies task at https://taskcluster-artifacts.net/LSkTBLiASuGvSRGTcHBgPg/0/public/logs/live_backing.log.

I've pushed https://treeherder.mozilla.org/#/jobs?repo=try&revision=f660def85057621b2df707680738f84cde92c5d9 which hopefully works better.

Locally I don't need the resolution strategy hunk -- please test without it.  If it's needed something bad is happening and I need to understand what that is.

In general, you can't don't drop suggested changes and expect the resulting pushes to succed.  In this case, I see that I didn't explain why the offending lines were removed in my commit message, and I will try to be more explicit next time.
Flags: needinfo?(nalexander)
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/267ff4f4ca9b
Update to latest Google Play Services version. r=nalexander
https://hg.mozilla.org/mozilla-central/rev/267ff4f4ca9b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.