Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for EnableGoogleServicesPlugin #438

Closed
wants to merge 1 commit into from

Conversation

chemerisuk
Copy link
Contributor

Platforms affected

Android

What does this PR do?

@macdonst proposed to add new preference to the platform, that runs google services plugin.

What testing has been done on this change?

Checked locally.

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@codecov-io
Copy link

Codecov Report

Merging #438 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #438   +/-   ##
=======================================
  Coverage   44.16%   44.16%           
=======================================
  Files          17       17           
  Lines        1698     1698           
  Branches      314      314           
=======================================
  Hits          750      750           
  Misses        948      948

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59e3b90...f2a4d92. Read the comment docs.

@macdonst macdonst requested a review from infil00p April 25, 2018 20:40
@@ -322,3 +323,7 @@ for (def func : cdvPluginPostBuildExtras) {
if (hasProperty('postBuildExtras')) {
postBuildExtras()
}

if (cdvHelpers.getConfigPreference('EnableGoogleServicesPlugin', 'false').toBoolean()) {
apply plugin: 'com.google.gms.google-services'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chemerisuk I've had to make this line:

apply plugin: 'com.google.gms.googleservices.GoogleServicesPlugin'

because the class must be used instead of id(string) to be able to apply plugin from non-root gradle file. I haven't tested the PR yet but I'm assuming the reason you are using the ID is because it is a root gradle file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because this is the root Gradle file. Worked fine on my side.

For testing I commented changes from cordova-support-google-services and inserted PR code manually. Then rebuilt the project and tried to start (btw google services v15 trigger error at runtime, if ‘google.services’ was not processed properly while previously it was build time error).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@macdonst
Copy link
Member

@infil00p wanna take a look at this?

@janpio janpio added this to 🐣 New PR / Untriaged in Apache Cordova: Platform Pull Requests Sep 2, 2018
@janpio janpio moved this from 🐣 New PR / Untriaged to ✅ Approved, waiting for Merge in Apache Cordova: Platform Pull Requests Sep 2, 2018
@@ -30,6 +30,7 @@ buildscript {

dependencies {
classpath 'com.android.tools.build:gradle:3.1.0'
classpath 'com.google.gms:google-services:3.2.1'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to wrap this classpath in an if condition, like how the apply is done?
It would be nice to minimize the amount of downloading of build related libraries if they are not needed.

if (cdvHelpers.getConfigPreference('EnableGoogleServicesPlugin', 'false').toBoolean()) {
  classpath 'com.google.gms:google-services:3.2.1'
}

Copy link
Contributor Author

@chemerisuk chemerisuk Sep 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the object cdvHelpers is not available at the dependency scope. Project build failed with the error:

Could not get unknown property 'cdvHelpers' for object of type org.gradle.api.internal.artifacts.dsl.dependencies.DefaultDependencyHandler.

Happens when I apply the changes you suggest.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chemerisuk Yes, it seems cordova.gradle would need to be applied within the scope so it becomes available.

    dependencies {
        apply from: '../CordovaLib/cordova.gradle'

        classpath 'com.android.tools.build:gradle:3.1.0'

        if(cdvHelpers.getConfigPreference('EnableGoogleServicesPlugin', 'false').toBoolean()) {
            println 'Adding classpath "com.google.gms:google-services:3.2.1"'

            classpath 'com.google.gms:google-services:3.2.1'
        }
    }

@dpogue Would you know off-hand if something like this would be problematic? The cordova.gradle content seems to be only helper methods. Adding it seems to solve the desire to make the classpaths conditional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about Gradle, and my experience consists mostly of swearing at it.

I believe you're correct that cordova.gradle is helper methods, but I don't know anything about scoping or when it's safe to import (apply?) stuff.

@erisu erisu mentioned this pull request Sep 11, 2018
3 tasks
@erisu erisu mentioned this pull request Sep 26, 2018
33 tasks
@dpogue dpogue mentioned this pull request Jan 2, 2019
@dpogue dpogue added this to the 9.0.0 milestone May 8, 2019
@erisu erisu added this to Reviewer approved in Release Plan - 9.1.0 Jan 6, 2020
@erisu
Copy link
Member

erisu commented Jan 14, 2020

@chemerisuk I would like to try and get this PR updated and merged before the upcoming major release.

If possible, could you rebase the PR with master and fix the conflicting files?

Also, as I proposed in a previous comment, could you wrap the classpath 'com.google.gms:google-services:3.2.1' with the EnableGoogleServicesPlugin?

The code snippet that I provided in a previous comment will work as intended.

You will need to add the apply from: '../CordovaLib/cordova.gradle' to the dependencies scope to have access to the helper methods. I had tested this locally and it should not cause any problems.

Lastly, since this PR was created awhile back, I had some other nice to have that could be applied with this PR, if you can do this, it will be great. If you do not have the time, I can do it in a second PR if needed.

  1. Bump the Google Services version to a newer version.

I recall that the phonegap-plugin-push plugin had build issues with 4.2.0 so maybe bump the version to 4.1.0 which people reported working.

Actually, with this PR and the phonegap-plugin-push plugin, there is a build issue which will need investigation and I am suspecting it might be related that the phonegap-plugin-push plugin dependency on cordova-support-google-services.

  1. Could we introduce a second preference config options: GoogleServicesPluginVersion which would allow users the ability to decide what version they would want to use. (Default to 4.1.0).

Please let me know if you can do this or if I should create a new PR based on your changes and apply the additional changes.

@erisu erisu mentioned this pull request Jan 15, 2020
2 tasks
@erisu
Copy link
Member

erisu commented Jan 15, 2020

@chemerisuk I went ahead and created a second PR to be able to quickly fix the merge conflicts as well as apply the additional change request that I posted.

This PR will close as soon as my PR is merged. Since I cherry-picked your commit in, it will still contain your contribution.

Thank you for your contribution and sorry that it took this long to merge in your commit.

@chemerisuk
Copy link
Contributor Author

@erisu thanks for the update. Unfortunately my current environment is a bit different now, so it will be hard to test those changes properly. So please go ahead with the PR, it's a long waited one.

If you have time could you post a link to a compilation problem at phonegap-plugin-push you've mentioned.

@erisu erisu closed this in #893 Jan 15, 2020
Apache Cordova: Platform Pull Requests automation moved this from ✅ Approved, waiting for Merge to ☠️ Closed/Abandoned Jan 15, 2020
Release Plan - 9.1.0 automation moved this from Reviewer approved to Done Jan 15, 2020
@erisu
Copy link
Member

erisu commented Jan 16, 2020

@chemerisuk I do not have an issue ticket regarding the issues I described with the phonegap-plugin-push plugin.

But, I pushed a PR that briefly explains changes to the plugin that will be needed to use these new preference options without having a build issue. It removed the enable google services plugin dependency.

The issue the plugin had was a conflict with the new preferences enabling the Google Services in the buildscript while the enable google services plugin dependency did the same thing.

Here is the PR: phonegap/phonegap-plugin-push#2873

It is currently labeled as DRAFT because we will need to first release the major.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants