-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Cleanup Gradle dependencies and miscellaneous improvements #1856
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
base: main
Are you sure you want to change the base?
Cleanup Gradle dependencies and miscellaneous improvements #1856
Conversation
- Inline versions that were used only once, allowing us to use the compact form for libraries & plugins - Merge TOML declarations with `group` + `name` into the unique `module` component - Expose new BoMs to enforce versioning across packages (`coil-kt-bom`, `kotlin-bom`, `kotlinx-coroutines-bom`, `kotlinx-serialization-bom`, `protobuf-bom`, `okhttp-bom`, `retrofit-bom`) - Refactor `jacoco` version declaration into a proper dependency declaration to enable actual dependency tracking - Rename `androidx.hilt.navigation.compose` to `hilt.ext.navigation.compose` to match the already existing `hiltExt` group. - Replace duplicated `*.gradlePlugin` dependencies with converter that converts plugin alias to usable dependencies. - Sort dependencies - Remove unused `kotlinx-coroutines-android` dependency
- Use BOM platform dependency. - Remove -kt suffix from dependency name has they serve no real purpose. - Add `coil-network` artifact to configure `CallFactory`. - Although "First party Decoder" `SvgDecoder` is now automatically added to each new ImageLoader through a service loader, keeping it in code prevents us from removing the dependency that would appear unused otherwise. - Replace explicit `respectCacheHeaders(false)` with `CacheStrategy::DEFAULT` which does not respect cache-control headers. - Migrate `ImageLoaderFactory` to `SingletonImageLoader.Factory` - Inline TOML dependencies (see android#1856) docs: https://coil-kt.github.io/coil/upgrading_to_coil3/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleanup! I like all of these changes except for the inlining of versions out of the [versions]
block if they're only used in one place - I feel like that ends being a bit messier even if slightly longer.
gradle/libs.versions.toml
Outdated
@@ -1,166 +1,118 @@ | |||
[versions] | |||
accompanist = "0.37.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of inlining versions that only appear once - it does remove lines from this file, but I find it a lot clearer to have all of the versions in continuous place.
For future maintenance, this also seems like this would be more likely to have a dependency get copied in the same version group without moving that shared version back to the versions
section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to agree with you, but in practice, this almost never holds true 😛.
Having top level [versions]
should be the exception, not the norm.
Let me try to explain my reasoning:
I find it a lot clearer to have all of the versions in continuous place.
I kind of disagree on this, but this is a personal opinion. I can't trust top-level variables like this, as I don't know exactly what they are used for, unless I also check where (and if) the variable is actually used/referenced in the file. So it always end up in extra checks and lookups everytime.
I always prefer simplicity, meaning less indirections.
For future maintenance, this also seems like this would be more likely to have a dependency get copied in the same version group without moving that shared version back to the versions section.
Again, in practice this is almost never what actually happens. We almost always update dependencies with automated tools like renovate, dependabot or whatever. And they take care of this for us. There is also Android Lint rules for this, when multiple versions are referenced for the same dependency. (I also implemented custom lint rules to avoid cases like this)
And most of the multi-artifacts dependencies are now slowly migrating to BoMs making the overall process simpler and less error prone, like Jetpack Compose (except all the other Jetpack library groups for whatever reason 😡)
Anyways, I'm not here to impose anything, but only to suggest improvements I feel worthy, so let me know if I should proceed to the un-inlining of versions in this file 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the inline version makes [version] category meaningless, and it would ruin the TOML convention.
If you want to remove the one time used version, you should be using this below.
This snippet from a gradle version catalog documentation.
https://docs.gradle.org/current/userguide/version_catalogs.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clearly a downgrade. Also, Gradle recently published guidelines that go against this.
https://docs.gradle.org/current/userguide/best_practices_dependencies.html#single-gav-string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the inline version makes [version] category meaningless, and it would ruin the TOML convention.
This is the whole point. Not using what is not necessary.
Which convention exactly? Extracting version numbers does not serve any purpose here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not wholly against this change. I think it does make it easier to read the dependencies, however, I also think it needs further investigation (does Android Studio break this format by automatically adding in new versions for single dependencies? Does this format work with our own version update tools/processes?)
@SimonMarquis Given that the rest of this PR is good to merge, would it be possible to split this change into a new PR we can discuss further there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dturner Ok, I'll restore the versions variables: 967e7ac
does Android Studio break this format by automatically adding in new versions for single dependencies?
What do you mean exactly? The Android Studio intent to add a missing dependency when it sees a known class? Android Studio has always been in a best-effort.
For instance, I tried to reference these 4 classes:
EmojiCompat
EmojiPickerView
EmojiButton
EmojiEditTextHelper
and here is what Android Studio did:
[versions]
emoji2 = "1.5.0"
emoji2Emojipicker = "1.5.0"
emoji2Views = "1.5.0"
emoji2ViewsHelper = "1.5.0"
[libraries]
androidx-emoji2 = { group = "androidx.emoji2", name = "emoji2", version.ref = "emoji2" }
androidx-emoji2-emojipicker = { group = "androidx.emoji2", name = "emoji2-emojipicker", version.ref = "emoji2Emojipicker" }
androidx-emoji2-views = { group = "androidx.emoji2", name = "emoji2-views", version.ref = "emoji2Views" }
androidx-emoji2-views-helper = { group = "androidx.emoji2", name = "emoji2-views-helper", version.ref = "emoji2ViewsHelper" }
So... I'll let you judge if we should follow what Android Studio does 😝
Does this format work with our own version update tools/processes?
Yes is does, you can see it here: android/platform-samples@4c394aa#diff-697f70cdd88ba88fe77eebda60c7e143f6ad1286bca75017421e93ad84fb87df
It is also smart enough to apply the same version update to multiple entries if they were not using a variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll re-open a new PR with the inlined versions after this one is merged so that we can continue the discussion 👍
Inline versions that were used only once, allowing us to use the compact form for libraries & pluginsgroup
+name
into the uniquemodule
componentcoil-kt-bom
,kotlin-bom
,kotlinx-coroutines-bom
,kotlinx-serialization-bom
,protobuf-bom
,okhttp-bom
,retrofit-bom
)jacoco
version declaration into a proper dependency declaration to enable actual dependency trackingandroidx.hilt.navigation.compose
tohilt.ext.navigation.compose
to match the already existinghiltExt
group.*.gradlePlugin
dependencies with converter that converts plugin alias to usable dependencies.kotlinx-coroutines-android
dependency