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.

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

build(deps): Bump the kotlin-ksp group with 11 updates #21515

Closed
wants to merge 9 commits into from
Closed

build(deps): Bump the kotlin-ksp group with 11 updates #21515

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ffcae27
build(deps): Bump the kotlin-ksp group with 11 updates
dependabot[bot] Dec 10, 2024
57abfc7
Merge branch 'trunk' into dependabot/gradle/kotlin-ksp-31007f7a9e
nbradbury Dec 10, 2024
e41ac16
Use uppercase and lowercase
nbradbury Dec 10, 2024
8b88057
Merge remote-tracking branch 'origin/dependabot/gradle/kotlin-ksp-310…
nbradbury Dec 10, 2024
81024b1
Fixed getSerializableCompat warning
nbradbury Dec 10, 2024
0b94318
Fixed "Specific imports should be used for FluxC Store inner classes"…
nbradbury Dec 10, 2024
cc33adc
Explicitly don't use KSP2
wzieba Dec 11, 2024
631549d
Replace kotlin compile testing with a fork
wzieba Dec 11, 2024
ca1d061
Removed locale
nbradbury Dec 12, 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
2 changes: 2 additions & 0 deletions gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,7 @@ android.nonTransitiveRClass=true
android.nonFinalResIds=false
android.enableR8.fullMode=false

ksp.useKSP2=false
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the commit message and your reasoning about this change @wzieba , makes this all very clear! 🙇

...but it ended up not to be.

I propose setting this flag to false to explicitly describe which KSP version we use, as I haven't found any way to log this.

But just to make it clear here, this ksp.useKSP2 flag is false by default atm, even on Kotlin 2.1.0? We just make this explicit with this change, right? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. There was plan to make this true with Kotlin 2.1, but apparently, this changed. So due to this uncertainty, I think it's best we hardcode usage of KSP1 for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks and I agree, good call @wzieba ! 👍

FYI: Maybe we should propagate the same (explicit) configuration to all other client projects. 🤔

PS: We should also just make sure to revisit that at some point and flip it to true when everything starts working on the KSP2 side again, maybe we should add that as an Apps Infra Roadmap so we don't forget. 🤔


# Dependency Analysis Plugin
dependency.analysis.android.ignored.variants=release,wordpressVanillaDebug,wordpressVanillaRelease,wordpressWasabiDebug,wordpressWasabiRelease,wordpressJalapenoRelease,jetpackVanillaDebug,jetpackVanillaRelease,jetpackWasabiDebug,jetpackWasabiRelease,jetpackJalapenoRelease
6 changes: 3 additions & 3 deletions gradle/libs.versions.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ jna = '5.15.0@aar'
json-path = '2.9.0'
jsoup = '1.18.3'
junit = '4.13.2'
kotlin-compile-testing = '1.6.0'
kotlin-compile-testing = '0.7.0'
kotlin-main = '2.1.0'
kotlinx-coroutines = '1.8.1'
kotlinx-kover = '0.7.6'
Expand Down Expand Up @@ -232,8 +232,8 @@ jna = { module = "net.java.dev.jna:jna", version.ref = "jna" }
json-path = { group = "com.jayway.jsonpath", name = "json-path", version.ref = "json-path" }
jsoup = { group = "org.jsoup", name = "jsoup", version.ref = "jsoup" }
junit = { group = "junit", name = "junit", version.ref = "junit" }
kotlin-compile-testing-ksp = { group = "com.github.tschuchortdev", name = "kotlin-compile-testing-ksp", version.ref = "kotlin-compile-testing" }
kotlin-compile-testing-main = { group = "com.github.tschuchortdev", name = "kotlin-compile-testing", version.ref = "kotlin-compile-testing" }
kotlin-compile-testing-ksp = { group = "dev.zacsweers.kctfork", name = "ksp", version.ref = "kotlin-compile-testing" }
kotlin-compile-testing-main = { group = "dev.zacsweers.kctfork", name = "core", version.ref = "kotlin-compile-testing" }
kotlin-reflect = { group = "org.jetbrains.kotlin", name = "kotlin-reflect", version.ref = "kotlin-main" }
kotlin-test-junit = { group = "org.jetbrains.kotlin", name = "kotlin-test-junit", version.ref = "kotlin-main" }
kotlinx-coroutines-android = { group = "org.jetbrains.kotlinx", name = "kotlinx-coroutines-android", version.ref = "kotlinx-coroutines" }
Expand Down
11 changes: 7 additions & 4 deletions libs/processors/src/test/kotlin/org/wordpress/android/processor/RemoteConfigProcessorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ package org.wordpress.android.processor

import com.tschuchort.compiletesting.KotlinCompilation
import com.tschuchort.compiletesting.SourceFile
import com.tschuchort.compiletesting.kspWithCompilation
import com.tschuchort.compiletesting.symbolProcessorProviders
import com.tschuchort.compiletesting.configureKsp
import org.assertj.core.api.Assertions.assertThat
import org.jetbrains.kotlin.compiler.plugin.ExperimentalCompilerApi
import org.jetbrains.kotlin.utils.addToStdlib.UnsafeCastFunction
Expand Down Expand Up @@ -119,10 +118,14 @@ class RemoteConfigProcessorTest {

private fun compile(src: List<SourceFile>) = KotlinCompilation().apply {
sources = src + fakeAppConfig
symbolProcessorProviders = listOf(RemoteConfigProcessorProvider())
kspWithCompilation = true
inheritClassPath = true
verbose = true
messageOutputStream = System.out
configureKsp(useKsp2 = false) {
symbolProcessorProviders += RemoteConfigProcessorProvider()
withCompilation = true
languageVersion = "1.9"
Copy link
Contributor

Choose a reason for hiding this comment

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

Additional reference:

To use KSP, you must set KotlinCompilation.languageVersion to 1.9.

ZacSweers/kotlin-compile-testing#196 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, nice find @wzieba ! 🥇

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, I have no idea why you "must" set KotlinCompilation.languageVersion to 1.9 for this to work, if you know, please share some more details, since this looks weird to me, and I am not sure that I am missing something basic here... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIU this is because KSP1 was meant to support language features up Kotlin 2.0+ (not included). So if -language-version compiler option (this is what KotlinCompilation.languageVersion sets) is set to higher, I'm guessing Kotlin Compiler forcefully chooses KSP2, which doesn't work with out processor. This is just a theory though, understanding it fully would diving into source of this logic or asking the author. Or find a way to instruct Kotlin compiler to print out which KSP it uses, but I haven't found a way.

Copy link
Contributor

Choose a reason for hiding this comment

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

ZacSweers/kotlin-compile-testing#309 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for sharing this theory @wzieba , it makes (some) sense indeed... 👍 + 🤔

Copy link
Contributor

@ParaskP7 ParaskP7 Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ZacSweers/kotlin-compile-testing#309 🙂

Awesome, thanks for that, upvoted/subscribed! 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

We got the answer, I think it's aligned with what I described above 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, 100%, great to have clarity and verification on that! 💯

}
}.compile()

// Fake AppConfig is needed, as it's a class that is expected to be present in the classpath. Originally, this class
Expand Down
Loading