-
Notifications
You must be signed in to change notification settings - Fork 411
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: Use buildSrc convention plugins to configure the Dokka subprojects #2704
build config: Use buildSrc convention plugins to configure the Dokka subprojects #2704
Conversation
I made some good progress but then I got stuck when I reached the integration test config, because it's a bit of a beast. I think most of the config in here should be converted to a specific 'integration test' buildSrc plugin. |
@IgnatBeresnev I see you've added the 'Gradle Plugin' label, and while this PR would help with that, it's more about the improving the general Gradle build config of the entire project :) |
Yeah, true. I don't want to create too many labels, but I definitely want to mark all Gradle-related PRs in case we ask for anyone's help with review and whatnot If it makes anyone uneasy, we can certainly create a separate one or rename the existing. We'll discuss it with the team this week, just in case |
With regards to labels, what about a 'build config' or 'ci/cd' label that would cover the build scripts, and also GitHub Actions, Qodana, TeamCity etc? |
…ntion_plugins # Conflicts: # build.gradle.kts
I've made some good progress! All shared build-config now comes from buildSrc convention plugins, not from |
…ption, which is specific to the convention plugin, not for implementations)
(kotlin-dsl plugin will auto-add dependencies)
…ntion_plugins # Conflicts: # runners/gradle-plugin/build.gradle.kts
Hi! Not sure if you noticed that the integration tests for coroutines are unable to be run because of the toolchain being set to 1.8 globally. This refactoring has uncovered quite a number of problems already, which is good, I guess :)
Can you please fix it when you have the time, so that the checks pass? I believe that's the last thing holding up the merge Not sure what the best solution for the fix is, but I'd say any that builds the same bytecode as before and makes the tests pass. We can revisit our requirements for the used java/kotlin versions later and separately |
thanks for checking @IgnatBeresnev, I didn't realise that was the problem. I set the toolchain for Dokka to use Java 8 because that matches the existing Line 33 in ded804e
If I set the toolchain to 11, then the
And there's a warning for
It looks like kotlinx.coroutines has a custom check for the JDK version... I'll look into it a little more. Perhaps the kotlinx.coroutines int. test requires a little more config. |
# Conflicts: # build.gradle.kts Merge pull request Kotlin#3582 from isaialvarado/patch-1 Fix example code Converted Serialization formats list to table PR Kotlin#2010 Updating docs after 7.0.0 release
Yeah, that's the most important part - we cannot break compatibility with Java 8 for now. We can set But whatever works is fine :) We can revisit the used versions later Tell me once it's ready to be looked at, I'll trigger the teamcity build |
@IgnatBeresnev Do you know why the jvm-version is set to 12 in the example workflows?
|
Toolchains don't let you separately define a language version; the JVM target is set to that of the Java version of the Toolchain. So if the project needs to target Java 8, the Toolchain needs to be Java 8. Don't ask me why Fortunately Toolchains are very flexible, so I set the @IgnatBeresnev Tests have passed, it's ready for the TeamCity build! |
It was copied from
Thank you once again! Triggered the builds, will merge once they pass :) |
@IgnatBeresnev By the way, good job on the GitHub labels! They look really smart, and they're helpful. |
@aSemy yeah, going through all of the issues :) Part of the reason is actually for the dokkatoo DSL. In particular, the |
java { | ||
toolchain { | ||
languageVersion.set(JavaLanguageVersion.of(8)) | ||
} | ||
} |
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 we apply toolchain
for all modules, they'll run tests on Java 8 instead of the JDK matrix on CI, we need to apply more toolchains in Test
which will slow down the build, I believe we should not use toolchain
for now. Check
- Configure languageVersion via toolchain detekt/detekt#5735
dokka/.github/workflows/gradle-test.pr.yml
Lines 12 to 13 in f5581c5
matrix: version: [ 8, 11, 17 ]
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 catch!
Toolchains can still help with this though, because it's possible to set them per-task. I did something similar for MockK - I'll put together a PR.
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.
Yeah, I used it, but it's slow, without parallelism, check
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.
Ah right, yes that would be slower. I agree, using GitHub parallel workflows is best, not multiple Gradle tasks.
Hi @aSemy.
This is unfortunate, because it causes variant resolution ambiguity with the Is this an expected consequence of this change?
|
I will take a look! I'm not very familiar with the Shadow plugin, but it looks like there are a few issues relating to What might be a quick fix is disabling publication of this specific configuration, with a similar way to how Test Fixtures publication can be disabled. val javaComponent = components["java"] as AdhocComponentWithVariants
javaComponent.withVariantsFromConfiguration(configurations["testFixturesApiElements"]) { skip() }
javaComponent.withVariantsFromConfiguration(configurations["testFixturesRuntimeElements"]) { skip() } |
This is a large part of the implementation for #2703
Status: ready for review
Important to note:
./buildSrc/build.gradle.kts
, so they are aligned across all subprojectsRelated