这是indexloc提供的服务,不要输入任何密码
Skip to content

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SimonMarquis
Copy link
Contributor

@SimonMarquis SimonMarquis commented Mar 30, 2025

  • 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

- 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
@SimonMarquis SimonMarquis marked this pull request as ready for review March 30, 2025 19:48
SimonMarquis added a commit to SimonMarquis/nowinandroid that referenced this pull request Apr 5, 2025
- 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/
Copy link
Contributor

@alexvanyo alexvanyo left a 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.

@@ -1,166 +1,118 @@
[versions]
accompanist = "0.37.0"
Copy link
Contributor

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.

Copy link
Contributor Author

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 😉

Copy link
Contributor

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
image

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

@SimonMarquis SimonMarquis Jun 26, 2025

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.

Copy link
Contributor Author

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants