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

Upgrade Gradle to 5.4.1 & Gradle plugin to 3.5.1 #10583

Merged
merged 16 commits into from Oct 17, 2019

Conversation

oguzkocer
Copy link
Contributor

This PR upgrades Gradle version to 5.4.1 and Gradle plugin version to 3.5.1. While doing this upgrade I ended up having to upgrade WellSql wordpress-mobile/wellsql#14 and FluxC wordpress-mobile/WordPress-FluxC-Android#1397. I also had a lot of different build errors related to Volley and some other dependencies, so I added/upgraded/excluded related dependencies. I aimed for the smallest set of changes for this upgrade and this was the best I could get. Let me know if any of these changes don't make sense or unnecessary.

I am opening this PR as a Draft PR since it depends on the linked FluxC PR and we'll need to update the FluxC version before we can merge this PR.

To test:
Do a clean build and make sure the app builds and works as expected.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@oguzkocer oguzkocer added this to the 13.5 milestone Oct 8, 2019
@peril-wordpress-mobile
Copy link

Messages
📖

This PR contains changes in the subtree libs/login/. It is your responsibility to ensure these changes are merged back into wordpress-mobile/WordPress-Login-Flow-Android. Follow these handy steps!
WARNING: Make sure your git version is 2.19.x or lower - there is currently a bug in later versions that will corrupt the subtree history!

  1. cd WordPress-Android
  2. git checkout issue/upgrade-gradle-to-5.4.1
  3. git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/10583
  4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/10583 and open a new PR.

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 8, 2019

You can test the changes on this Pull Request by downloading the APK here.

@oguzkocer
Copy link
Contributor Author

@malinajirka If you have any extra time, do you think you could take a look at the test issue. I have looked into it and it looks like it's related to Robolectric version, however upgrading that creates another issue. Once I manually add the guava dependency, I get yet another issue. I tried to use a different version, but had no luck with that either.

I think this might be the last step to upgrade the Gradle version. We have some lint issues, but they seem rather straightforward to fix, although there are quite a few of them, so I am thinking we do that in a separate PR first.

@malinajirka
Copy link
Contributor

malinajirka commented Oct 9, 2019

Thanks Oguz! ;) The current change set LGTM!

I've updated Robolectric version to 4.3. When I run ./gradlew --stacktrace testVanillaRelease locally it passes. However, the CI seems to timeout - perhaps when downloading ("Downloading: org/robolectric/android-all/9-robolectric-4913185-2/android-all-9-robolectric-4913185-2.jar" - it took quite some time on my machine).

Regarding the lint issues
UseSparseArrays - I'd consider suppressing this warning at a project level or decreasing its severity to info. Sparse arrays can cause weird behavior in unit tests and imho the performance benefits aren't significant anyway. Wdyt?

StaticFieldLeak - This set of errors is pretty weird as they should be suppressed through the lint-baseline.xml. Not sure what's going on there 🤷‍♂

DefaultLocale, DiffUtilEquals - These seem like issues we should probably fix 🤔

@maxme @hypest I'm not sure how much related this is, but just to be safe. Could you please confirm these changes won't break the Gutenberg development flow. Thanks!

@malinajirka
Copy link
Contributor

However, the CI seems to timeout - perhaps when downloading ("Downloading: org/robolectric/android-all/9-robolectric-4913185-2/android-all-9-robolectric-4913185-2.jar" - it took quite some time on my machine).

I've temporarily increased the no output timeout to 30 minutes so the CI would have enough time to download the file and store it into a cache. It seems that it worked as expected, so the only thing left is fixing the lint issues ;).

@oguzkocer oguzkocer mentioned this pull request Oct 9, 2019
2 tasks
@oguzkocer
Copy link
Contributor Author

Thank you for your help @malinajirka! 🙇 I've separated the lint PR #10593, let's discuss the issues there and merged it into this branch when it's ready.

Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

As mentioned in #10593 (review) I tested the changes and everything seems to be working as expected.

I'm approving this PR to speed things up but I'd feel safer if someone from the Gutenberg team verified it doesn't break their development flow.

Really great job @oguzkocer !!

@mchowning
Copy link
Contributor

👋 @malinajirka and @oguzkocer ! This is great! Thanks for taking this on.

I am seeing a build failure from this branch when I run ./gradlew assembleWasabiDebug with wp.BUILD_GUTENBERG_FROM_SOURCE=true in gradle.properties. We use this flag for development of gutenberg to see gutenberg changes in WPAndroid. The error I'm seeing seems to be the result of it not being able to find the AppLog class for some reason.

Build error logs
> Task :react-native-gutenberg-bridge:compileDebugJavaWithJavac FAILED

/Users/matt/dev/a8c/WordPress-Android/libs/gutenberg-mobile/react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java:35: error: cannot find symbol
import org.wordpress.android.util.AppLog;
                                 ^
  symbol:   class AppLog
  location: package org.wordpress.android.util

/Users/matt/dev/a8c/WordPress-Android/libs/gutenberg-mobile/react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java:236: error: package AppLog does not exist
                        AppLog.d(AppLog.T.EDITOR, message);
                                       ^

/Users/matt/dev/a8c/WordPress-Android/libs/gutenberg-mobile/react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java:236: error: cannot find symbol
                        AppLog.d(AppLog.T.EDITOR, message);
                        ^
  symbol: variable AppLog

/Users/matt/dev/a8c/WordPress-Android/libs/gutenberg-mobile/react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java:239: error: package AppLog does not exist
                        AppLog.i(AppLog.T.EDITOR, message);
                                       ^

/Users/matt/dev/a8c/WordPress-Android/libs/gutenberg-mobile/react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java:239: error: cannot find symbol
                        AppLog.i(AppLog.T.EDITOR, message);
                        ^
  symbol: variable AppLog

/Users/matt/dev/a8c/WordPress-Android/libs/gutenberg-mobile/react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java:242: error: package AppLog does not exist
                        AppLog.w(AppLog.T.EDITOR, message);
                                       ^

/Users/matt/dev/a8c/WordPress-Android/libs/gutenberg-mobile/react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java:242: error: cannot find symbol
                        AppLog.w(AppLog.T.EDITOR, message);
                        ^
  symbol: variable AppLog

/Users/matt/dev/a8c/WordPress-Android/libs/gutenberg-mobile/react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java:245: error: package AppLog does not exist
                        AppLog.e(AppLog.T.EDITOR, message);
                                       ^

/Users/matt/dev/a8c/WordPress-Android/libs/gutenberg-mobile/react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java:245: error: cannot find symbol
                        AppLog.e(AppLog.T.EDITOR, message);
                        ^
  symbol: variable AppLog

I have seen an error similar to this in a different context, so it is possible there is something going on with my setup, but I don't see this error when I build off of develop so I don't think it's just me. With that said, it wouldn't be a bad idea to verify that someone else is seeing the same issue when they build on their machine.

I am still in support rotation today, so I can't dig into this now, but I would be happy to take a closer look at this on Monday if you'd like.

malinajirka and others added 3 commits October 11, 2019 13:05
String.capitalize(Locale) call crates a lint error claiming that the default locale usage is a source of bugs. This should not happen when we are already passing a locale to it. In order to get around this issue, a wrapper extension method is introduced so we can suppress the lint warning in a single place, document the reason for it and make it easy to remove it in the future.
@mchowning
Copy link
Contributor

Following up on the issue with building gutenberg from source enabled. I merged a fix for that that will be included in the gutenberg-mobile release that should get merged into the develop branches for both WPAndroid and mogile-gutenberg this Friday. After that, we will be able to build gutenberg from source with this PR.

If you would like to merge this sooner (i.e., this week), just let me know. I'll just need to do two quick things to make that possible:

  1. Update the gutenberg-mobile submodule ref in this PR so that my fix from gutenberg-mobile is included (may also need to merge in the latest changes from develop into this PR in order to avoid a merge conflict with my change to the submodule ref); and
  2. Merge my fix into the gutenberg-mobile develop branch so that anyone doing development this week has my fix.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 17, 2019

You can test the changes on this Pull Request by downloading the APK here.

Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

Building gutenberg from source is working great for me now. Thanks @oguzkocer !

@oguzkocer oguzkocer marked this pull request as ready for review October 17, 2019 21:44
Copy link
Member

@shiki shiki left a comment

Choose a reason for hiding this comment

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

Did a quick review of the code and tested the app randomly as well. LGTM! Thank you, everyone.

@shiki shiki merged commit efed73c into develop Oct 17, 2019
@shiki shiki deleted the issue/upgrade-gradle-to-5.4.1 branch October 17, 2019 22:00
@maxme
Copy link
Contributor

maxme commented Oct 21, 2019

Thanks for checking the gutenberg flow @mchowning, and thanks @oguzkocer for doing this change.

@jkmassel jkmassel mentioned this pull request Oct 24, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants