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

Android build #852

Merged
merged 4 commits into from
Sep 17, 2018
Merged

Android build #852

merged 4 commits into from
Sep 17, 2018

Conversation

briantq
Copy link
Contributor

@briantq briantq commented Sep 15, 2018

This should resolve all the Android build problems related to the Crashlytics Pull Request #784. I spent last night and this morning figuring out how the build system work, both Cordova's and the scripts in the project.

The issue for #837 is that the Crashlytics Pull Request removed some keys that Firebase needed. As in the comments, it was outside the scope of the PR. Had those not been removed, it should have built correctly.

Then the fix in PR #816 didn't resolve the initFirebase issue. It did get closer to the right way to do it though, which is to use the google services plugin instead of modifying keys directly.

So to get the google play services plugin to work Issue #837, we need to modify the build.gradle in the root of the project. Since that was not possible via the Cordova hooks, I rewrote the add plugin/remove plugin hooks (as they did not work).

After the first commit I was able to get it to compile, but then when I ran I encountered #848. This was due to a key that the Crashlytics plugin randomly generates for each build. Instead of trying to add code to generate the key, I instead opted to use the recommended way of calling the io.fabric gradle plugin. To do this, we again needed to modify the root build.gradle.

Lastly, since I couldn't figure out how it worked in v1.0.5, I found that there was a step in the prepare hook that set some of the keys firebase is looking for that are generated by the google services plugin. Since we are now using the recommended way to generate the keys, we no longer need to hack the custom keys ourselves so I removed the code.

After all of that, I think things should now be working, so I am going to enjoy the rest of my weekend :)

@soumak77
Copy link
Contributor

@briantq I'm still a bit confused why we don't just add the necessary lines to build.gradle instead of having to do this programmatically. Some build systems can't run the hooks, such as phonegap, so those developers will need to modify files themselves, which is not sustainable.

@soumak77
Copy link
Contributor

We also use the hook to setup the iOS build. I'm wondering if there is a way around that as well.

@soumak77
Copy link
Contributor

Also, if we must update the build files in the hook, we should add a test case which verifies the build.gradle file is being updated properly. This would be possible with our existing tests since we don't need to run the app to verify the file has the correct content.

I'm not sure whether there is a way to verify the iOS build files are updated properly, but if there is, we should add a test case for that as well.

@briantq
Copy link
Contributor Author

briantq commented Sep 17, 2018

@soumak77 There are two build.gradle files in an Android project, one in the root and one in the app directory. The one in the app directory of the most commonly modified one. In fact, this is where the dependencies go when we add them to the plugin.xml. The build.gradle that we supply is run as an extension of the app's build.gradle. This is good for the most part, but is not sufficient to run plugins (custom build code). We need to run the google service and fabric plugin as part of the build. As such, they need to go into the project's build.gradle as they are run in the app build. When gradle runs the app's build.gradle and see's the plugin reference, it needs to know how to resolve it. It's sort of like a build dependency.

Hence, the only way to get this to work is to add the dependencies in the root project build.gradle. Adding it to our build.gradle or as dependencies in our plugin.xml won't resolve the plugin references as the build has already started by the time those are executed.

@briantq
Copy link
Contributor Author

briantq commented Sep 17, 2018

@soumak77 Can you point me to documentation on the PhoneGap hook limitations? I couldn't find anything when I googled.

@soumak77
Copy link
Contributor

Thanks for the detailed explanation. We should provide steps for those that can't run these hooks (similar to the existing explanation for phonegap builds). Probably just linking to the firebase docs for how to setup Crashlytics on android and iOS would be sufficient.

@soumak77
Copy link
Contributor

I don't use phonegap, so I'm unaware of the exact limitations. I'm just going off of what was mentioned in the README.

@briantq
Copy link
Contributor Author

briantq commented Sep 17, 2018

@soumak77 you read the documentation? I've heard about people doing that but have never seen it in practice :-)

You are right that adding these hooks do make it more work assuming this is still the case.

Looking at this documentation it looks like hooks are supported. I too don't use it so not sure what's really possible.

@soumak77
Copy link
Contributor

This project was created a long time ago, so perhaps phonegap has since added support for cordova hooks. Though as mentioned by @julienkermarec, gitlab doesn't appear to support hooks.

Copy link
Contributor

@soumak77 soumak77 left a comment

Choose a reason for hiding this comment

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

Changes are approved. Only suggestion I have is to also include a test case to verify the build files are in fact being updated as expected. This will also ensure no regression in future versions.

Since we need to get this fix out sooner than later, I'd be okay releasing a new version without the desired test cases assuming we have enough users manually test this fix.

@soumak77
Copy link
Contributor

Seems we've had at least 1 developer verify this issue. More is obviously welcomed, however, I would be fine releasing a new version with this fix.

@briantq let me know when you feel confident in the fix and I'll publish a new release

@briantq
Copy link
Contributor Author

briantq commented Sep 17, 2018

@soumak77 I am cool releasing the fix.

As far as the tests, is the proper test to verify the file contents or to try to run a gradle sync? If we were able to actually try to sync/build the project, this would have shown up much earlier. Additionally, if someone adds another plugin reference, that will catch an error updating the root build.gradle.

I have never written any tests for cordova so would need some help/guidance on where to start.

@soumak77
Copy link
Contributor

soumak77 commented Sep 17, 2018

The tests I have created so far run the following commands:

  1. cordova platform add
  2. cordova plugin add
  3. cordova platform build

If cordova reports a failure, the tests fail. In the specific case of android, I have also created a test which verifies this plugin works with other plugins (specifically cordova-plugin-request-location-accuracy). If you just try to add that plugin, the build will fail. In order to get that specific test to pass, the cordova-android-play-services-gradle-release and cordova-android-firebase-gradle-release plugins must also be added.

For some reason, the android build is passing without needing the fix in this PR, so it must be some environment specific use case that the tests do not run under (or difference in cordova/platform version). If we can figure out a way to get the android build to fail as others have observed, then we wouldn't need to verify the build files manually.

@soumak77 soumak77 merged commit 5a3c08d into arnesson:master Sep 17, 2018
@jtushar53
Copy link

For me its not working , below is the error i am getting. I have added cordova-android-play-services-gradle-release and cordova-android-firebase-gradle-release plugin and yes i am using cordova-plugin-request-location-accuracy plugin too

FAILURE: Build failed with an exception.

* Where:
Script '/Users/Tushar/Projects/sign_on_glass/platforms/android/cordova-plugin-firebase/app-build.gradle' line: 27

* What went wrong:
A problem occurred evaluating project ':app'.
> Plugin with id 'com.google.gms.google-services' not found.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

* Get more help at https://help.gradle.org

BUILD FAILED in 2m 26s

@giovinazzo-kevin
Copy link

Hey @jtushar53, did you find out what caused the problem eventually? I'm running into the same issue here, but it's hard to pinpoint the reason when you're offloading everything to Phonegap.

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

Successfully merging this pull request may close these issues.

4 participants