Skip to content
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

Merged
merged 18 commits into from
Dec 1, 2023

Conversation

dkhalanskyjb
Copy link
Contributor

Fixes #3944

buildSrc/src/main/kotlin/VersionFile.kt Show resolved Hide resolved
buildSrc/src/main/kotlin/VersionFile.kt Show resolved Hide resolved
kotlinx-coroutines-debug/build.gradle.kts Outdated Show resolved Hide resolved
kotlinx-coroutines-debug/build.gradle.kts Outdated Show resolved Hide resolved
Copy link

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

buildSrc/src/main/kotlin/VersionFile.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-debug/build.gradle.kts Outdated Show resolved Hide resolved
kotlinx-coroutines-debug/build.gradle.kts Outdated Show resolved Hide resolved
kotlinx-coroutines-debug/build.gradle.kts Outdated Show resolved Hide resolved
kotlinx-coroutines-debug/build.gradle.kts Outdated Show resolved Hide resolved
kotlinx-coroutines-debug/build.gradle.kts Outdated Show resolved Hide resolved
/* `kotlinx-coroutines-debug` configures `VersionFile` on its own when the corresponding task is created. */
val invalidModules = listOf("kotlinx-coroutines-debug")

configure(subprojects.filter {
Copy link

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)

Copy link
Contributor Author

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.

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.

@dkhalanskyjb dkhalanskyjb force-pushed the dk-remove-debug-module-info branch 3 times, most recently from 4c0cbd1 to 82152a6 Compare November 28, 2023 12:20
@dkhalanskyjb
Copy link
Contributor Author

Using advice from @shanshin, I redid the whole approach to just pack the module-info.class into shadowJar in doLast.

kotlinx-coroutines-debug/build.gradle.kts Outdated Show resolved Hide resolved
kotlinx-coroutines-debug/build.gradle.kts Outdated Show resolved Hide resolved
VersionFile.fromVersionFile(this, versionFileTask)
duplicatesStrategy = DuplicatesStrategy.FAIL
dependsOn(tasks.compileModuleInfoJava)
doLast {
Copy link
Contributor

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

@dkhalanskyjb dkhalanskyjb force-pushed the dk-remove-debug-module-info branch 2 times, most recently from 48ad741 to d5e8cbb Compare November 29, 2023 11:01
@qwwdfsad
Copy link
Contributor

Good to go, great job!

@dkhalanskyjb dkhalanskyjb merged commit 3ceb35d into develop Dec 1, 2023
1 check passed
@dkhalanskyjb dkhalanskyjb deleted the dk-remove-debug-module-info branch December 1, 2023 10:35
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.

None yet

3 participants