Skip to content

Final android x migration - #9977

Merged
nbradbury merged 11 commits into
developfrom
final-android-x-migration
Jun 5, 2019
Merged

Final android x migration#9977
nbradbury merged 11 commits into
developfrom
final-android-x-migration

Conversation

@malinajirka

@malinajirka malinajirka commented Jun 4, 2019

Copy link
Copy Markdown
Contributor

Fixes #9923

This PRs migrates SupportLibrary -> AndroidX.

  • I had to update gradle build tools to v3.3.2
  • Manually fix a couple of minor issues - mostly references to support.R
  • Fix import ordering

The new build tools version introduced a couple of new build warnings

WARNING: API 'variant.getJavaCompile()' is obsolete and has been replaced with 'variant.getJavaCompileProvider()'. It will be removed at the end of 2019.

  • This is known issue with the Kotlin plugin.

WARNING: API 'variantOutput.getProcessManifest()' is obsolete and has been replaced with 'variantOutput.getProcessManifestProvider()'. It will be removed at the end of 2019. WARNING: API 'variant.getMergeAssets()' is obsolete and has been replaced with 'variant.getMergeAssetsProvider()'.It will be removed at the end of 2019.

  • It seems these two issuse are caused by SentryPlugin as the stacktraces contain io.sentry.android.gradle.SentryPlugin$_apply_closure2$_closure3$_closure4.doCall(SentryPlugin.groovy:153). and io.sentry.android.gradle.SentryPlugin.getDebugMetaPropPath(SentryPlugin.groovy:136). However, it doesn't mean the other libraries don't use them. It just means these methods were first used by Sentry.

I think we don't need to worry about any of them for now. These are just warnings that the libs use obsolete gradle methods. I believe we'll update the libs before we update gradle build tools to v6 anyway. Wdyt?

To test:

  • Basic app flows - login, publish a post...

Update release notes:

  • [ x ] There are no user facing changes

Note:
You need to manually update gradle.properties (see gradle.properties-example)

@malinajirka malinajirka added this to the 12.7 milestone Jun 4, 2019
@peril-wordpress-mobile

Copy link
Copy Markdown
Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@aforcier

aforcier commented Jun 4, 2019

Copy link
Copy Markdown
Contributor

@malinajirka do you have a source for this?

WARNING: API 'variant.getJavaCompile()' is obsolete and has been replaced with 'variant.getJavaCompileProvider()'. It will be removed at the end of 2019.

This is known issue with the Kotlin plugin.

Perhaps there is an issue with the Kotlin plugin, but this warning was also being generated by the Bintray plugin used by the utils and analytics libs: #9436

Did you try Bintray plugin 0.9.1?

@malinajirka

Copy link
Copy Markdown
Contributor Author

do you have a source for this?

Build tools it's the source -> Full Message

WARNING: API 'variant.getJavaCompile()' is obsolete and has been replaced with 'variant.getJavaCompileProvider()'.
It will be removed at the end of 2019.
For more information, see https://d.android.com/r/tools/task-configuration-avoidance.
REASON: The Kotlin plugin is currently calling this API. We are working to solve this.
WARNING: Debugging obsolete API calls can take time during configuration. It's recommended to not keep it on at all times.

Perhaps there is an issue with the Kotlin plugin, but this warning was also being generated by the Bintray plugin used by the utils and analytics libs: #9436

Yes, as I mentioned it prints just the first encounter with the obsolete api. In this case the first encounter was in the kotlin plugin.

Did you try Bintray plugin 0.9.1?

Yes, I did and it didn't help. Imho we shouldn't spent time looking into it until we upgrade gradle to v6. We might have much fewer issues as most of the libraries will be updated by then.

@malinajirka malinajirka changed the title Final android x migration WIP: Final android x migration Jun 4, 2019
@malinajirka

Copy link
Copy Markdown
Contributor Author

Lint is failing on some false positives. I'll continue looking into it tomorrow.

@malinajirka malinajirka changed the title WIP: Final android x migration Final android x migration Jun 5, 2019
@malinajirka

Copy link
Copy Markdown
Contributor Author

This PR is finally ready for review! ;)

There are several open PRs in which authors will need to manually resolve conflicts, but I think we'll never be in a state with no open PRs or working branches.

I think at least two developers should review this PR before we merge it. Wdyt?

@nbradbury

Copy link
Copy Markdown
Contributor

I think at least two developers should review this PR before we merge it. Wdyt?

Sounds good to me! I'll try to get my review done quickly.

@nbradbury

Copy link
Copy Markdown
Contributor

@malinajirka The build is failing for me with this:

Execution failed for task ':WordPress:processVanillaDebugManifest'.
> Manifest merger failed : Attribute application@appComponentFactory value=(androidx.core.app.CoreComponentFactory) from [androidx.core:core:1.0.2] AndroidManifest.xml:22:18-86
  	is also present at [com.android.support:support-compat:28.0.0] AndroidManifest.xml:22:18-91 value=(android.support.v4.app.CoreComponentFactory).
  	Suggestion: add 'tools:replace="android:appComponentFactory"' to <application> element at AndroidManifest.xml:7:5-11:19 to override.

This happens with both the release and debug build variants.

@malinajirka

Copy link
Copy Markdown
Contributor Author

Have you updated your gradle.properties with the updated version from gradle.properties-example?
You need to add android.useAndroidX=true android.enableJetifier=true. Let me know if it helped;).

@nbradbury

Copy link
Copy Markdown
Contributor

Have you updated your gradle.properties with the updated version from gradle.properties-example?

🤦‍♂

Sorry, @malinajirka - that was, of course, the problem. Builds fine now.

@nbradbury

Copy link
Copy Markdown
Contributor

@malinajirka I ran through all the primary features and everything works just fine. I know we want another reviewer still, but this gets a 👍 from me.

@malinajirka

Copy link
Copy Markdown
Contributor Author

Glad to hear that! Thank you so much for the review @nbradbury 🥇!!

@aforcier

aforcier commented Jun 5, 2019

Copy link
Copy Markdown
Contributor

Did you try Bintray plugin 0.9.1?

Yes, I did and it didn't help. Imho we shouldn't spent time looking into it until we upgrade gradle to v6. We might have much fewer issues as most of the libraries will be updated by then.

Ah, that's a shame. The Gradle warnings are pretty annoying IMO when building from the IDE, which is why it made sense to avoid updating the Gradle plugin until it was needed. Agreed that it's not enough of a concern if it's required for AndroidX support though.

And, yes, hopefully by the time it's actually removed these libraries have sorted things out. 🤞 It would be nice if we could tell Gradle not to bother with the Bintray import when building those two libraries as subprojects since it's not relevant - but I'm not sure if there's a way to do that. May be worth looking into if the warnings get on our nerves and Bintray is the last cause of a warning.

@aforcier

aforcier commented Jun 5, 2019

Copy link
Copy Markdown
Contributor

Gave this a look over:

  • Hard to comb through all the code changes, but the fundamental parts make sense 👍
  • Builds looks okay - I noticed a lint error but it looks to be unrelated and already fixed in develop: Fix './gradlew build' in libs/mocks #9972 👍
  • Ran through a variety of app flows, didn't run into any issues 👍

One problem though, I can't seem to get the e2e tests to run (./gradlew WordPress:connectedVanillaDebugAndroidTest --stacktrace) - they fail to build with a org.gradle.api.GradleException: Compilation error.. Are you able to run them @malinajirka ?

Still looking things over but though I'd report the e2e issue and my partial 👍

@nbradbury

Copy link
Copy Markdown
Contributor

I'm also seeing e2e test woes, but I think we're still good to merge this and deal with that separately. CircleCI is green so the sooner we get this into develop the better! :shipit:

@nbradbury
nbradbury merged commit 50f98a1 into develop Jun 5, 2019
@malinajirka

Copy link
Copy Markdown
Contributor Author

@aforcier I'm able to run them - perhaps you haven't pulled the most recent changes made here 7973e84 or you might need to run clean? Having said that, they don't succeed, but I'm not even sure they were succeeding before.

@aforcier

aforcier commented Jun 5, 2019

Copy link
Copy Markdown
Contributor

@malinajirka yep the fixes in 7973e84 were needed - with those and the latest gradle.properties tests are running. A few tests are failing but they seem to be in develop as well so I don't think it's due to these changes.

@oguzkocer
oguzkocer deleted the final-android-x-migration branch April 15, 2020 12:27
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.

Migration to AndroidX

4 participants