Skip to content

[K2] Run unit tests against the latest Analysis API #3552

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 5, 2024
Merged

[K2] Run unit tests against the latest Analysis API #3552

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a71a6a
[K2] Run unit tests against the latest Analysis API
vmishenev Mar 28, 2024
98d33b2
Use a property with an override version and overwrite a version catalog
vmishenev Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions build-logic/src/main/kotlin/dokkabuild/DokkaBuildProperties.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ abstract class DokkaBuildProperties @Inject constructor(
private val layout: ProjectLayout,
) {

/**
* The latest version of Analysis API (`kotlin-compiler-latest` from the toml file) should be used to build the Dokka
*/
val useTheLatestAnalysisAPI: Provider<Boolean> =
dokkaProperty("build.useTheLatestAnalysisAPI", String::toBoolean)
.orElse(false)

private val buildingOnTeamCity: Provider<Boolean> =
providers.environmentVariable("TEAMCITY_VERSION").map(String::isNotBlank)

Expand Down
5 changes: 5 additions & 0 deletions dokka-subprojects/analysis-kotlin-symbols/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,8 @@ The `.jar` produced by this project shadows all dependencies. There are several
2. There are many intertwining transitive dependencies of different versions, as well as direct copy-paste,
that can lead to runtime errors due to classpath conflicts, so it's best to let Gradle take care of
dependency resolution, and then pack everything into a single jar in a single place that can be tuned.

## Testing with the latest version of Analysis API

To build it with the latest version of Analysis API, the property
`org.jetbrains.dokka.build.useTheLatestAnalysisAPI=true` should be added to the project.
13 changes: 13 additions & 0 deletions dokka-subprojects/analysis-kotlin-symbols/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,3 +140,16 @@ tasks.shadowJar {
configurations = emptyList()
from(shadowOverride, shadowDependenciesJar)
}

// for testing with the latest version of Analysis API
if (dokkaBuild.useTheLatestAnalysisAPI.get()) {
Copy link
Collaborator

@whyoleg whyoleg Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, that it will better to override version directly in the version catalog instead of via configurations as it will be more idiomatic.
F.e like described in this issue: gradle/gradle#20070

So in root settings.gradle.kts we should have something like:

dependencyResolutionManagement {
    versionCatalogs {
        create("libs") {
            val kotlinCompilerK2Version = providers.gradleProperty(
                "org.jetbrains.dokka.build.kotlin-compiler-k2-version"
            ).orNull
            if (kotlinCompilerK2Version != null) {
                version("kotlin-compiler-k2", kotlinCompilerK2Version)
            }
        }
    }
}

(property name is TBD, just an example)
And then, no additional configuration is needed in a project.

It's possible to just have a boolean flag, but I believe that it will be better if we provide this version on CI (as you suggested in the comment above) as it will allow us easier to update to newer version, by just changing property on TeamCity instead of changing it in the repository.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this for some time.

I personally like both approaches (the current one and what Oleg proposed), both have advantages and disadvantages. Oleg's proposal seems to cover some additional use cases though: it will be possible to run the tests with any custom version of kotlin-compiler-k2, not just the hardcoded and latest -dev one. This might come in useful when debugging or trying to narrow down the version in which a bug appeared (i.e test 2.0.20-dev-10, then 2.0.20-dev-1, then 2.0.20-dev-5, etc) without having to change the project's code - all done in CI

The proposed approach does have one disadvantage in my opinion: it's less obvious what is happening, we're overriding but a single specific version in the very common and unexpected place. In Vadim's approach the resolution logic is mostly isolated in analysis-kotlin-symbols, which is nice and easy to understand. But hopefully Gradle will provide some sort of a solution in gradle/gradle#20070, like -Pgradle.libs.kotlin-compiler-k2="2.0.20-dev-+", and this disadvantage won't be relevant anymore, and the resolution code will actually be easy to get rid of.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, missed @vmishenev's comment above

Extract the version pattern from the property directly (org.jetbrains.dokka.build.useTheLatestAnalysisAPI=2.0.20-dev-+) rather than from the toml file. It can be more convenient to change the pattern

I guess I would mostly vote for passing the version as a property (instead of the boolean flag), not necessarily as Oleg proposed :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed approach does have one disadvantage in my opinion: it's less obvious what is happening, we're overriding but a single specific version in the very common and unexpected place.

TBH, for me, the variant with configurations in this PR is less obvious, because configurations are like for advanced usage and most of the time there is a solution which doesn't involve them and it's easier and more straightforward.

F.e with logic implemented in current PR we override all dependencies (not only which use kotlin-compiler-k2 version) to newer version. F.e in case we will have kotlin-compiler-k2=2.1.0 and kotlin-compiler-k2-override-version=2.2.0-dev-* and some dependency with version 2.1.0 we will update this dependency version and so receive a build failure. May be it will not happen, but when something like this happens, the issue is not obvious. It could be prevented of course, by checking group/module names of dependency.

Instead when we dynamically modify version inside Version Catalog we are explicitly changing only one version and so only those dependencies which use it via Gradle-native way.
Additionally, what if we would like to add one more property to configure testing with newer version of kotlin-compiler for descriptors (to check, that there is no regressions in analysis) or any other thing - I would expect to see all such version overrides in one place, not in separate projects.

While for current use-case both variants are fine, IMO we should use version catalogs native solution instead of configuration based.

Copy link
Contributor Author

@vmishenev vmishenev Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with logic implemented in current PR we override all dependencies

Yes, I agree. It can theoretically lead to false-positive builds.

I would expect to see all such version overrides in one place, not in separate projects.

By the way, I see the request on some local DSL to override a version https://github.com/gradle/gradle/issues/26029
since e.g., another implementation is to override a version only for the test configuration. (but does not work for our integration tests)

I think we can try https://docs.gradle.org/current/userguide/platforms.html#sec:overwriting-catalog-versions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it would be nicer to have the version defined in one place, and to allow it to be overridden with a specific version. Using the version catalog has some benefits (it's nice to have a version defined in one place, and it should be possible to override a version via a runtime property).

However, Gradle dependency management is tricky...

A version catalog doesn't always control the version. Example: if a project has a direct dependency on x.y.z:foo:1.0.0, and some other dependency has a transient dependency on x.y.z:foo:2.0.0, then Gradle will select the highest version.

So, while version catalogs are convenient, to control the version of dependencies we'd need to use constraints.

This is what I'd propose:

  1. In DokkaBuild, add a val kotlinAnalysisVersionOverride: Provider<String>.

  2. And then, in the project, define constraints to control the versions. This can either use the overridden version (if present), or the default value in the version catalog.

    // dokka-subprojects/analysis-kotlin-symbols/build.gradle.kts


dependencies {
    constraints {
        listOf(
            libs.kotlin.high.level.api.api,
            libs.kotlin.high.level.api.impl,
            libs.kotlin.high.level.api.fir,
            // ...
        ).forEach {
            implementation(it) {
                version {
                    strictly(
                        // find the optional override,
                        dokkaBuild.kotlinAnalysisVersionOverride
                            // otherwise, use the default version
                            .orElse(libs.versions.kotlin.compiler.k2)
                            .get()
                    )
                }
            }
        }
    }
}

For Dokka currently, the analysis dependencies are shadowed into the project's artifact, so I don't think that aligning the versions across multiple projects is required. If it is, and the constraints should be shared across subprojects, then the solution is to use the Java Platform Plugin to share the constraints.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is interesting. Thank you for participating.
It seems overly strict.

A version catalog doesn't always control the version. Example: if a project has a direct dependency on x.y.z:foo:1.0.0, and some other dependency has a transient dependency on x.y.z:foo:2.0.0, then Gradle will select the highest version.

I thought we should have an existing case to introduce a version constraint.
Currently, I can not imagine any case with Analysis API surface that breaks using the API.

@whyoleg Do you have an opinion on what approach we should implement?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, I think that constraints is the direct replacement for what is proposed in current PR (using configurations), but with nicer API and more strict behaviour.
But, I'm not sure, that we will fall in the situation when for some reason, we would have transitive dependency on Analysis API of higher version :)
So we have 2 options:

  • using constraints
  • overriding version in version catalog

I would say, that the main difference for our use case is locality of the change:

  • with constraints - we override version for single module
  • with version catalog - we override version in whole project

Both variants are fine for me. Still I'm more in favour of changing the version via version catalog. No specific arguments for this, mostly it just fills more clear and natural for me: if we want to override version, we override it in version catalog.
BTW, @atyrin setup to test Dokka over newer Kotlin versions also overrides version via version catalog, so setup will be similar and so more understandable (IMO).

On additional note: I think we agreed that we will provide newer dev version via property (not just boolean flag) and so it would be good to add to the Version Catalog near kotlin-compiler-k2 that this version could be overridden by Gradle property.

project.logger.lifecycle("Using the latest version of Analysis API")
configurations.all {
resolutionStrategy.eachDependency {
if (requested.version == libs.versions.kotlin.compiler.k2.get()) {
useVersion(libs.versions.kotlin.compiler.latest.get())
because("unit testing can use the latest version of Analysis API")
}
}
}
}
3 changes: 3 additions & 0 deletions gradle/libs.versions.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ kotlinx-bcv = "0.13.2"
## Analysis
kotlin-compiler = "1.9.22"
kotlin-compiler-k2 = "2.0.20-dev-7"
# to run testing against the latest dev build of Analysis API
# it is used in the build script of symbol analysis
kotlin-compiler-latest = "2.0.20-dev-+"

# MUST match the version of the intellij platform used in the kotlin compiler,
# otherwise this will lead to different versions of psi API and implementations
Expand Down