Skip to content

[Dependency Analysis] Remove Unused Dependencies#235

Merged
wzieba merged 14 commits intotrunkfrom
build/remove-unused-dependencies
Aug 21, 2024
Merged

[Dependency Analysis] Remove Unused Dependencies#235
wzieba merged 14 commits intotrunkfrom
build/remove-unused-dependencies

Conversation

@ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Aug 20, 2024

Project Thread: paaHJt-6YV-p2

Description

Based on the unused related 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

  1. Tooling:
    • Run the ./gradlew buildHealth task and verify that there is no unused related advise remaining within both reports, JSON and txt included:
      • build-health-report.json -> Search for unusedCount
      • build-health-report.txt -> Search for unused
    • Check the manually triggered scheduled build and verify that everything works as expected with this newer version of the Dependency Analysis Gradle plugin (1.33.0).
  2. Testing:
    • Smoke test the sample tracks app, try out all available screens and functionality. Verify everything is working as expected.
    • Also, if you want to be thorough about reviewing the changes, you could quickly smoke test, either the JP/WPAndroid and/or WCAndroid, with this version of Tracks, or via composite builds, and see if it works as expected.

Release Notes: https://github.com/autonomousapps/
dependency-analysis-gradle-plugin/blob/main/CHANGELOG.md#version-1330
This dependency was added as part of this
(9b00ec5) commit, which added the
'CrashLoggingOkHttpInterceptorProvider' class on the project. But then,
as part of this (24ae750) other commit,
this 'CrashLoggingOkHttpInterceptorProvider' class got moved into the
newly created 'crashlogging' module. As such, this dependency is no
longer needed on 'AutomatticTracks'.

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

This removal was suggested by the dependency analysis report, see below:

Advice for :AutomatticTracks
Unused dependencies which should be removed:
  implementation 'com.squareup.okhttp3:okhttp:4.9.0'
This 'benchmark' module is a special module that was automatically
generated from the Android Studio wizard
(828fb43) for basic track method
benchmarking purposes only (7a302f1).
Ignoring this Kotlin Stdlib advise instead of trying to act on this
advise seems reasonable, especially since there is no Kotlin code on the
main source-set whatsoever.

As such this advice is instead suppressed and the 'onUnusedDependencies'
exclusion configuration were used to achieve that.

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

Advice for :benchmark
Unused dependencies which should be removed:
  api 'org.jetbrains.kotlin:kotlin-stdlib:1.9.24'
Removing the 'sentry-android-okhttp' dependency wasn't enough by itself
as the build was then failing. Adding the transient 'sentry-okhttp'
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 :crashlogging
Unused dependencies which should be removed:
  implementation 'io.sentry:sentry-android-okhttp:7.14.0'
  ...

These transitive dependencies should be declared directly:
  ...
  implementation 'io.sentry:sentry-okhttp:7.14.0'
  ...
Removing the 'sentry-android' dependency wasn't enough by itself as the
build was then failing. Adding the transient 'sentry-android-core'
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 :crashlogging
Unused dependencies which should be removed:
  implementation 'io.sentry:sentry-android:7.14.0'

These transitive dependencies should be declared directly:
  ...
  implementation 'io.sentry:sentry-android-core:7.14.0'
  ...
This dependency is completely unused.

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

This removal was suggested by the dependency analysis report, see below:

Advice for :sampletracksapp
Unused dependencies which should be removed:
  implementation 'androidx.core:core-ktx:1.3.2'
  ...
This 'unused' related advise for the 'sampletracksapp' module is
actually correct and 'AutomatticTracks' can be removed as a dependency
since it is actually unused and not being utilized from within the
sample tracks 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:
- AutomatticTracks
- crashlogging

As such this advice is instead suppressed and the 'onUnusedDependencies'
exclusion configuration were used to achieve that.

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

Advice for :sampletracksapp
Unused dependencies which should be removed:
  implementation project(':AutomatticTracks')
Error Message: "Class referenced in the manifest,
com.automattic.tracks.benchmark.TrackingMethodsBenchmark$SampleActivity,
was not found in the project or the libraries"
Error Message: "Error: Library lint checks reference invalid APIs;
these checks will be skipped!

Lint found an issue registry (androidx.compose.runtime.lint.
RuntimeIssueRegistry)
which contains some references to invalid API:
org.jetbrains.kotlin.analysis.api.session.KtAnalysisSessionProvider:
org.jetbrains.kotlin.analysis.api.lifetime.KtLifetimeTokenFactory
getTokenFactory()
(Referenced from androidx/compose/runtime/lint/
AutoboxingStateCreationDetector.class)

Therefore, this lint check library is not included
in analysis. This affects the following lint checks:
AutoboxingStateValueProperty
AutoboxingStateCreation
CoroutineCreationDuringComposition
FlowOperatorInvokedInComposition
ComposableLambdaParameterNaming
ComposableLambdaParameterPosition
ComposableNaming
StateFlowValueCalledInComposition
CompositionLocalNaming
MutableCollectionMutableState
ProduceStateDoesNotAssignValue
RememberReturnType
OpaqueUnitKey
UnrememberedMutableState"
@ParaskP7 ParaskP7 marked this pull request as ready for review August 20, 2024 12:21
@ParaskP7 ParaskP7 requested a review from wzieba August 20, 2024 12:21
Comment on lines +66 to +73
dependencyAnalysis {
issues {
onUnusedDependencies {
// This dependency is not needed but kept to preserve the module's default configuration.
exclude("org.jetbrains.kotlin:kotlin-stdlib")
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I have similar concern as in other, similar scenario. If kotlin-stdlib is not required, then let's maybe instruct kotlin gradle plugin to... not include kotlin-stdlib.

It won't affect build in any way, as this is about adding a dependency to project's classpath (not build classpath). The authors of the plugin also offered a ready solution for this: https://kotlinlang.org/docs/gradle-configure-project.html#dependency-on-the-standard-library so we don't hack anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👋 @wzieba !

...then let's maybe instruct kotlin gradle plugin to... not include kotlin-stdlib.

To my understanding, you suggest adding the kotlin.stdlib.default.dependency=false within gradle.properties, but have you (maybe) tried that yourself? This, as I would also expect, fails the build, because although this benchmark module doesn't need the kotlin-stdlib dependency, all other modules do. Isn't that right? 🤔

What we want is to tell our build to ignore the kotlin-stdlib dependency, yes, but for benchmark only. I am not sure what's the correct way to do that, especially since this also needs to be done on the source-set level. 🤷

PS: I think that for what is worth we shouldn't care too much about the configuration of this module, it is just an auto-generate benchmark module that serve a very specific purpose.

Copy link
Member

Choose a reason for hiding this comment

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

Each module can have its own gradle.properties. Having kotlin.stdlib.default.dependency=false in the gradle.properties in benchmark works just fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True @wzieba , I just tried it and it indeed works, I am curious why though, as I would have expected that ./gradlew benchmark:assembleAndroidTest to fail since TrackingMethodsBenchmark is written in Kotlin, but it works and the project compiles, strange... 🤔

No matter, this is now done: a2a349d

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Just sharing that the stdlib contains some additional features, but it's not necessary to compile Kotlin code - that's why this module still compiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah true @wzieba (😅 ) , it is just a Kotlin standard library with some additional features... I didn't have to deal with that for so long that I am now forgetting things, thanks for the merge! 🤷

@wzieba wzieba merged commit 0c4c9c0 into trunk Aug 21, 2024
@wzieba wzieba deleted the build/remove-unused-dependencies branch August 21, 2024 13:32
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.

2 participants