peril-wordpress-mobile[bot]
commented
2 years ago You can trigger optional UI/connected tests for these changes by visiting CircleCI here.
Closed malinajirka closed 2 years ago
peril-wordpress-mobile[bot]
commented
2 years ago You can trigger optional UI/connected tests for these changes by visiting CircleCI here.
peril-wordpress-mobile[bot]
commented
2 years ago You can test the changes on this Pull Request by downloading the APKs:
malinajirka
commented
2 years ago Thanks for looking into this @oguzkocer!
I started looking into this, even reviewed and merged stories PR but I see that there are a few other merge instructions to go through before this PR is ready for review/testing...
I believe this PR is already ready for review/testing. It is using all the final dependencies - just not the final hashes. AFAIK it's a regular process that we test against open-PRs and merge all of the PRs in a cascade when all of them are approved. Otherwise, if we merge the PRs in the sub-repos and this PR gets blocked we might end up in an untested state. Wdyt?
If you were referring to step Merge GB PR which updates Aztec Version - it doesn't exist yet I believe that step is technically optional - WordPress Android will use the final Aztec version either case, so you'd be testing it with the correct version. I created that step just to keep the versions consistent across repos.
I'll try, with the help of gutenberg folks, to approve the gutenberg PRs so it's as close to the final version as possible. Marking it as a draft as you suggested until they are approved.
malinajirka
commented
2 years ago Update on this PR: I've talked to @hypest and we discussed that the timing of this PR isn't ideal. We are in the middle of migrating react to a newer version and a lot of the folks are AFK, so merging the PRs in gutenberg repo is a bit more tricky. This PR is not urgent so we decided to put it on hold until the react native bump is tested and merged (sometime in January/February).
oguzkocer
commented
2 years ago I believe this PR is already ready for review/testing. It is using all the final dependencies - just not the final hashes. AFAIK it's a regular process that we test against open-PRs and merge all of the PRs in a cascade when all of them are approved. Otherwise, if we merge the PRs in the sub-repos and this PR gets blocked we might end up in an untested state. Wdyt?
@malinajirka How would we end up in an untested state?
If you were referring to step Merge GB PR which updates Aztec Version - it doesn't exist yet I believe that step is technically optional
Yes and no. This was a major factor, but I wanted dependencies to be reviewed/tested first as any issues in those would propagate to this PR and would require a second set of testing. It's also not marked as optional, so I interpreted as a requirement, more on that below.
If you were referring to step Merge GB PR which updates Aztec Version - it doesn't exist yet I believe that step is technically optional - WordPress Android will use the final Aztec version either case, so you'd be testing it with the correct version. I created that step just to keep the versions consistent across repos.
gutenberg-mobile is the main consumer of the Aztec library, so in my opinion it's more important that gutenberg-mobile is updated. This trinity between Aztec, gutenberg-mobile and WPAndroid is not ideal. I actually have an item about this in my todo list, to look into how we are using Aztec and whether we could get away with using the transitive dependency we get from gutenberg-mobile. Either way we go, we are actually leaving either gutenberg-mobile or WPAndroid in an untested state, unless we methodologically update each one together. So, in short, I don't fully agree that it's an optional step even though as you said we'd end up using the updated aztec version once this PR is merged.
Update on this PR: I've talked to @hypest and we discussed that the timing of this PR isn't ideal. We are in the middle of migrating react to a newer version and a lot of the folks are AFK, so merging the PRs in gutenberg repo is a bit more tricky. This PR is not urgent so we decided to put it on hold until the react native bump is tested and merged (sometime in January/February).
Sounds good. @hypest can we make sure we don't end up in a situation that this PR gets blocked forever. We had some instances like that and I'd like to help avoid them if I can. If it turns out the react-native version update is not happening as fast as you expect it to, can we reconsider going forward with these changes?
malinajirka
commented
2 years ago How would we end up in an untested state?
I was thinking about the following scenario:
1 are in WPAndroid even though this current PR is not reviewed/tested/merged yetIdeally, they'd notice in step 3 that the number of changes coming from gutenberg mobile to WPAndroid is bigger than what they created, but I'd be worried they might miss it. Even if they wouldn't miss it, it'd be quite tricky to merge their change into WPAndroid without including change from step 1.
would require a second set of testing.
I agree that any changes there would require second set of testing, but merging the PRs in the other repos before this PR is reviewed feels even more risky as per the above scenario☝️ .
This trinity between Aztec, gutenberg-mobile and WPAndroid is not ideal.
I agree, but I feel like that any dependency that is used in multiple modules have the same issue - eg. utils. Ideally we wouldn't be introducing any breaking changes in the libraries - including aztec. If we need to introduce a breaking change, it should be obvious from the version so whoever bumps the version knows that they might be introducing runtime errors and that they need to update even the dependencies.
Either way we go, we are actually leaving either gutenberg-mobile or WPAndroid in an untested state, unless we methodologically update each one together.
I might be missing something, but if we update Aztec in WPAndroid -> the new version of Aztec is used even in gutenberg editor in WPAndroid -> so if you test gutenberg editor in WPAndroid you are testing it with the new Aztec version, so I wouldn't call it untested. I agree that you might be introducing a runtime crash if Aztec contains a breaking change, but it can be caught during testing. The only benefit of updating Aztec version in gutenberg mobile is that the compiler catches breaking changes => that's why I called it "optional" but to be on the safe side I included it in the merge instructions.
Anyway, I think we both understand each other. It's just a matter of a preference. I personally prefer for these kind of tricky/bigger efforts to start testing ASAP even with the risk that a change in other repos will require retesting this PR.
oguzkocer
commented
2 years ago @malinajirka I think what we don't fully agree on is what type of change this is. If it was a feature change, then I mostly agree with everything you said. However, I'd consider this a tooling change and reviewing the PR is not just testing if the app is working, but verifying that it doesn't introduce new warnings/errors in the compilation process. With gutenberg-mobile having a different structure to every other Android library (it being react-native) I felt the correct approach was to do aztec -> gutenberg-mobile -> WPAndroid, however I am now doubting myself 😅
hypest
commented
2 years ago Update on this PR: I've talked to @hypest and we discussed that the timing of this PR isn't ideal. We are in the middle of migrating react to a newer version and a lot of the folks are AFK, so merging the PRs in gutenberg repo is a bit more tricky. This PR is not urgent so we decided to put it on hold until the react native bump is tested and merged (sometime in January/February).
Sounds good. @hypest can we make sure we don't end up in a situation that this PR gets blocked forever. We had some instances like that and I'd like to help avoid them if I can. If it turns out the react-native version update is not happening as fast as you expect it to, can we reconsider going forward with these changes?
The RN upgrade is now finished and with year-turn AFKs behind us, we can now resume working on this, no desire to block this. @mchowning , can you perhaps lend a hand on this one? Thanks!
mchowning
commented
2 years ago @mchowning , can you perhaps lend a hand on this one?
👍 I'll take a look next week
mchowning
commented
2 years ago
- Merge GB PR which updates Aztec Version - it doesn't exist yet
I believe we can cross this one off. There's been an AztecAndroid release (1.5.3] since the AztecAndroid PR dropping Jetifier was merged, and that new release of AztecAndroid is in use in both Gutenberg and WPAndroid.
mchowning
commented
2 years ago Pulled in the latest changes from trunk and bumped the exoplayer version to 2.13.3 along with bumping the gutenberg-mobile version (to the latest code, which is using that exoplayer version). In my testing this version of exoplayer is working fine both within Gutenberg and in WPAndroid's media picker.
Note that I also removed the gradle configuration that had us retrieving exoplayer from our jcenter-clone because we can retrieve 2.13.3 from google's maven repository.
I think this is (finally) good-to-go with respect to gutenberg-mobile.
ParaskP7
commented
2 years ago 👋 @mchowning @oguzkocer !
As I am digging deeper and deeper into how everything is connected and starting to understand the overall picture, I have a question related to this.
In addition to the below main modules, which indeed got updated with android.enableJetifier=false:
react-native-aztecreact-native-bridgereact-native-editorShouldn't all the individual note modules and their corresponding android.enableJetifier be updated as well (from true to false) to make sure that this work is fully complete?
PS: The 6 question mark (❓) you see on some of those node modules it because from the gutenberg repo itself, after building the node modules, not GitHub, I am seeing those to either:
gradle.properties file, but not a gradle wrapper to point to latest Gradle distribution. 2 of them:
react-native-svgreact-native-safe-area-contextreact-native-linear-gradientreact-native-gesture-handlerreact-native-screensreact-native-reanimatedAny idea why that happens? 🤔
oguzkocer
commented
2 years ago Shouldn't all the individual note modules and their corresponding android.enableJetifier be updated as well (from true to false) to make sure that this work is fully complete?
@ParaskP7 That sounds correct - or, we can remove the property all together since the default is false.
PS: The 6 question mark (❓) you see on some of those node modules it because from the gutenberg repo itself, after building the node modules, not GitHub, I am seeing those to either:
I am sorry, I don't understand the question. Is this about the case for gb-mobile development builds after you run npm install? If so, I don't have any particular knowledge on what gets pulled by npm, but Gradle wrapper is not really necessary because we don't build those as standalone projects from within node_modules. They'll all be built by the Gradle wrapper in the react-native-bridge project.
The more important thing for these forked projects is that the artifact we are publishing (by Jitpack) doesn't have the jetifier enabled as that's what WPAndroid will end up using.
Hope that provides some clarity. If you can clarify the above question, I can try and further help. If there is a specific situation that needs to be looked at, if you can provide a branch, I can investigate it as well.
ParaskP7
commented
2 years ago 👋 @oguzkocer !
@ParaskP7 That sounds correct - or, we can remove the property all together since the default is false.
👍
I am sorry, I don't understand the question. Is this about the case for gb-mobile development builds after you run npm install?
Yes, exactly.
If so, I don't have any particular knowledge on what gets pulled by npm, but Gradle wrapper is not really necessary because we don't build those as standalone projects from within node_modules. They'll all be built by the Gradle wrapper in the react-native-bridge project.
Got it. So, in order for me to understand that a bit better, we don't need the Gradle wrapper because we don't build those as standalone project from within the node_modules, but then wouldn't that mean that we also don't need their corresponding and associated gradle.properties configuration file and thus it can be also safely ignored? 🤔
Thus, and if my above assumption is correct, for the scope of this issue, the android.enableJetifier removal on the forks, so it defaults to false, is not necessary, is it?
The more important thing for these forked projects is that the artifact we are publishing (by Jitpack) doesn't have the jetifier enabled as that's what WPAndroid will end up using.
Ah, got it, so my above assumption is incorrect, we don't need the Gradle wrapper but we do need the gradle.properties. However, I do see some node modules not having the gradle.properties after running npm install, is that also expected?
I am so confused for seeing a node module like react-native-screens within the node_modules of the gutenberg project, after running the npm install, to not having any of those Gradle related files we are discussing, while other node modules, like the react-native-webview, to be having both.
Hope that provides some clarity. If you can clarify the above question, I can try and further help.
Yes, it provides some clarity, and thank you so much! 🙏 Apologies for me not being able to ask specific questions, or clarify those to the detail needed for you to answer at your best. I am still exploring, not seeing the whole picture, thus I am trying to learn as much as I can until it clicks within.
If there is a specific situation that needs to be looked at, if you can provide a branch, I can investigate it as well.
For all the above, the trunk branch is enough for you to see what I am seeing. You can build from the latest trunk and we can compare our note_modules output, for all the 13 forks that interest us.
For example, below is my output:
What we should expect to be seeing is a triplet of the gradle-wrapper.properties + gradle.properties + settings.gradle.kts files, but as you can see not all the 13 forks have that triplet after running npm install. For example:
react-native-linear-gradient, which only has one, the settings.gradle.kts file,react-native-svg has two, the gradle.properties + settings.gradle.kts files,react-native-video/android-exomplayer has all three, the gradle-wrapper.properties + gradle.properties + settings.gradle.kts files.react-native-gesture-handler or react-native-screens doesn't have any such files.I wonder if you get the same output. 🤔
oguzkocer
commented
2 years ago Thus, and if my above assumption is correct, for the scope of this issue, the android.enableJetifier removal on the forks, so it defaults to false, is not necessary, is it?
You already answered this in your next statement, but just for the sake of avoiding confusion by others, I'll repeat that this is incorrect because what we most care about is how the artifact is published. During that process the gradle.properties in the project will be utilized and how the project is handled by npm is not relevant.
However, I do see some node modules not having the gradle.properties after running npm install, is that also expected?
Hopefully my specific answers at the end of this comment will provide some clarity.
Yes, it provides some clarity, and thank you so much! 🙏 Apologies for me not being able to ask specific questions, or clarify those to the detail needed for you to answer at your best. I am still exploring, not seeing the whole picture, thus I am trying to learn as much as I can until it clicks within.
No apologies necessary. I am just clearly stating when I don't understand something so that there isn't further confusion. It's not meant as a feedback for your communication, so if it came off that way, I am sorry.
For all the above, the
trunkbranch is enough for you to see what I am seeing. You can build from the latesttrunkand we can compare our note_modules output, for all the 13 forks that interest us.
I am a bit confused as to what that screenshot is 😓 When you say trunk, I assume you mean trunk for WordPress-Android, but if that's the case how are you getting those files to show up in Android Studio unless you are using composite build? I think I am going to need some instructions to produce the result you're looking for. (that's unless my replies below doesn't already answer your question)
What we should expect to be seeing is a triplet of the gradle-wrapper.properties + gradle.properties + settings.gradle.kts files, but as you can see not all the 13 forks have that triplet after running npm install. For example:
I'll reply to this part assuming that you're referring to the node_modules folder.
See the
react-native-linear-gradient, which only has one, thesettings.gradle.ktsfile,
Because the other files are ignored through .npmignore within the android folder.
While the
react-native-svghas two, thegradle.properties+settings.gradle.ktsfiles,
Similarly, the Gradle wrapper files are ignored.
And in comparison, the
react-native-video/android-exoplayerhas all three, thegradle-wrapper.properties+gradle.properties+settings.gradle.ktsfiles.
Because android-exoplayer is part of the packaged files and there is no .npmignore within this directory.
While
react-native-gesture-handlerorreact-native-screensdoesn't have any such files.
In react-native-gesture-handler these files are not added to the package.
In react-native-screens these files are not added to the package.
I am hoping that the npm related answers can provide you some clarity, but I want to re-iterate that how these projects are handled within the react ecosystem shouldn't have any impact on our work about the jetifier. Admittedly this was interesting to learn about and may have implications on our other work, so I guess it all worked out somehow 😅
@ParaskP7 Let me know if you have any further questions!
ParaskP7
commented
2 years ago 👋 @oguzkocer !
You already answered this in your next statement, but just for the sake of avoiding confusion by others, I'll repeat that this is incorrect because what we most care about is how the artifact is published. During that process the gradle.properties in the project will be utilized and how the project is handled by npm is not relevant.
👍
Hopefully my specific answers at the end of this comment will provide some clarity.
💯
No apologies necessary. I am just clearly stating when I don't understand something so that there isn't further confusion. It's not meant as a feedback for your communication, so if it came off that way, I am sorry.
🤗 Nothing came off the wrong way, there is nothing to be sorry about, I didn't perceived any of that as feedback for my communication, it is just me letting you know that I know I am asking too many question and many of them are not too targeted or specific. 🤗
I am a bit confused as to what that screenshot is 😓 When you say trunk, I assume you mean trunk for WordPress-Android, but if that's the case how are you getting those files to show up in Android Studio unless you are using composite build? I think I am going to need some instructions to produce the result you're looking for. (that's unless my replies below doesn't already answer your question)
Ah, apologies for the confusing, I meant trunk for the gutenberg repo, not the WordPress-Android.
I'll reply to this part assuming that you're referring to the node_modules folder.
Yes, indeed. 🙏
Because the other files are ignored through .npmignore within the android folder.
Similarly, the Gradle wrapper files are ignored.
Because android-exoplayer is part of the packaged files and there is no .npmignore within this directory.
In react-native-gesture-handler these files are not added to the package.
In react-native-screens these files are not added to the package.
Oh, this is no interesting, I had no idea, thanks for sharing all that configuration, this now makes so much sense to me! 🌟
I am hoping that the npm related answers can provide you some clarity, ...
Your answers did add so much clarity Oguz, thank you so much! 🥇
... , but I want to re-iterate that how these projects are handled within the react ecosystem shouldn't have any impact on our work about the jetifier.
👍
Admittedly this was interesting to learn about and may have implications on our other work, so I guess it all worked out somehow 😅
😅 👍
@ParaskP7 Let me know if you have any further questions!
One last question and call to actions from my side. 😊
Since we do need to disable or remove the android.enableJetifier from all those 13 node module fork libraries, how do you recommend we approach that?
We can either:
Gradle & AGP upgrade work that is currently in progress, I can myself pick this up and run each node module fork library through bye-bye-jetifier to verify that we can do that, then disable or remove the flag.Your thoughts? 🤔
oguzkocer
commented
2 years ago Ah, apologies for the confusing, I meant trunk for the gutenberg repo, not the WordPress-Android.
The default branch for gb-mobile is develop, so that was one part of the confusion. I never considered gutenberg repo, because we shouldn't work with that project in isolation and prefer gb-mobile. Anyhow, glad the confusion is cleared up.
Since we do need to disable or remove the
android.enableJetifierfrom all those 13 node module fork libraries, how do you recommend we approach that?
I think it makes sense to combine this with your Gradle upgrade PRs. Since you'll visit each repo to upgrade AGP & Gradle again, it'll be easier to get it done together. Ideally it'd be a separate PR, but in practice there is very little difference and I think it's better to be practical in this case considering how many projects there are.
ParaskP7
commented
2 years ago The default branch for gb-mobile is develop, so that was one part of the confusion. I never considered gutenberg repo, because we shouldn't work with that project in isolation and prefer gb-mobile. Anyhow, glad the confusion is cleared up.
👍
I think it makes sense to combine this with your Gradle upgrade PRs. Since you'll visit each repo to upgrade AGP & Gradle again, it'll be easier to get it done together. Ideally it'd be a separate PR, but in practice there is very little difference and I think it's better to be practical in this case considering how many projects there are.
I agree on that, with the hope that me doing that wouldn't require any additional work or dependency updates on those node module forked libraries based on what bye-bye-jetifier will report to me. 🤞
@mchowning @malinajirka are you on-board with me doing that as part of the Gradle & AGP upgrade I am currently progressing with?
Question (❓): Does that mean that this PR will then depend on my Gradle & AGP upgrade to finish first, thus putting this on pause?
ParaskP7
commented
2 years ago 👋 @malinajirka !
UPDATE: I am also seeing that WordPress-Login-Flow-Android is also having Jetifier still enabled, thus as part of my Gradle & AGP upgrade work I'll be disabling Jetifier on that repo as well.
Let me know if I shouldn't be doing that because I am not understand something? 🤔
PS: I am also seeing that WordPress-FluxC-Android is also having Jetifier still enabled, is that expected because of the fact that WordPress-Android is not free of Jetifier just yet, while woocommerce-android is. Thus, both clients can enable/disable Jetifier as needed, but the default still remains true just because of WPAndroid? 🤔
malinajirka
commented
2 years ago My goal was to remove jetifier from the main repos as I expected that to have the biggest impact. My understanding is that if the main repos don't require jetifier than all the subrepos don't require it either - assuming that they don't need it for tests or other dependencies that are not part of the aar. Or am I missing something?
is that expected because of the fact that WordPress-Android is not free of Jetifier just yet
Nope, it's because FluxC or Login was never part of the plan :). As mentioned above, I think in those repos it's technically just about changing the flag since they should be already jetifier free (leaving tests and example apps aside) - otherwise WCAndroid wouldn't work without jetifier. But yeah I agree that for the clarify and tidiness it's a good call to update them too. Thanks!
ParaskP7
commented
2 years ago 👋 @malinajirka !
My goal was to remove jetifier from the main repos as I expected that to have the biggest impact. My understanding is that if the main repos don't require jetifier than all the subrepos don't require it either - assuming that they don't need it for tests or other dependencies that are not part of the aar. Or am I missing something?
👍 Nope, this is my understanding as well. However, when adding composite builds to the equation, I am not that certain anymore... 🤔
Nope, it's because FluxC or Login was never part of the plan :).
👍 about FluxC.
What confused me about the Login library was the fact that I saw you did the Jetifier change on Stories and Aztec repos, but then when I moved to the Login repo, I didn't see the flag being flipped like I was expected, thus I started thinking.
As mentioned above, I think in those repos it's technically just about changing the flag since they should be already jetifier free (leaving tests and example apps aside) - otherwise WCAndroid wouldn't work without jetifier. But yeah I agree that for the clarify and tidiness it's a good call to update them too. Thanks!
👍
PS: About FluxC, it seems that this repo will be the last one of them that needs to become Jetifier free (I did Login already). I know that this was never part of the plan, but I am sure you have an idea about the work required on that repo as well. Can you let me know, in case you have a feeling already, how easy is to flip the flag on FluxC? Maybe I'll flip that flag on FluxC as part of the Gradle & AGP work I am doing atm, just like I did for other repos already. 🤔
malinajirka
commented
2 years ago What confused me about the Login library was the fact that I saw you did the Jetifier change on Stories and Aztec repos, but then when I moved to the Login repo, I didn't see the flag being flipped like I was expected, thus I started thinking.
I used the byebyejetifier tool in WCAndroid/WPAndroid and they complained about dependencies which need to be jetified in Stories, Aztec and Utils => I removed those dependencies in those repos and WCAndroid/WPAndroid stopped complaining. So I think they should be okay even with composite builds, but I haven't verified that. When it comes to Login library, WCAndroid/WPAndroid were not complaining about any dependencies coming from the login library => I didn't flip the flag there. I still think that the effort will be close to zero, since AFAIU it's just about flipping the flag => no other changes are necessary afaik (assuming we don't use any dependencies which need to be jetified in integration tests which I believe we don't even have in Login library).
ParaskP7
commented
2 years ago I used the byebyejetifier tool in WCAndroid/WPAndroid and they complained about dependencies which need to be jetified in Stories, Aztec and Utils => I removed those dependencies in those repos and WCAndroid/WPAndroid stopped complaining. So I think they should be okay even with composite builds, but I haven't verified that.
👍
When it comes to Login library, WCAndroid/WPAndroid were not complaining about any dependencies coming from the login library => I didn't flip the flag there.
Ah, now I get it! 👍
I still think that the effort will be close to zero, since AFAIU it's just about flipping the flag => no other changes are necessary afaik (assuming we don't use any dependencies which need to be jetified in integration tests which I believe we don't even have in Login library).
👍 🤞
ParaskP7
commented
2 years ago 👋 @malinajirka !
UPDATE: Just an FYI that I am also seeing that WordPress-MediaPicker-Android is also having Jetifier still enabled. PS: This library is being utilized as composite build on WCAndroid.
I tried to quickly disable Jetifier as part of my Gradle & AGP upgrade on this repo, but it wasn't as straightforward and thus I stepped back. The main issues with Jetifier on this repo are:
2.0.0, which uses a previous version of EventBus that still has Jetifier. Updating that to its latest 2.4.0 version should fix this issue.0.5.1, which still has Jetifier. Creating and adding a Jetified AAR of this tenor library, just like it is done in this eab1df06017b0e648ee026e51e06e0c9ce3939d2 commit on this PR should fix this issue.Cc Shamelessly pinging @0nko here as well since I am seeing him being the last contributor to this repo.
malinajirka
commented
2 years ago Just an FYI that I am also seeing that WordPress-MediaPicker-Android is also having Jetifier still enabled. PS: This library is being utilized as composite build on WCAndroid.
Nice find! It's very weird that the "byebye jetifier" tool didn't warn me about it 🤔 . Any ideas why that might be?
0nko
commented
2 years ago @malinajirka @ParaskP7 I've created a MediaPicker PR that removes the Jetifier as per your suggestions.
ParaskP7
commented
2 years ago 👋 @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
commented
2 years ago 👋 @0nko !
@malinajirka @ParaskP7 I've created a https://github.com/wordpress-mobile/WordPress-MediaPicker-Android/pull/56 that removes the Jetifier as per your suggestions.
This is awesome, thank you! 🙇
malinajirka
commented
2 years ago 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 -> 09ff7009ec6d559590ab02816f12c907591429ca. 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.
https://github.com/wordpress-mobile/WordPress-Android/pull/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.
https://github.com/wordpress-mobile/WordPress-Android/pull/16170
I've updated the checklist in the description.
ParaskP7
commented
2 years ago 👋 @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 -> https://github.com/wordpress-mobile/WordPress-Android/commit/09ff7009ec6d559590ab02816f12c907591429ca. 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. https://github.com/wordpress-mobile/WordPress-Android/pull/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. https://github.com/wordpress-mobile/WordPress-Android/pull/16170
🙏
I've updated the checklist in the description.
🙇
AliSoftware
commented
2 years ago FYI: I'm doing the 19.6 code freeze today, so I'm moving the milestone of this PR to 19.7
malinajirka
commented
2 years ago 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
commented
2 years ago Found 1 violations:
-+--- 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
Merge Instructions:
android.enableJetifier=truetoandroid.enableJetifier=falsein 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
To test: Make sure to disable Jetifier in your local gradle.properties => Smoke test the app - anything you can think of.
Make sure the preview is displayed correctly and you can play the video
Add a gif
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on) The above "to test" section
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:
RELEASE-NOTES.txtif necessary.