-
Notifications
You must be signed in to change notification settings - Fork 408
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
KT-64377 Maven dev publish #3433
Conversation
…egration-test-build-config
- create a test suite for each project under test (helps with Gradle caching, task avoidance) - update CI/CD tasks to run specific test-suite tasks - disable integration test tasks due to dependency on Maven Local publishing (this will be fixed in an upcoming PR)
# Conflicts: # build-logic/src/main/kotlin/dokkabuild/DokkaBuildProperties.kt # dokka-integration-tests/gradle/README.md # dokka-integration-tests/gradle/build.gradle.kts # dokka-integration-tests/utilities/src/main/kotlin/org/jetbrains/dokka/it/AbstractIntegrationTest.kt
3de37c5
to
d8c86ac
Compare
Maven publish artifacts to project-local directory. This helps with Gradle Build & Config cache.
d8c86ac
to
70fd4f8
Compare
- classpath "org.jetbrains.kotlinx:kotlinx-knit:$knit_version" | ||
+ classpath("org.jetbrains.kotlinx:kotlinx-knit:$knit_version") { | ||
+ exclude(group: "org.jetbrains.kotlinx", module: "dokka-pathsaver-plugin") | ||
+ } | ||
+ classpath("org.jetbrains.kotlinx:dokka-pathsaver-plugin:$knit_version") { | ||
+ exclude(group: "org.jetbrains.dokka", module: "templating-plugin") | ||
+ exclude(group: "org.jetbrains.dokka", module: "dokka-base") | ||
+ } | ||
+ classpath("org.jetbrains.dokka:templating-plugin:${providers.gradleProperty("dokka_it_dokka_version").get()}") | ||
+ classpath("org.jetbrains.dokka:dokka-base:${providers.gradleProperty("dokka_it_dokka_version").get()}") |
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.
Knit uses Dokka v1.8.10 dependencies. Since the local dev-repo is marked as an exclusive source for org.jetbrains.dokka
dependencies, and the repo doesn't contain 1.8.10 versions, the build will fail because it can't find the 1.8.10 dependencies.
Maybe there's a better way of doing this? Or update the version of Coroutines we're testing?
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.
Knit uses Dokka v1.8.10 dependencies. Since the local dev-repo is marked as an exclusive source for org.jetbrains.dokka dependencies, and the repo doesn't contain 1.8.10 versions, the build will fail because it can't find the 1.8.10 dependencies.
Maybe there's a better way of doing this? Or update the version of Coroutines we're testing?
Hmm, that's both good and bad that we found it, I didn't realize it would be a problem...
One of the things that these tests test is the compatibility with Dokka plugins, such as knit. Especially for cases such as when AGP started shipping an older versions of Dokka, and it produced build errors if you had a newer version of Dokka in one of the subprojects (not in the root, which had older Dokka from AGP), and all sorts of things ended up on the classpath. So it would help to keep it working roughly the same way as it would work for the users, meaning coroutines will be running with an explicit new version of Dokka, and with knit that depends on the older version of Dokka.
Or update the version of Coroutines we're testing?
Will it help? Coroutines use the published versions of knit that depend on published versions of Dokka, so if we update coroutines - knit would depend not on Dokka 1.8.10, but on Dokka 1.9.10, which doesn't seem to help us that much?
Maybe there's a better way of doing this?
Just a thought: is there a reason for why the local dev repo should be the exclusive source for org.jetbrains.dokka
? Can it be resolved with the order of the repositories? Meaning, use the local dev-repo as the first repository, so that it resolves all snapshot dependencies first, and then mavenCentral / mavenLocal after it so that it resolves other versions of Dokka and other dependencies if requested? I understand it will slow down the build (Gradle's lookup will be longer), but it's fine in this particular case, won't make much of a difference - integration tests are expected to be rather long and not really expected to be run locally often anyway.
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.
Maybe there's a better way of doing this?
Just a thought: is there a reason for why the local dev repo should be the exclusive source for
org.jetbrains.dokka
?
The exclusivity filter is there to ensure the integration tests only test the current Dokka version.
I wanted to prevent the scenario where 1. publishing to the local test-repo silently fails, and 2. the external projects pick up some other Dokka version (for example, from Maven Local).
If you think that risk is acceptable then we could rely on ordering, as you suggested.
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 wanted to prevent the scenario where 1. publishing to the local test-repo silently fails, and 2. the external projects pick up some other Dokka version (for example, from Maven Local).
An idea: can the value of dokka_it_dokka_version
be something specific to the integration tests? A long time ago it was set to for-integration-tests-SNAPSHOT
, I don't know what for exactly, but I can assume now that it was done to avoid the problem you mentioned. The chances of having a for-integration-tests-SNAPSHOT
Dokka version in Maven Local seem to be slim, so I think it'd be OK
It doesn't have to be for-integration-tests-SNAPSHOT
, but something that is not 2.0.0-SNAPSHOT
, not a release, -dev
or -test
version, which could indeed be available elsewhere
I see that the tests are passing now - were you able to solve it in some other way?
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.
An idea: can the value of
dokka_it_dokka_version
be something specific to the integration tests?
Unfortunately, it's a little more difficult than that :(
The version is set in multiple places (e.g. there's a generated .properties
file in the plugin JAR that contains the version, the version is set into all of the Maven/Gradle metadata.) So in order to set it consistently, it needs to be set from the start. And since the integration-test project is 'just' a consumer of the Dokka subprojects, it's hard for it to influence the version.
Maybe it could be possible if the whole Dokka project just set its version to 1.9.20-DEV by default, but never publishing any -DEV version (we could add some checks in the Gradle tasks to prevent this). Or with some Composite Build hackery.
I see that the tests are passing now - were you able to solve it in some other way?
Oh, the code I highlighted fixed the problem, so the code ran, but I was highlighting to ask if there was a better idea.
Tremendous work with this and all the prerequisites, thanks!
Isn't that the whole point of Say, I have 2.0.0-SNAPSHOT now, I run integration tests and I now have a local Maven repo with Dokka 2.0.0-SNAPSHOT. Then I introduce a breaking change in dokka-base (something obvious such as throwing an exception on start), and run integration tests again without doing |
dokka-integration-tests/gradle/projects/serialization/kotlinx-serialization
Outdated
Show resolved
Hide resolved
- classpath "org.jetbrains.kotlinx:kotlinx-knit:$knit_version" | ||
+ classpath("org.jetbrains.kotlinx:kotlinx-knit:$knit_version") { | ||
+ exclude(group: "org.jetbrains.kotlinx", module: "dokka-pathsaver-plugin") | ||
+ } | ||
+ classpath("org.jetbrains.kotlinx:dokka-pathsaver-plugin:$knit_version") { | ||
+ exclude(group: "org.jetbrains.dokka", module: "templating-plugin") | ||
+ exclude(group: "org.jetbrains.dokka", module: "dokka-base") | ||
+ } | ||
+ classpath("org.jetbrains.dokka:templating-plugin:${providers.gradleProperty("dokka_it_dokka_version").get()}") | ||
+ classpath("org.jetbrains.dokka:dokka-base:${providers.gradleProperty("dokka_it_dokka_version").get()}") |
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.
Knit uses Dokka v1.8.10 dependencies. Since the local dev-repo is marked as an exclusive source for org.jetbrains.dokka dependencies, and the repo doesn't contain 1.8.10 versions, the build will fail because it can't find the 1.8.10 dependencies.
Maybe there's a better way of doing this? Or update the version of Coroutines we're testing?
Hmm, that's both good and bad that we found it, I didn't realize it would be a problem...
One of the things that these tests test is the compatibility with Dokka plugins, such as knit. Especially for cases such as when AGP started shipping an older versions of Dokka, and it produced build errors if you had a newer version of Dokka in one of the subprojects (not in the root, which had older Dokka from AGP), and all sorts of things ended up on the classpath. So it would help to keep it working roughly the same way as it would work for the users, meaning coroutines will be running with an explicit new version of Dokka, and with knit that depends on the older version of Dokka.
Or update the version of Coroutines we're testing?
Will it help? Coroutines use the published versions of knit that depend on published versions of Dokka, so if we update coroutines - knit would depend not on Dokka 1.8.10, but on Dokka 1.9.10, which doesn't seem to help us that much?
Maybe there's a better way of doing this?
Just a thought: is there a reason for why the local dev repo should be the exclusive source for org.jetbrains.dokka
? Can it be resolved with the order of the repositories? Meaning, use the local dev-repo as the first repository, so that it resolves all snapshot dependencies first, and then mavenCentral / mavenLocal after it so that it resolves other versions of Dokka and other dependencies if requested? I understand it will slow down the build (Gradle's lookup will be longer), but it's fine in this particular case, won't make much of a difference - integration tests are expected to be rather long and not really expected to be run locally often anyway.
@@ -1,72 +1,139 @@ | |||
diff --git a/build.gradle b/build.gradle | |||
index e7d405e12..236310823 100644 | |||
index e7d405e12..db5dcec66 100644 |
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.
Slightly concerned with how the diff grew, and some additional changes in here.
Ideally, these integration tests (which are e2e really) should represent the real-life application of Dokka in user projects, so I would expect the diff to contain minimal changes to the build logic. If we start excluding dependencies and overriding them explicltly in a user project - it looks like there's a problem with Dokka itself that needs to be addressed, and we might miss actual bugs
Keeping changes to the absolute bare minimum also helps with updating the versions of the tested projects. I mentioned it that updating the diffs is tedious, because people update their build configs and we then have to resolve merge conflicts and figure out their whole setup to understand where to add things. So far it's been simple: add mavenLocal, bump the version of Dokka, done. Now there's some additional repositories and plugins, dependency manipulation and so on. So it'd be more difficult for us to update it now compared to before, but I think I'm fine with it if you agree to help us update the diffs on request
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, modifying the projects via git diffs is not nice. Testing the external Gradle projects would be easier if they used idiomatic Gradle (for example, if they used Version Catalogs then there would be just one place where the Dokka version should be changed). The build config of Serialization and Coroutines has improved in recent versions, so maybe it would be easier.
We did discuss a while ago about alternatives, and one option is to create a fork of the external projects in Space, and modify it to use the latest Dokka version. That would be easier to update and maintain.
In the meantime, yes, I can help out with updating the external projects as required.
When publishing to repos, yes, Gradle will always publish a new version. This is okay for testing: because we're using a local repo for testing, publishing is effectively a 'zip & copy files' operation, which is cheap and quick to do, and isn't worth caching.
Yes, Gradle will detected that the source files changed, so it will re-run the compilation tasks. The resulting files will be packaged and re-published. |
- only register deterministic repo files as a task input - cache the output of Gradle integration test tasks
Use project-local Maven dev repos for integration testing.
Impact
Using a Maven Repo is useful for integration tests, because it verifies that the artifacts are published correctly with the expected coordinates. It is also useful for Gradle Plugins, because we can verify that the plugin marker was correctly published.
Removing the dependency on Maven Local is one of the last steps before Dokka's build logic (just the build logic, not Dokka Gradle Plugin!) can be made fully Config Cache compatible.
Verification
To test locally, you can use configuration cache:
(Although there will be warnings, they can be ignored - Dokka build scripts (not DGP!) will be made CC compatible in upcoming PRs.)
Summary
Local convention plugin
dokkabuild.dev-maven-publish.gradle.kts
publishes subprojects to a project-local directory.Benefits:
Dynamically inject the local Maven dir:
/* %{PROJECT_LOCAL_MAVEN_DIR}% */
with an exclusive Maven reposettings.xml
with the local plugin repo.