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

Update Fluxc and Login Libraries to Latest Trunk Version #17056

Merged
merged 2 commits into from
Aug 22, 2022

Conversation

ParaskP7
Copy link
Contributor

Relates To:

This PR is about updating both, the FluxC and Login libraries, to their corresponding latest trunk version. This is mainly done because of a crash that was identified on the WCAndroid site due to the RefreshSitesXMLRPCPayload data class becoming immutable as part of a FluxC update. However, the Login library was pointing to a previous version of FluxC, which didn't have the same change, thus causing that specific flow which was utilizing the previous version of RefreshSitesXMLRPCPayload to crash.

This update is done in order to make sure that both, this WPAndroid client and its Login library dependency, are pointing to that FluxC version where everything works as expected, without crashes or weird behaviors.


👋 @zwarm , I shamelessly added you as an extra reviewer. Can you help @AliSoftware with the smoke testing part, just to verify that everything works as expected before he starts with the 20.6 code freeze on Monday? 🙇


To test:

  • Verify that all the CI checks are successful.
  • Smoke test the apps, both WordPress and Jetpack, and verify everything works as expected (like Login functionality, My Site/Reader/Notification tabs, Stats/Media/Posts/Pages/Comments/Me/Jetpack screens, create Story/Post/Page, etc).
  • Additionally, you can follow the testing instructions within this related WCAndroid PR PR, and connect with a self-hosted site on a Jetpack connection and verify that the app isn't crashing.

Regression Notes

  1. Potential unintended areas of impact

Since this FluxC version comes with lots of small changes, like models becoming immutable (var -> val, etc), it might be the case that either the app is not complilying (can be easily verified by CI) or the app might be crashing on specific scenarios (thus needing manual smoke testing) due to transitive dependencies on various libraries that point to a different version of FluxC (like the Login library).

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

See To test section above.

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

N/A

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.

This 'Login' hash update 'Login' to that latest 'trunk' version where
its 'FluxC' hash updates 'FluxC' to that latest 'trunk' version where
the 'RefreshSitesXMLRPCPayload' data class becomes immutable.

For more info see: https://github.com/wordpress-mobile/
WordPress-Login-Flow-Android/pull/92

This, in coordination to the subsequent 'FluxC' update commit that will
follow, is done in order to make sure that both, this WPAndroid client
and its 'Login' library dependency, are pointing to that 'FluxC' version
where everything works as expected, without crashes or weird behaviors.
This 'FluxC' hash updates 'FluxC' to that latest 'trunk' version where
the 'RefreshSitesXMLRPCPayload' data class becomes immutable. The
'Login' library and its 'FluxC' dependency is using this model and as
such requires this change.

For more info see: https://github.com/wordpress-mobile/
WordPress-Login-Flow-Android/pull/92

This, in coordination to the previous 'Login' update commit that
followed, is done in order to make sure that both, this WPAndroid client
and its 'Login' library dependency, are pointing to that 'FluxC' version
where everything works as expected, without crashes or weird behaviors.
@ParaskP7 ParaskP7 added this to the 20.6 milestone Aug 19, 2022
@ParaskP7 ParaskP7 self-assigned this Aug 19, 2022
@wpmobilebot
Copy link
Contributor

Found 1 violations:

The PR caused the following dependency changes:

-+--- org.wordpress:fluxc:{strictly 1.49.0} -> 1.49.0
-|    +--- org.wordpress:wellsql:1.7.0
-|    |    \--- org.wordpress.wellsql:wellsql-annotations:1.7.0
-|    +--- org.wordpress.fluxc:fluxc-annotations:1.49.0
-|    +--- org.greenrobot:eventbus:3.3.1 (*)
-|    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.9.2 (*)
-|    +--- com.android.volley:volley:1.1.1 -> 1.2.0
-|    +--- androidx.paging:paging-runtime:2.1.2
-|    |    +--- androidx.paging:paging-common:2.1.2
-|    |    |    +--- androidx.annotation:annotation:1.0.0 -> 1.2.0
-|    |    |    \--- androidx.arch.core:core-common:2.0.0 -> 2.1.0 (*)
-|    |    +--- androidx.arch.core:core-runtime:2.0.0 -> 2.1.0 (*)
-|    |    +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.4.1 (*)
-|    |    +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.4.1 (*)
-|    |    \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
-|    +--- com.goterl:lazysodium-android:5.0.2
-|    +--- net.java.dev.jna:jna:5.5.0
-|    +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 (*)
-|    +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.10 (*)
-|    +--- androidx.appcompat:appcompat:1.0.2 -> 1.3.1 (*)
-|    +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
-|    +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.2.0 (*)
-|    +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0 -> 4.9.2
-|    |    +--- com.squareup.okhttp3:okhttp:4.9.2 (*)
-|    |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.6.10 (*)
-|    +--- com.google.code.gson:gson:2.8.5
-|    +--- org.apache.commons:commons-text:1.1 (*)
-|    +--- androidx.room:room-runtime:2.4.2 (*)
-|    +--- androidx.room:room-ktx:2.4.2
-|    |    +--- androidx.room:room-common:2.4.2 (*)
-|    |    +--- androidx.room:room-runtime:2.4.2 (*)
-|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 (*)
-|    |    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 (*)
-|    +--- com.google.dagger:dagger:2.42 (*)
-|    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.5.2 (*)
-|    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.5.2 (*)
++--- org.wordpress:fluxc:{strictly trunk-eb1d39ad095c01367af03c4259ab93cda14c675e} -> trunk-eb1d39ad095c01367af03c4259ab93cda14c675e
+|    +--- org.wordpress:wellsql:1.7.0
+|    |    \--- org.wordpress.wellsql:wellsql-annotations:1.7.0
+|    +--- org.wordpress.fluxc:fluxc-annotations:trunk-eb1d39ad095c01367af03c4259ab93cda14c675e
+|    +--- org.greenrobot:eventbus:3.3.1 (*)
+|    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.9.2 (*)
+|    +--- com.android.volley:volley:1.1.1 -> 1.2.0
+|    +--- androidx.paging:paging-runtime:2.1.2
+|    |    +--- androidx.paging:paging-common:2.1.2
+|    |    |    +--- androidx.annotation:annotation:1.0.0 -> 1.2.0
+|    |    |    \--- androidx.arch.core:core-common:2.0.0 -> 2.1.0 (*)
+|    |    +--- androidx.arch.core:core-runtime:2.0.0 -> 2.1.0 (*)
+|    |    +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.4.1 (*)
+|    |    +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.4.1 (*)
+|    |    \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
+|    +--- com.goterl:lazysodium-android:5.0.2
+|    +--- net.java.dev.jna:jna:5.5.0
+|    +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 (*)
+|    +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.10 (*)
+|    +--- androidx.appcompat:appcompat:1.0.2 -> 1.3.1 (*)
+|    +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
+|    +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.2.0 (*)
+|    +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0 -> 4.9.2
+|    |    +--- com.squareup.okhttp3:okhttp:4.9.2 (*)
+|    |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.6.10 (*)
+|    +--- com.google.code.gson:gson:2.8.5
+|    +--- org.apache.commons:commons-text:1.1 (*)
+|    +--- androidx.room:room-runtime:2.4.2 (*)
+|    +--- androidx.room:room-ktx:2.4.2
+|    |    +--- androidx.room:room-common:2.4.2 (*)
+|    |    +--- androidx.room:room-runtime:2.4.2 (*)
+|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 (*)
+|    |    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 (*)
+|    +--- com.google.dagger:dagger:2.42 (*)
+|    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.5.2 (*)
+|    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.5.2 (*)
-\--- org.wordpress:login:trunk-657bed28fa735780e3ffcc214503a9e5d1286d6c
-     +--- com.google.android.gms:play-services-auth:18.1.0 (*)
-     +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 (*)
-     +--- androidx.appcompat:appcompat:1.0.2 -> 1.3.1 (*)
-     +--- androidx.vectordrawable:vectordrawable-animated:1.1.0 (*)
-     +--- androidx.media:media:1.2.1 (*)
-     +--- androidx.legacy:legacy-support-v13:1.0.0
-     |    \--- androidx.legacy:legacy-support-v4:1.0.0 (*)
-     +--- androidx.gridlayout:gridlayout:1.0.0 (*)
-     +--- androidx.constraintlayout:constraintlayout:2.0.4 (*)
-     +--- com.google.android.material:material:1.2.1 -> 1.6.0-alpha01 (*)
-     +--- androidx.core:core-ktx:1.3.2 -> 1.7.0 (*)
-     +--- org.wordpress:fluxc:1.49.0 (*)
-     +--- com.google.dagger:dagger:2.42 (*)
-     \--- com.google.dagger:dagger-android-support:2.42 (*)
+\--- org.wordpress:login:trunk-e11535105725e02dd1e8aa8065314ceb67079278
+     +--- com.google.android.gms:play-services-auth:18.1.0 (*)
+     +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 (*)
+     +--- androidx.appcompat:appcompat:1.0.2 -> 1.3.1 (*)
+     +--- androidx.vectordrawable:vectordrawable-animated:1.1.0 (*)
+     +--- androidx.media:media:1.2.1 (*)
+     +--- androidx.legacy:legacy-support-v13:1.0.0
+     |    \--- androidx.legacy:legacy-support-v4:1.0.0 (*)
+     +--- androidx.gridlayout:gridlayout:1.0.0 (*)
+     +--- androidx.constraintlayout:constraintlayout:2.0.4 (*)
+     +--- com.google.android.material:material:1.2.1 -> 1.6.0-alpha01 (*)
+     +--- androidx.core:core-ktx:1.3.2 -> 1.7.0 (*)
+     +--- org.wordpress:fluxc:trunk-e94126bf9eb942d262768f8851cf2c4980e249fc -> trunk-eb1d39ad095c01367af03c4259ab93cda14c675e (*)
+     +--- com.google.dagger:dagger:2.42 (*)
+     \--- com.google.dagger:dagger-android-support:2.42 (*)

Please review and act accordingly

@wpmobilebot
Copy link
Contributor

You can test the Jetpack changes on this Pull Request by downloading an installable build (jetpack-installable-build-pr17056-0f2d043.apk), or scanning this QR code:

@wpmobilebot
Copy link
Contributor

You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr17056-0f2d043.apk), or scanning this QR code:

@@ -9,7 +9,7 @@ plugins {

ext {
wordPressUtilsVersion = '2.7.0'
wordPressLoginVersion = 'trunk-657bed28fa735780e3ffcc214503a9e5d1286d6c'
wordPressLoginVersion = 'trunk-e11535105725e02dd1e8aa8065314ceb67079278'
Copy link
Contributor

Choose a reason for hiding this comment

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

This matches the sha1 of current Login's trunk, so 👍

But that Login commit… points to fluxc:trunk-e94126bf9eb942d262768f8851cf2c4980e249fc in its own build.gradle, as opposed to pointing to FluxC's latest commit in trunk aka trunk-eb1d39ad095c01367af03c4259ab93cda14c675e aka what's also used as fluxCVersion in this WPAndroid build.gradle on line 24. And FluxC's e94126b (which Login points to) is 3 commits behind eb1d39a.

While it's not surprising to see Login's pointer to fluxc be behind the FluxC we points to in the clients, especially because FluxC is updated way more often than the Login lib is, I'm a bit surprised by this given that Login and FluxC were just synchronzed a couple of hours ago — in fact, that is the whole points of this update of both Login and FluxC being done in parallel here — and I wouldn't expect FluxC to already have moved ahead much?

So I'm wondering if this is expected to have Login not include those 3 recent commits from FluxC while this was all about synchronizing and updating the version of FluxC that both were pointing to to match 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok after checking with @ParaskP7 in Slack, this is in fact expected. And the 3 FluxC commits that Login does not include are not relevant to the Login library anyway.

While it's not surprising to see Login's pointer to fluxc be behind the FluxC we points to in the clients, especially because FluxC is updated way more often than the Login lib is, I'm a bit surprised by this given that Login and FluxC were just synchronzed a couple of hours ago […] and I wouldn't expect FluxC to already have moved ahead much?

So in practice this is just because FluxC already shipped a couple of recent PRs after this synchronization in Login lib happened earlier today. So… FluxC already moving that much ahead already after all 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👋 @AliSoftware !

Short answer, yes, this is expected and thank you for asking! 🥇

  • This morning we fixed an issue with the Login PR and then update WCAndroid to point to that with a WCAndroid PR.
  • Then, later on today, I merged 3 more PRs on FluxC (here, here and here) and completed the project I was working for this sprint.
  • This problem that this PR is trying to fix was on a previous FluxC commit, even before there extra 3 PRs, which got already merged 10 days ago on FluxC (via this PR and this commit).
  • Thus these 3 PRs that were merged afterwards weren’t going to help with this Login problem anyway, they are just nice to have on top of what Login's FluxC is pointing to.

Just that makes sense to you? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes total sense now, thanks for explaining in such details 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thank you for taking the time to investigate all that! 🥇

@AliSoftware
Copy link
Contributor

The diff looks good (once my question above wrt to the sha1 was clarified and confirmed that all is expected).

I probably won't have time to smoke-test the app with those changes today tho, as I'm already late to do the final build to submit 20.5 in my day.
So I'll let @zwarm do the smoke-testing, and approve and land the PR if everything looks good to her

Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

Smoke tested both WordPress and Jetpack this morning (as I'll need this PR to land before I can do the code freeze) and didn't find anything unusual, apps worked as expected.

@AliSoftware AliSoftware merged commit 435db3c into trunk Aug 22, 2022
@AliSoftware AliSoftware deleted the update/fluxc-and-login-to-latest-trunk branch August 22, 2022 09:55
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

3 participants