Change project build structure and use version catalog for storing dependencies #50
Conversation
|
I also had problems with compilation after removing casts to KProperty1 in ValidatableTest despite my IDE compiler was pretty sure those casts were unnecessary (and project was alredy using kotlin version 2.0.20). I was able to fix this in c8ad3f4, what do you think? Looks like method api should stay the same |
bejibx
changed the title
|
Reverted my changes regarding definitely non-nullable types. @nesk Is there any chance you'll be able to look at this PR anytime soon and provide some feedback? |
- build-conventions module moved to akkurate-build-conventions for consistency - lib version updated to 0.11.0 (forgot about this during merge) - extension for kmp library component plugin renamed from "kmpLibrary" to "component" - "ignoreTargets" renamed to "ignoredTargets" for better consistency with similar gradle kotlin dsl - simplify ignoredTargets method realization. KmpTarget enum is no longer needed
Really sorry for taking som much time. This isn't an easy PR to review 😁 My hunch here is don't want that much modifications in the build sources. It took me quite some time to improve them to current state and I'm afraid to break publishing/signing. However, I'm really interested in the But let me make a proper review of your PR, to see what we can keep or not :) And thank you for your time! |
Yay! Thank you! No need to rush, I just wanted any reaction (even, "it is too much, I'm closing this one" 😁). Maybe I shouldn't have changed plugin structure this much, probably I got too excited during the process. In my defence I can say that I mostly tried to reorganize plugins into smaller peaces without changing actual logic. KMP targets setup may look different, but it uses same logic as before - we have some predefined supported targets and in some cases we can disable some. I just have to slightly adapt implementation for work with convention plugins. I have to tell you, this is first time I manage to keep ALL dependencies and versions inside catalog. Every time I tried this before, some libraries (mostly plugins) remain declared outside of it despite my dest efforts. So I'm pretty surprised I finally found a way to do this. As I said before, I'm mostly worrying about publish logic. Same thing, I tried not to change anything, just to collect all publish logic in one plugin. Sadly I have no idea how to test it on my side. Hmmm... Actually now when I'm thinking about it maybe I just finally need to figure out how to test gradle using their testing toolkit? Be right back! 😁 Oh, and please ask me if something is unclear. I'm ready to explain things. |
|
Hi @bejibx, I've finally took the time to review this. First of all, thank you again for your contribution! After reviewing, I stand by what I said: this PR is a bit too much. You bring a lot of build modularity (and that's impressive!) for a library that doesn't require that much of it. The build system of Akkurate is actually quite simple and should stay like that (it has its quirks but it's mainly related to Gradle/KMP support). I've created some convention plugins only to avoid repeating myself across multiple Gradle projects. This is why the KMP libraries share a convention plugin, while the KSP processor doesn't publish any convention plugin for its specific use case. I intend to keep it as simple as this. It's really interesting to see how you've managed the target configuration, nice engineering here! But I'd rather stay with the extension function. This isn't as beautiful than what you did, but it's way more maintainable and understandable. I intend to keep the "beautiful API" for the public API of Akkurate, not for its own build system :) However, there are some really good things you brought here that I would love to see in Akkurate:
What I propose you is to create a new PR for the latter. And, concerning the former, don't waste your time, I will simply create a commit in my
Could you list me the exceptions here? |
Oh, I already fix this in later commit Ok, I will make new PR only with version catalog. There is only one problem I think. If I remember correctly there was some difficulties with using version catalog in buildSrc and that's why I moved everything into separate module. Are you ok with it or should I figure out how to do same thing with buildSrc? |
|
It's OK if some version numbers stay in the |
Looks like only place where I can't use version catalog would be Should I instead declare it in every place where this convention plugin is used? |
|
Closing in favor of #56 |
2 participants
Fixes #46. Sorry I did not discussed these changes with you ahead of time. I'm completely ok with you discarding this PR because I did this mostly as an exercise in gradle configuration. So mostly what I've done is:
Ok, now for some implementation details.
Convention plugins now organized into 3 different categories: base, component and feature. Base plugins just contains some reusable build logic for component plugins. Component plugins configure project modules. Feature plugins configure separate build features. I'm also rearrange plugin hierarchy a bit. Namely kmp-library (successor of kmp-library-conventions) plugin now not applying publishing-conventions. Did this because publishing seams to me like a feature which component could have or not.
Pretty much all versions are now listed inside version catalog with few small exception due to the fact that it's impossible to use generated catalog accessors in convention plugins. Plugin versions are declared by including concrete plugin dependencies inside root build.gradle.kts in included build. I don't use plugin aliases because in my experience their usage is problematic, especially with kotlin plugin (see gradle/gradle#17968 for example).
I switched from extension method to convention plugin for target configuration inside kmp library modules. Firstly, I made attempt to make target ignoring syntax similar to how we adding new targets in kotlin multiplatform plugin. Secondly, for configuring actual ignored targets I have to add plugin extension and it was quite tricky to figure out how to configure targets from convention plugin and, at the same time, get fully configured extension from the build script apllying this plugin. It seems gradle have no way to await specific extension configuration, my first solution was to configure targets in
afterEvaluate {}block but later I tried another approach - to initially add all targets and remove them by name while subscribing to property changes in plugin extension and to my surprice this seem to work. I don't really like this solution but after reading a TON of forums, github issues and slack conversations it seems there is no idiomatic way to sync configuration phases between convention plugin and extensions in consumer plugin. Either this orafterEvaluate {}and everybody sayingafterEvaluateis bad (duh).I had to update some deps in the process, most notably:
I ran all unit tests locally, all seems green. My main concern is I don't know how to check if publication stuff still working as intended. Can I somehow test this on my side or will you help me to check?