-
Notifications
You must be signed in to change notification settings - Fork 410
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
build config: Gradle Version Catalog + type-safe project dependencies #2884
build config: Gradle Version Catalog + type-safe project dependencies #2884
Conversation
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.
Good to see! We can migrate to type-safe project accessors in next PR.
gradle/libs.versions.toml
Outdated
kotlin-plugin = "213-1.8.10-release-430-IJ6777.52" | ||
kotlinx-coroutines = "1.6.3" | ||
kotlinx-html = "0.7.5" | ||
jsoup = "1.15.3" |
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.
We can inline these once-use versions.
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 it's best to define all the versions at the top, because it's consistent, it's easier to scan the top to see the versions, and if any versions do need to be re-used then then it's less work (personally I find editing libs.versions.toml
a bit fiddly, so I try to avoid it).
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.
We can easy to know which versions are reused after inlining unnecessary ones, but either way.
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 it should be consistent - either all versions are at the top, or all of the versions are inlined, otherwise it might eventually lead to chaos. At the moment, I personally don't have any preference, so I'm fine with either approach 👍
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.
Had a glance - looking good, thanks!
I'll have a closer look once #2704 gets merged and this PR is rebased onto it
Since there are a bunch of conflicts, proposing to merge #2652 first, it's been waiting for a while :) Hopefully shouldn't take as long |
Agreed, sounds good! |
The PR is ready to be rebased :) Looking forward to it! Please, if you can, try to address copy-pasted versions that I mentioned in #2912 (comment), I assumed that we'd have the actual version strings in the toml only |
d59d0eb
to
4f49425
Compare
rootProject.name = "dokka" | ||
|
||
pluginManagement { | ||
includeBuild("build-logic") |
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 moved includeBuild
into the plugins block as recommended
You may also use the
includeBuild
mechanism outsidepluginManagement
to include plugin builds. However, this does not support all use cases and including plugin builds like that might be deprecated in a future Gradle version.
I also moved the repository definitions to be above plugins block, since logically the repos should be defined before plugins are resolved - but that's not important and I can revert if you want.
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 moved it out cause I noticed detekt/detekt#5690.
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.
Hm. We don't use configuration cache in this project, do we? So we don't have the warning yet, but it might resurface one day
I don't mind leaving it as is, we can deal with the warning when we get it, but if you do move it out - please add a comment why
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.
Make sense.
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 don't think the warning in detekt/detekt#5690 is about configuration cache, it's about the (configuration of) build cache.
But Dokka doesn't have that (JetBrains really should set up a Gradle Build Cache server though!)
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.
JetBrains really should set up a Gradle Build Cache server though!
JFYI we actually have a JetBrains instance for Gradle Enterprise (ge.jetbrains.com), but it didn't look like it using it would benefit us in any way compared to what we have now. If there's some really cool things that can be done with it that will work with Dokka, we can switch to it, no problem
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.
If there's public, read-only access to the JetBrains Gradle build cache server then that would definitely help! It means that everyone's builds would be faster.
https://docs.gradle.org/current/userguide/build_cache.html#sec:build_cache_configure_remote
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.
Good to see this, both version catalog and type-safe project accessors!
We can rename this PR's title to something like "Migrate to type-safe dependency accessors"?
gradle/libs.versions.toml
Outdated
kotlin-plugin = "213-1.8.10-release-430-IJ6777.52" | ||
kotlinx-coroutines = "1.6.3" | ||
kotlinx-html = "0.7.5" | ||
jsoup = "1.15.3" |
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.
We can easy to know which versions are reused after inlining unnecessary ones, but either way.
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.
Great to see, as always!
The main integration tests are failing, could you please fix it? I'll run longer integration tests after that
…ode will probably be refactored later in Kotlin#2889)
# Conflicts: # gradle.properties # runners/maven-plugin/build.gradle.kts
- re-use Maven versions from version-catalog - update naming to be more consistent
|
||
compileOnly("org.jetbrains.kotlin:kotlin-gradle-plugin") | ||
compileOnly("com.android.tools.build:gradle:4.0.1") | ||
compileOnly(gradleKotlinDsl()) |
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 removed compileOnly(gradleKotlinDsl())
because it's added by the kotlin-dsl
plugin
integration-tests/gradle/projects/serialization/kotlinx-serialization
Outdated
Show resolved
Hide resolved
Triggered teamcity integration tests, can be merged once they pass upd: some failed :( |
helps with compatibility with older Kotlin versions, probably not needed once support for Kotlin 1.4 is dropped Co-authored-by: Ignat Beresnev <[email protected]>
Looks like all the tests passed, merging it now 🎉 Thank you for this huge amount of work! |
gradle.properties
and intolibs.versions.toml
version catalogDepends on #2652