-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix kotlinx-coroutines-debug having the wrong module-info #3948
Conversation
c93bdc0
to
13f969f
Compare
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 left some tips on fixing some configuration avoidance breakage and some config cache breakage. These are informative, and should probably not block this MR, but it's mainly to share good practise and speed up the build.
/* `kotlinx-coroutines-debug` configures `VersionFile` on its own when the corresponding task is created. */ | ||
val invalidModules = listOf("kotlinx-coroutines-debug") | ||
|
||
configure(subprojects.filter { |
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.
Configuring subproject is usually a bad sign.
Instead, you should probably add this configuration logic to your convention plugin, and apply the convention plugin to all relevant subprojects using the plugins
block.
Gradle recommend that approach as it allow more optimisations. Also, more clarity.
See that part of the docs.
You are already using a convention plugin, you just need to use it fully. (I can detail if you need)
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.
Thank you, the Archangel. The wisdom of this advice is indisputable. Yet our land is too full of sin to be ready for such a revelation. The project is ripe with the filthy touch of subproject iteration, and making one place holy will but incite more chaos. The whole city must be burned to crisps before it rises as a temple (if you catch my meaning).
Though if you have quick access to some specific links to the required reading, sure, please share them. Maybe I'm wrong, and this could conveniently be done one iteration over subprojects at a time, then there's no reason not to do that.
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.
Hahahahaha.
I did not look into details yet, I may not have time today, but maybe if I'm bored this weekend, we never know!
But indeed, feel free to ignore my comment for now, it was mainly informative and we probably should nuke all of these cross-configuration at once.
554d99b
to
7521130
Compare
4c0cbd1
to
82152a6
Compare
Using advice from @shanshin, I redid the whole approach to just pack the |
VersionFile.fromVersionFile(this, versionFileTask) | ||
duplicatesStrategy = DuplicatesStrategy.FAIL | ||
dependsOn(tasks.compileModuleInfoJava) | ||
doLast { |
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.
Thumbs up for the new approach, it's much more straightforward now
48ad741
to
d5e8cbb
Compare
This module-info taken from bytebuddy was incorrectly picked up as the module info for `kotlinx-coroutines-debug`
Also try to fix the shadow jar being published
Co-authored-by: Vsevolod Tolstopyatov <qwwdfsad@gmail.com>
Co-authored-by: Titouan BION <titouan.bion@gmail.com>
Co-authored-by: Titouan BION <titouan.bion@gmail.com>
Co-authored-by: Titouan BION <titouan.bion@gmail.com>
d5e8cbb
to
afa50d0
Compare
Good to go, great job! |
Fixes #3944