[Dependency Analysis] Remove Unused Dependencies #83
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Release Notes: https://github.com/autonomousapps/ dependency-analysis-gradle-plugin/blob/main/CHANGELOG.md#version-1330
Removing the 'navigation-fragment-ktx' dependency wasn't enough by itself as the build was then failing. Adding the transient 'fragment-ktx' dependency was actually required in order to make the build successful again. ------------------------------------------------------------------------ This replacement was suggested by the dependency analysis report, see below: Advice for :mediapicker Unused dependencies which should be removed: implementation 'androidx.navigation:navigation-fragment-ktx:2.5.3' These transitive dependencies should be declared directly: ... api 'androidx.fragment:fragment:1.5.4' ... implementation 'androidx.fragment:fragment-ktx:1.5.4' ...
Removing the 'core-ktx' dependency wasn't enough by itself as the build was then failing. Adding the transient 'annotation' dependency was actually required in order to make the build successful again. ------------------------------------------------------------------------ This replacement was suggested by the dependency analysis report, see below: Advice for :mediapicker:domain Unused dependencies which should be removed: implementation 'androidx.core:core-ktx:1.12.0' These transitive dependencies should be declared directly: ... implementation 'androidx.annotation:annotation:1.6.0' ...
There are no used annotation processors for this module. ------------------------------------------------------------------------ This removal was suggested by the dependency analysis report, see below: Advice for :mediapicker:domain Unused plugins that can be removed: kotlin-kapt: this project has the kotlin-kapt (org.jetbrains.kotlin.kapt) plugin applied, but there are no used annotation processors.
This 'unused' related advise for the 'mediapicker.source-camera' module is incorrect because otherwise Lint complains that 'androidx.core.content.FileProvider' cannot be found within the 'AndroidManifest.xml' file on that module. As such this advice is instead suppressed and the 'onUnusedDependencies' exclusion configuration were used to achieve that. ------------------------------------------------------------------------ Advice for :mediapicker:source-camera Unused dependencies which should be removed: implementation 'androidx.core:core-ktx:1.12.0'
Removing the 'hilt-compiler' dependency wasn't enough by itself, the build wasn't failing, but also, wasn't working as expected. Adding the transient 'dagger-compiler' dependency was actually required in order to make the app work as expected. ------------------------------------------------------------------------ This replacement was suggested by the dependency analysis report, see below: Advice for :mediapicker:source-device These transitive dependencies should be declared directly: ... kapt 'com.google.dagger:dagger-compiler:2.46.1' Unused annotation processors that should be removed: kapt 'com.google.dagger:hilt-compiler:2.46.1'
This 'unused' related advise for the 'mediapicker.source-gif' module is incorrect because otherwise the build fails with an 'NoClassDefFoundError' exception on 'GsonConverterFactory' (see below). As such this advice is instead suppressed and the 'onUnusedDependencies' exclusion configuration were used to achieve that. ------------------------------------------------------------------------ java.lang.NoClassDefFoundError: Failed resolution of: Lretrofit2/converter/gson/GsonConverterFactory; ... Caused by: java.lang.ClassNotFoundException: Didn't find class "retrofit2.converter.gson.GsonConverterFactory" on path: DexPathList[...] ... ------------------------------------------------------------------------ Advice for :mediapicker:source-gif Unused dependencies which should be removed: implementation 'com.squareup.retrofit2:converter-gson:2.9.0'
Removing the 'hilt-compiler' dependency wasn't enough by itself, the build wasn't failing, but also, wasn't working as expected. Adding the transient 'dagger-compiler' dependency was actually required in order to make the app work as expected. ------------------------------------------------------------------------ This replacement was suggested by the dependency analysis report, see below: Advice for :mediapicker:source-wordpress These transitive dependencies should be declared directly: ... kapt 'com.google.dagger:dagger-compiler:2.46.1' Unused annotation processors that should be removed: kapt 'com.google.dagger:hilt-compiler:2.46.1'
This unused dependency got added to this project via this (78fdbd3) commit, which actually created this module with an initial (default) setup. However, this dependency never got to be used and as such can be safely removed. ------------------------------------------------------------------------ This removal was suggested by the dependency analysis report, see below: Advice for :sampleapp Unused dependencies which should be removed: implementation 'androidx.core:core-ktx:1.12.0' ...
This 'unused' related advise for the 'sampleapp' module is actually
correct and 'mediapicker:source-wordpress' can be removed as a
dependency since it is actually unused and not being utilized from
within the sample app.
However, I chose to keep it there and suppress the advise instead in
order to preserve the sample app default configuration, its purpose
being to test all those individual modules:
- mediapicker:source-device
- mediapicker:source-gif
- mediapicker:source-wordpress
- mediapicker:source-camera
As such this advice is instead suppressed and the 'onUnusedDependencies'
exclusion configuration were used to achieve that.
------------------------------------------------------------------------
Advice for :sampleapp
Unused dependencies which should be removed:
implementation project(':mediapicker:source-wordpress')
wzieba
approved these changes
Aug 14, 2024
| issues { | ||
| onUnusedDependencies { | ||
| // This dependency is actually needed because of 'androidx.core.content.FileProvider' on 'AndroidManifest.xml'. | ||
| exclude("androidx.core:core-ktx") |
There was a problem hiding this comment.
It's quite unexpected that this module has FileProvider in the manifest, although it doesn't really use it. I wonder - does it make sense to move decleration of FileProvider to manifest of mediapicker?
There was a problem hiding this comment.
Yea, maybe @wzieba , let me take a closer look on in next week and see if that's a better solution, thanks for taking a closer look yourself, much appreciated! 🙇
There was a problem hiding this comment.
👋 @wzieba !
So, I tried to move the FileProvider configuration from the mediapicker:source-camera module and into the mediapicker module, but then I started to get the below IllegalArgumentException exception when trying to open the CAMERA using the SampleApp.
java.lang.IllegalArgumentException: Couldn't find meta-data for provider with authority org.wordpress.android.sampleapp.provider
12:53:09.411 E FATAL EXCEPTION: main
Process: org.wordpress.android.sampleapp, PID: 15307
java.lang.IllegalArgumentException: Couldn't find meta-data for provider with authority org.wordpress.android.sampleapp.provider
at androidx.core.content.FileProvider.getFileProviderPathsMetaData(FileProvider.java:688)
at androidx.core.content.FileProvider.parsePathStrategy(FileProvider.java:719)
at androidx.core.content.FileProvider.getPathStrategy(FileProvider.java:669)
at androidx.core.content.FileProvider.getUriForFile(FileProvider.java:451)
at org.wordpress.android.mediapicker.MediaPickerUtils.createCaptureImageIntent(MediaPickerUtils.kt:96)
at org.wordpress.android.mediapicker.ui.MediaPickerFragment.onActionSelected(MediaPickerFragment.kt:293)
at org.wordpress.android.mediapicker.ui.MediaPickerFragment.access$onActionSelected(MediaPickerFragment.kt:75)
at org.wordpress.android.mediapicker.ui.MediaPickerFragment$onViewCreated$3$3.invoke(MediaPickerFragment.kt:259)
at org.wordpress.android.mediapicker.ui.MediaPickerFragment$onViewCreated$3$3.invoke(MediaPickerFragment.kt:231)
at org.wordpress.android.mediapicker.viewmodel.EventKt$observeEvent$1.invoke(Event.kt:36)
at org.wordpress.android.mediapicker.viewmodel.EventKt$observeEvent$1.invoke(Event.kt:36)
at org.wordpress.android.mediapicker.viewmodel.EventKt$sam$androidx_lifecycle_Observer$0.onChanged(Unknown Source:2)
at androidx.lifecycle.LiveData.considerNotify(LiveData.java:133)
at androidx.lifecycle.LiveData.dispatchingValue(LiveData.java:151)
at androidx.lifecycle.LiveData.setValue(LiveData.java:309)
at androidx.lifecycle.MutableLiveData.setValue(MutableLiveData.java:50)
at androidx.lifecycle.LiveData$1.run(LiveData.java:93)
at android.os.Handler.handleCallback(Handler.java:959)
at android.os.Handler.dispatchMessage(Handler.java:100)
at android.os.Looper.loopOnce(Looper.java:232)
at android.os.Looper.loop(Looper.java:317)
at android.app.ActivityThread.main(ActivityThread.java:8592)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:580)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:878)
I then timeboxed to try and quickly make it work but I couldn't figure this out and then didn't want to spend more time on it unnecessarily. 🤷
Maybe it is better to keep that configuration there after all, wdyt? 🤔
For more info see: MediaPickerUtils.createCaptureImageIntent(...)
There was a problem hiding this comment.
Have you maybe compared the merged manifests before and after your change? Is there any diff there? If you consider this not important or low priority, I'm also okay with merging as it is, hence the ✅!
There was a problem hiding this comment.
I actually didn't @wzieba , but I really didn't want to go that far to figure this out, I also didn't want to start playing with Manifest permission and moving them around too, I did try debugging to see whether and how the authority is changing but couldn't figure it out, also tried a custom FileProvider... 🤷
I am not sure about how important this is or its priority, but I do consider that out of scope of unused dependencies, especially since the fix doesn't seem to be straightforward and could mean applying more changing that initially expected.
As such, let's merge this as is for now, but I created this #84 issue for us to pick this up at some point in the future (if need be).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Project Thread: paaHJt-6YV-p2
Description
Based on the
unusedrelated advises generated by the Dependency Analysis Gradle plugin, this PR removes all unused dependencies from this project.FYI: As part of this change, the Dependency Analysis Gradle plugin got also updated to its latest 1.33.0 version.
Testing Steps
./gradlew buildHealthtask and verify that there is nounusedrelated advise remaining within both reports, JSON and txt included:build-health-report.json-> Search forunusedCountbuild-health-report.txt-> Search forunusedSampleapp, try out all available screens and functionality. Verify everything is working as expected.MediaPicker, or via composite builds, and see if it works as expected.