-
Notifications
You must be signed in to change notification settings - Fork 0
Prototype migration of dependencies to a published Version Catalog #145
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: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #145 +/- ##
============================================
+ Coverage 94.61% 94.73% +0.11%
- Complexity 341 342 +1
============================================
Files 52 52
Lines 892 892
Branches 18 18
============================================
+ Hits 844 845 +1
Misses 43 43
+ Partials 5 4 -1 |
|
@armiol @alexander-yevsyukov Please, take a look. |
alexander-yevsyukov
left a comment
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.
Got only one comment. But I did only a cursory review. In general it's LGTM. Unfortunately, I cannot put much efforts in this PR — busy with other urgent things now. So I rely on review from @armiol for a formal approval.
armiol
left a comment
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.
@yevhenii-nadtochii please see my comments. We need to discuss my comments vocally.
buildSrc/build.gradle.kts
Outdated
| * Dokka Releases</a> | ||
| */ | ||
| val dokkaVersion = "1.6.20" | ||
| // Let's get rid of warnings about different Kotlin version on the classpath. |
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.
Reading it again, I am thinking now, to which piece of code this comment relates? Perhaps, it is misplaced?
buildSrc/build.gradle.kts
Outdated
| implementation(libs.protobuf.gradlePlugin) | ||
|
|
||
| /* | ||
| These guys below use a fat jar with Kotlin runtime inside. One more |
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.
Let's be more polite and format here. They aren't "these guys".
version-catalog/README.md
Outdated
|
|
||
| `spine-version-catalog` consists of several modules: | ||
|
|
||
| 1. `api` – represents a facade upon Gradle's provided `VersionCatalogBuilder`. |
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 have discussed this previously. I would not have this module as a standalone entity.
| package io.spine.internal.catalog | ||
|
|
||
| /** | ||
| * Defines what information is necessary to create one or another version |
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.
Did you mean this?
"Defines the information necessary to create a versioned catalog-compatible item."
| * Defines what information is necessary to create one or another version | ||
| * catalog-compatible item. | ||
| * | ||
| * Notations are citizens of a declaration site. They form DSL and say what is |
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 need to discuss these notations vocally. So far, I have very little understanding of what they are (especially in this DSL part), and why there's still some parallel hierarchies with "Catalog-Version-Library-..." terms.
| * | ||
| * See direct implementations: [PluginEntry], [DependencyEntry]. | ||
| */ | ||
| open class VersionInheritingEntry : CatalogEntry(), VersionNotation { |
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.
Let's discuss this vocally. From our last round, you said CatalogEntry is a counter-example for VersionInheritingEntry, as CatalogEntry takes the properties from its children, while VersionInheritingEntry descendants — at the moment — just take their versions from their parent.
Two points:
- We might need to inherit more than just a version. Hence the name is still questionable.
CatalogEntryis not a counter-example, but a direct parent type. Which makes your previous comments confusing to me.
|
|
||
| // "GradlePlugin" - is a special entry name for `PluginEntry`. | ||
| // For plugin entries with this name, the facade will not put "gradlePlugin" | ||
| // suffix for a plugin's id. Note, that we have this suffix for the version |
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.
"ID".
| package io.spine.internal.catalog.entry | ||
|
|
||
| /** | ||
| * This dependency describes an imaginary library. |
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.
Here it is again. See, you say that it describes a library, while you aren't using a LibraryEntry on top, but a DependencyEntry.
I suggest to pick just one of them and kill another one.
...ion-catalog/func-test/dummy-catalog/src/main/kotlin/io/spine/internal/catalog/entry/Dummy.kt
Show resolved
Hide resolved
| object Tools : VersionEntry() { | ||
| override val version = "3.0.0" // libs.versions.dummy.tools | ||
| } | ||
| } |
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.
It would also be nice to see how one overrides a version of some Dummy part (such as Gradle plugin, for instance) in a Gradle project which originally imports the version catalog declaring Dummy.
9a6f115 to
17aaf48
Compare
|
@armiol Please take another look. |
version-catalog/README.md
Outdated
| 1. Go to `catalog` module. | ||
| 2. Open `io.spine.internal.catalog.entry` package. | ||
| 3. Create a new file there, which contains an object declaration, named after | ||
| a dependency, that is being added. |
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.
Please adjust the text wrapping so that vertical line of item numbers is not broken by wrapped text.
armiol
left a comment
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.
@yevhenii-nadtochii LGTM with a number of minor documentation-related comments. Please make sure to address those.
version-catalog/README.md
Outdated
| val api by lib("$group:dummy-api") // libs.dummy.api | ||
|
|
||
| // In bundles, you can reference entries (which declare module), extra | ||
| // libraries or declare them in-place. |
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.
", or declare"
version-catalog/README.md
Outdated
|
|
||
| ## Overriding of items shipped by `SpineVersionCatalog` | ||
|
|
||
| Sometimes, it happens that a projects needs to override items, shipped by the catalog. |
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.
"that a project"
|
|
||
| When overriding libraries and plugins, a version should be specified in-place. | ||
|
|
||
| For example: |
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.
In this example, there should be more comments which literally point to the rules which you describe in the sentences above.
version-catalog/README.md
Outdated
| which exposes the declarative facade. | ||
| 2. Makes `dummy-project` use `dummy-catalog` from Maven local. | ||
| 3. Builds `dummy-project`. It has assertions in its build file. Those assertions | ||
| verify the generated type-safe accessors to `Dummy` dependency. When any |
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 any"
This PR is a prototype. It is not going to be merged.
It addresses #359.
Information on this matter:
The PR prototypes migration of dependencies into a published version catalog. Initially, it was supposed to be a plugin for
Settings, that should have declared alibscatalog on its own. But due to the problems with override, it took the form of just class which can operate upon Gradle-providedVersionCatalogBuilder.In this PR, the catalog is represented by a separate
version-catalogproject that is published to Maven local in order to simplify prototyping. Also, it resides intimeproject, they are independent.timemodules use this catalog, fetching it from Maven local insettings.gradle.ktsfile.version-cataloghas a facade uponVersionCatalogBuilder, which provides a declarative API for dependencies. The main goal was to hide the imperative nature of the builder. The spirit of the resulted API for dependencies declaration is very similar to the way we declare dependencies now. As a result, we get all the same Kotlin objects, only with a more strict, unified structure.What have been done:
buildSrc/io/spine/internal/dependencyto the catalog and translated with the new API. For some of them, GitHub set an incorrect match in itsFiles changedsection, but it is still possible to compare most of them.buildSrcandtimemodules have been adopted to use Version Catalog, which the module provides.A showcase of the new API is a
Dummydependency. It covers all the available API in a single place. The comments show how a particular statements will be reflected in the generated type-safe accessors. Details on a local overriding of items, shipped by the catalog, can be found in README file.The code of
Dummydependency, which showcases the API:The issues below are relevant to this prototype:
1. Using of Version Catalog corrupts applying of standalone scripts.
2. Make version catalogs accessible from precompiled script plugins.
3. Typesafe accessors to version catalog do not work in the subprojects block.
4. Provide a way to use plugin definition from version catalog without version.
5. Accept plugin declarations from version catalog also as libraries.
6. False-positive "can't be called in this context by implicit receiver" with plugins in Gradle version catalogs as a TOML file.
7. Add possibility to overwrite items of already created version catalogs.
8. Provide means for testing Settings plugins.
Off-topic
Having to use output from
buildSrca lot for development, I've tried to get rid of the warnings it emits:'compileJava' task (current target is 11) and 'compileKotlin' task (current target is 1.8) jvm target compatibility should be set to the same Java version.- nothing can be done. It's a bug in Gradle. They state, it is fixed inv7.5, which is the current Release Candidate.buildSrcto the one, used by Gradle itself. I've left a note about this inbuildSrc/settings.gradle.ktsfile.