Skip to content

Drop jetifier#15675

Merged
malinajirka merged 33 commits intotrunkfrom
drop-jetifier-2
Apr 19, 2022
Merged

Drop jetifier#15675
malinajirka merged 33 commits intotrunkfrom
drop-jetifier-2

Conversation

@malinajirka
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka commented Dec 8, 2021

Merge Instructions:

  1. Review and approve (don't merge) - Update react-native-video from upstream gutenberg-mobile#4361
  2. Review this PR
  3. Merge Aztec https://github.com/wordpress-mobile/AztecEditor-Android/pull/946/files
  4. Merge Stories Drop jetifier Automattic/stories-android#717
  5. Merge GB PR - Update react-native-video from upstream gutenberg-mobile#4361
  6. Merge GB PR which updates Aztec Version - it doesn't exist yet
  7. Update GB hash/version in this repo
  8. Review and Merge upgrade of test dependencies - Update espresso and AndroidX test dependencies #16170 (technically not a blocker)
  9. Merge the branch in .mobile-secrets into trunk
  10. Update hash and branch in .configure (9f811ea)
  11. Remove "Not ready for merge" label
  12. Merge this PR
  13. Optional: Ping the team that they can change android.enableJetifier=true to android.enableJetifier=false in their gradle.properties or run ./gradlew configureApply.

This PR migrates all libraries to versions which are migrated to AndroidX so we can disable jetifier on our builds. Jetifier was always intended to be used only temporarily and is planned to be removed from future versions of AGP.

List of changes

  • Bumps Stetho version to 1.6.0 - the only change is that the version is migrated to AndroidX - https://github.com/facebook/stetho/releases
  • Bumps ExoPlayer from 2.9.3 to 2.12.3 -> this was the most challenging task as ExoPlayer is used in both MediaPreivewFragment and Gutenberg-Mobile => we had to bump the version in both Gutenberg and MediaPreviewFragment. (This work isn't done yet, the GB part is still WIP)
  • Updates WordPress utils version to a version which is migrated to AndroidX -> I bumped the version in all the modules to make the version consistent
  • Adds our S3 repo to Analytics and Networking modules so the new version of WordPress utils is reachable
  • Disables jetifier - updated gradle.properties and .configure
  • "Manually" (using cmd tool) Jetified the Tenor library since it hasn't been updated since 2018 and I don't think it'll get updated any time soon
  • Updated Aztec version with version that uses the updated version of WordPress utils

To test:
Make sure to disable Jetifier in your local gradle.properties => Smoke test the app - anything you can think of.

  1. Open Media Library
  2. Open a detail of a video
  3. Tap on the preview
  4. Make sure the preview is displayed correctly and you can play the video

  1. Create a new post
  2. Add image block
  3. Select "Choose from Tenor"
  4. Type "Cat"
  5. Add a gif

  1. Create a new post
  2. Add Cover block
  3. Add Video
  4. Choose a video file
  5. Make sure the video preview works

Regression Notes

  1. Potential unintended areas of impact
  • Video previews (media library)
  • Video blocks in Gutenberg
  • Adding gifs from Tenor
  1. What I did to test those areas of impact (or what existing automated tests I relied on)
    The above "to test" section

  2. What automated tests I added (or what prevented me from doing so)

None - this is a technical improvement which doesn't change the UX

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Dec 8, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Dec 8, 2021

You can test the changes on this Pull Request by downloading the APKs:

Sometime when we specify version using a commithash eg.
`trunk-1ed207c03d2242b6fc3d74f9e388e9163cbc82a6` a pinned version wins and eg. version `1.22` is used instead of the
newer `trunk-1ed207c03d2242b6fc3d74f9e388e9163cbc82a6`.
implementation "$gradle.ext.wputilsBinaryPath:$wordPressUtilsVersion"
testImplementation "$gradle.ext.wputilsBinaryPath:$wordPressUtilsVersion"
debugImplementation "$gradle.ext.wputilsBinaryPath:$wordPressUtilsVersion"
implementation ("$gradle.ext.wputilsBinaryPath") {
Copy link
Copy Markdown
Contributor Author

@malinajirka malinajirka Dec 9, 2021

Choose a reason for hiding this comment

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

All three were identical, so I used only implementation - or am I missing something and we need all three?

I added the strict mode to prevent gradle from wrongly considering a pinned version (eg. 1.22) as more recent than a commit-hash version (eg. trunk-1ed207c03d2242b6fc3d74f9e388e9163cbc82a6). Alternative solution is to publish a new version of Utils.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for updating this. It should have been updated while doing the composite build but at the time I was also worried about missing something 😅

}

implementation 'com.github.Tenor-Inc:tenor-android-core:0.5.1'
implementation (name:'tenor-android-core-jetified', ext:'aar') // Jetified Tenor Gif library
Copy link
Copy Markdown
Contributor Author

@malinajirka malinajirka Dec 9, 2021

Choose a reason for hiding this comment

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

Tenor hasn't been updated since 2018 - it still uses the support library. I had to jetify the AAR locally and include it in the repo.

// Debug
debugImplementation 'com.facebook.stetho:stetho:1.5.1'
debugImplementation 'com.facebook.stetho:stetho-okhttp3:1.5.1'
debugImplementation 'com.facebook.stetho:stetho:1.6.0'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The only change in the new version is that it uses AndroidX - https://github.com/facebook/stetho/releases


private void initializePlayer() {
mPlayer = ExoPlayerFactory.newSimpleInstance(requireContext());
mPlayer = (new SimpleExoPlayer.Builder(requireContext())).build();
Copy link
Copy Markdown
Contributor Author

@malinajirka malinajirka Dec 9, 2021

Choose a reason for hiding this comment

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

This was the only breaking change in the new version of ExoPlayer.

apply plugin: 'com.android.library'

repositories {
maven {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adds S3 repository as that's where we store Utils artifacts now.

@@ -1,5 +1,5 @@
buildscript {
ext.aztecVersion = 'v1.5.1'
ext.aztecVersion = '946-74d200b819d457d42d405a8858665bd7d471acc9'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updates aztec to a version which is using the new version of utils which is not dependant on the support library.

api ("$gradle.ext.aztecAndroidWordPressCommentsPath:$aztecVersion")

implementation ("$gradle.ext.wputilsBinaryPath") {
version {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as a few comments above: Uses strictly to workaround gradle wrong conflict resolution for pinned vs commithash versions.

@malinajirka malinajirka added this to the 19.0 milestone Dec 9, 2021
@0nko
Copy link
Copy Markdown
Contributor

0nko commented Mar 10, 2022

@malinajirka @ParaskP7 I've created a MediaPicker PR that removes the Jetifier as per your suggestions.

@ParaskP7
Copy link
Copy Markdown
Contributor

👋 @malinajirka !

Nice find! It's very weird that the "byebye jetifier" tool didn't warn me about it 🤔 . Any ideas why that might be?

😅 I actually used bye bye jetifier and it was the one warning me about it... 😅

I am not sure how you applies the plugin, but I applied it on all modules and gradle configuration files, maybe you just applied it on one module or the top module, not sure how you configured and tested it. 🤔

@ParaskP7
Copy link
Copy Markdown
Contributor

👋 @0nko !

@malinajirka @ParaskP7 I've created a wordpress-mobile/WordPress-MediaPicker-Android#56 that removes the Jetifier as per your suggestions.

This is awesome, thank you! 🙇

@AliSoftware AliSoftware modified the milestones: 19.5, 19.6 Mar 21, 2022
@malinajirka
Copy link
Copy Markdown
Contributor Author

org.wordpress:fluxc:trunk-7035f6772f4e0cb4980da0556a5762ea7904eb24 -> org.wordpress:wellsql:1.7.0 -> com.android.support:support-annotations:28.0.0

Fixed by excluding support-annotations from fluxc -> 09ff700. AFAIU it's used only as a hint for static analysis tools -> shouldn't have effect at runtime.

+---org.mockito:mockito-android:3.3.3
Issues found: * org/mockito/android/internal/creation/AndroidTempFileLocator.class -> android.support.test.InstrumentationRegistry

IMO a false positive -> looking at the code of AndroidTempFileLocator it contains a reference to the support library only in a string -> it looks at android.support.test. and if doesn't find it it looks at androidx.test..

+---androidx.test.espresso:espresso-contrib:3.1.0
+---com.google.android.apps.common.testing.accessibility.framework:accessibility-test-framework:2.0
+---androidx.test.espresso:espresso-accessibility:3.3.0-alpha05
+---com.google.android.apps.common.testing.accessibility.framework:accessibility-test-framework:2.0

Fixed by upgrading androidxTestVersion and espressoVersion - in a separate PR so we can easily check that it doesn't have any unintended effects.
#16170

Graphs to this dependency:
+---org.robolectric:shadows-multidex:4.4
Issues found:

  • org/robolectric/shadows/multidex/Shadows.class -> android/support/multidex/MultiDex
  • org/robolectric/shadows/multidex/Shadows.class -> android.support.multidex.MultiDex
  • org/robolectric/shadows/multidex/ShadowMultiDex.class -> android/support/multidex/MultiDex

Fixed by upgrading robolectric from 4.4 -> 4.7.3 and removing unused shadows-multidex - in a separate PR so we can easily check it doesn't have any unintended sideffects.
#16170

I've updated the checklist in the description.

@malinajirka malinajirka marked this pull request as ready for review March 25, 2022 07:59
@ParaskP7
Copy link
Copy Markdown
Contributor

👋 @malinajirka !

Thank you for your continuous efforts on this PR and all these improvements, we are so close into merging it! 🙇

Fixed by excluding support-annotations from fluxc -> 09ff700. AFAIU it's used only as a hint for static analysis tools -> shouldn't have effect at runtime.

💯

IMO a false positive -> looking at the code of AndroidTempFileLocator it contains a reference to the support library only in a string -> it looks at android.support.test. and if doesn't find it it looks at androidx.test..

👍 Also, since this is a test related artifact, I am much more relaxed... 😅

Fixed by upgrading androidxTestVersion and espressoVersion - in a separate PR so we can easily check that it doesn't have any unintended effects.
#16170

🥇 Let's work together in merging this dependant PR! 🚀

Fixed by upgrading robolectric from 4.4 -> 4.7.3 and removing unused shadows-multidex - in a separate PR so we can easily check it doesn't have any unintended sideffects.
#16170

🙏

I've updated the checklist in the description.

🙇

@AliSoftware
Copy link
Copy Markdown
Contributor

FYI: I'm doing the 19.6 code freeze today, so I'm moving the milestone of this PR to 19.7

@AliSoftware AliSoftware modified the milestones: 19.6, 19.7 Apr 4, 2022
@ParaskP7 ParaskP7 modified the milestones: 19.7, 19.8 Apr 12, 2022
@ParaskP7 ParaskP7 self-requested a review April 12, 2022 12:31
Copy link
Copy Markdown
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @malinajirka !

Once more, thank you so much for creating this PR, working on it and progressing it till the very end. I reviewed it once more and tested it as per the description. I also applied the bye-bye-jetifier plugin one more, with its new version, the 1.2.2 and verified that we are now jetifier free (except of course org.mockito:mockito-android:3.3.3 which you anyway explained why here).

As such, it is a 🟢 light from my side and we can progress with the remaining merge instructions (step 9 and rest).

PS: I also update the milestone to 19.8 as I would recommend we merge this next Tuesday (19/04/22), right after the 19.7 is cut, just so we get a full 2 weeks of engineering testing before the next beta. Wdyt?

@malinajirka
Copy link
Copy Markdown
Contributor Author

Awesome, thank you so much for all the work you've put into this! 🙇

I also update the milestone to 19.8 as I would recommend we merge this next Tuesday (19/04/22)

I was planning on suggesting that too 🍻!

I'll proceed with the remaining steps and merge on Tuesday/Wednesday.

@wpmobilebot
Copy link
Copy Markdown
Contributor

Found 1 violations:

The PR caused the following dependency changes:

-+--- org.wordpress:fluxc:{strictly trunk-d49cf4e9d716af757b1c7044a0f786d3015d491e} -> trunk-d49cf4e9d716af757b1c7044a0f786d3015d491e
-|    \--- org.wordpress:wellsql:1.7.0
-|         \--- com.android.support:support-annotations:28.0.0
-+--- org.wordpress:utils:2.5.0
++--- org.wordpress:utils:{strictly 2.5.0} -> 2.5.0
 +--- project :libs:editor:WordPressEditor
-|    +--- org.wordpress:aztec:trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762 -> v1.5.4
+|    +--- org.wordpress:aztec:{strictly trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762} -> trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762
-|    +--- org.wordpress.aztec:wordpress-shortcodes:trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762 -> v1.5.4
-|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.4.20 -> 1.6.10 (*)
-|    |    +--- org.wordpress:aztec:v1.5.4 (*)
-|    |    \--- androidx.appcompat:appcompat:1.0.0 -> 1.3.1 (*)
+|    +--- org.wordpress.aztec:wordpress-shortcodes:{strictly trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762} -> trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762
+|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.4.20 -> 1.6.10 (*)
+|    |    +--- org.wordpress:aztec:trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762 (*)
+|    |    \--- androidx.appcompat:appcompat:1.0.0 -> 1.3.1 (*)
-|    +--- org.wordpress.aztec:wordpress-comments:trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762 -> v1.5.4
-|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.4.20 -> 1.6.10 (*)
-|    |    +--- org.wordpress:aztec:v1.5.4 (*)
-|    |    +--- androidx.legacy:legacy-support-v4:1.0.0 (*)
-|    |    \--- com.google.android.material:material:1.0.0 -> 1.6.0-alpha01 (*)
+|    +--- org.wordpress.aztec:wordpress-comments:{strictly trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762} -> trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762
+|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.4.20 -> 1.6.10 (*)
+|    |    +--- org.wordpress:aztec:trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762 (*)
+|    |    +--- androidx.legacy:legacy-support-v4:1.0.0 (*)
+|    |    \--- com.google.android.material:material:1.0.0 -> 1.6.0-alpha01 (*)
 |    \--- org.wordpress-mobile.gutenberg-mobile:react-native-gutenberg-bridge:v1.74.0
 |         \--- org.wordpress-mobile.gutenberg-mobile:react-native-aztec:v1.74.0
-|              +--- org.wordpress:aztec:v1.5.4 (*)
+|              +--- org.wordpress:aztec:v1.5.4 -> trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762 (*)
-|              +--- org.wordpress.aztec:wordpress-shortcodes:v1.5.4 (*)
+|              +--- org.wordpress.aztec:wordpress-shortcodes:v1.5.4 -> trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762 (*)
-|              +--- org.wordpress.aztec:wordpress-comments:v1.5.4 (*)
+|              +--- org.wordpress.aztec:wordpress-comments:v1.5.4 -> trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762 (*)
 |              \--- org.wordpress.aztec:glide-loader:v1.5.4
-|                   \--- org.wordpress:aztec:v1.5.4 (*)
+|                   \--- org.wordpress:aztec:v1.5.4 -> trunk-f3fc2a67ba38feb4adb4a93703587627aa29c762 (*)
++--- :tenor-android-core-jetified
-+--- com.github.Tenor-Inc:tenor-android-core:0.5.1
-|    +--- com.android.support:support-v13:26.1.0
-|    |    +--- com.android.support:support-annotations:26.1.0 -> 28.0.0
-|    |    \--- com.android.support:support-v4:26.1.0
-|    |         +--- com.android.support:support-compat:26.1.0
-|    |         |    +--- com.android.support:support-annotations:26.1.0 -> 28.0.0
-|    |         |    \--- android.arch.lifecycle:runtime:1.0.0
-|    |         |         +--- android.arch.lifecycle:common:1.0.0
-|    |         |         \--- android.arch.core:common:1.0.0
-|    |         +--- com.android.support:support-media-compat:26.1.0
-|    |         |    +--- com.android.support:support-annotations:26.1.0 -> 28.0.0
-|    |         |    \--- com.android.support:support-compat:26.1.0 (*)
-|    |         +--- com.android.support:support-core-utils:26.1.0
-|    |         |    +--- com.android.support:support-annotations:26.1.0 -> 28.0.0
-|    |         |    \--- com.android.support:support-compat:26.1.0 (*)
-|    |         +--- com.android.support:support-core-ui:26.1.0
-|    |         |    +--- com.android.support:support-annotations:26.1.0 -> 28.0.0
-|    |         |    \--- com.android.support:support-compat:26.1.0 (*)
-|    |         \--- com.android.support:support-fragment:26.1.0
-|    |              +--- com.android.support:support-compat:26.1.0 (*)
-|    |              +--- com.android.support:support-core-ui:26.1.0 (*)
-|    |              \--- com.android.support:support-core-utils:26.1.0 (*)
-|    +--- com.android.support:support-annotations:26.1.0 -> 28.0.0
-|    +--- com.android.support:recyclerview-v7:26.1.0
-|    |    +--- com.android.support:support-annotations:26.1.0 -> 28.0.0
-|    |    +--- com.android.support:support-compat:26.1.0 (*)
-|    |    \--- com.android.support:support-core-ui:26.1.0 (*)
-|    +--- com.squareup.retrofit2:converter-gson:2.3.0 (*)
-|    \--- com.github.bumptech.glide:glide:3.8.0 -> 4.11.0 (*)
-\--- com.google.android.exoplayer:exoplayer:2.9.3 -> 2.13.3 (*)
+\--- com.google.android.exoplayer:exoplayer:2.13.3 (*)

Please review and act accordingly

@malinajirka malinajirka merged commit efb2440 into trunk Apr 19, 2022
@malinajirka malinajirka deleted the drop-jetifier-2 branch April 19, 2022 05:17
ParaskP7 added a commit to wordpress-mobile/WordPress-FluxC-Android that referenced this pull request Jul 4, 2023
This is long overdue and shouldn't cause any issues to WPAndroid and/or
WCAndroid whatsoever. For more info see WPAndroid PR below, and more
specifically the discussion about 'FluxC' and the below final comment
on that:

Drop jetifier #15675:
- wordpress-mobile/WordPress-Android#15675
FluxC was never part of the plan:
- https://github.com/wordpress-mobile/WordPress-Android/pull/
15675#issuecomment-1057840724

------------------------------------------------------------------------

Also, as part of this change, this 'android.jetifier.ignorelist' Gradle
property got removed as well. This Gradle property was added to fix
Robolectric tests in CI (df56fb1), then
updated to newer equivalent (232ce17),
but I am not sure if this is still needed, especially now that Jetifier
is disabled.
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.

8 participants