-
Notifications
You must be signed in to change notification settings - Fork 81
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
Provide dumb klibs for annotation and collection modules with dependencies on androidx artifacts #1819
base: jb-main
Are you sure you want to change the base?
Conversation
…ncies on androidx artifacts
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.
It sounds like we cannot ever drop any of the dependencies from the publishing. It shouldn't be the case - we're planning to remove some libs in a few other places.
Needs to be discussed before moving forward
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.
Can we keep commonMain in the same state as in AOSP?
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.
Why would you like to keep commonMain the same? I believe we don't need to synchronize the source code of these modules anymore.
If we keep the source code, then the klib will include the symbols and the klib won't be "dumb" anymore. It will lead to symbols duplication, which is solved by keeping the klib's source code dumb/empty.
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.
It sounds like we cannot ever drop any of the dependencies from the publishing.
It will be possible after https://youtrack.jetbrains.com/issue/KT-61096/KLIB-Resolve-Drop-KLIB-resolve-inside-the-Kotlin-compiler is solved.
Not immediately though, because we will want to support the previously published klibs.
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.
Why would you like to keep commonMain the same
We aim to restore the state of all files in the fork that we don't maintain. It reduces merge/build issues.
We have 3 options:
- delete all classes from
commonMain
, exclude the folder from merge inscripts/mergeAOSP.sh
- somehow remove classes from publication on the
build.gradle
level (rename sourcesets?) - create additional
collection-compatibility-stub
, etc with the same group, excludecollection
fromsettings.gradle
I vote for the latest - seems easy, and doesn't add issues.
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.
Implemented the latest option.
collection/collection/build.gradle
Outdated
implementation(libs.kotlinTest) | ||
implementation(libs.kotlinTestAnnotationsCommon) | ||
implementation(libs.kotlinCoroutinesCore) | ||
api("androidx.collection:collection:1.5.0-beta02") |
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.
Please use a version from catalog
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.
It is a pinned version, and we currently don't have pinned versions in version catalogs.
I don't mind to have them there, but we have to explicitly decide to do that as the general approach.
If you talk about libraryversions.toml
- we can't use it, can elaborate more if needed.
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.
Also, when we depend on not a Stable version, we should create a task to replace it by Stable with Target version: Compose 1.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.
compose/runtime/runtime/build.gradle
Outdated
implementation(project(":annotation:annotation")) | ||
implementation(project(":collection:collection")) | ||
implementation(project(":performance:performance-annotation")) | ||
implementation("androidx.annotation:annotation:1.9.1") | ||
implementation("androidx.collection:collection:1.5.0-beta02") |
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.
- Use a version from catalog file
- Can we use Google version for all modules?
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.
Can we use Google version for all modules?
I think yes. We don't need to use a project dependency in all other modules.
Compose runtime is unavoidable dependency of any Compose project.
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 mean I still don't fully understand why we should continue to publish it in case of removing all dependencies to it
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 still don't fully understand why we should continue to publish it in case of removing all dependencies to it
There are many published 3rd party klibs which depend on compose-runtime 1.7.x and older.
Those 3rd party klibs contain a list of all dependencies (including transitive dependencies). The list contains the unique names of the dependencies-klibs, including org.jetbrains.compose.collection-internal
that we were publishing.
When Kotlin consumes one klib, it expects that all the dependencies are supplied (paths to those dependencies-klibs).
If we stop publishing our dumb klib with unique name org.jetbrains.compose.collection-internal
, then no klib path would be provided and Kotlin would report an error about missing klib.
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.
Does this mean that we just need to publish only one no-op version and then remove it from publishing?
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.
Those 3rd party klibs contain a list of all dependencies (including transitive dependencies). The list contains the unique names of the dependencies-klibs, including org.jetbrains.compose.collection-internal that we were publishing.
So, how are we going to drop experimental dependencies in the future? Like ui-backhandler
that is supposed to be replaced by Google one once it will be there
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.
Does this mean that we just need to publish only one no-op version and then remove it from publishing?
Not that simple unfortunately.
In order to be provided to Kotlin compiler, the dependency must be present in the dependency graph (configured by gradle).
Yes, we can stop publishing the dumb klib. But we will need to pin the dependency on the previously published dumb klib.
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.
So, how are we going to drop experimental dependencies in the future? Like ui-backhandler that is supposed to be replaced by Google one once it will be there
We'll need the "dumb klib" approach too. The published dumb klib version can be used as a pinned dependency in one of our modules, so it will be present in the dependencies graph.
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.
It will be possible after https://youtrack.jetbrains.com/issue/KT-61096/KLIB-Resolve-Drop-KLIB-resolve-inside-the-Kotlin-compiler is solved.
As we discussed privately, this won't solve the issue. We should publish it until we decide to drop backward compatibility.
A case explaining the issue:
App -> Compose 1.8 -> android.compose.annotation 1.5
App -> SomeLib -> Compose 1.7 -> org.jetbrains.compose.annotation-internal 1.7
Without a dumb lib:
App -> Compose 1.8 -> org.jetbrains.compose.annotation-internal 1.8
It will always lead to duplicated classes
Yes, we can stop publishing the dumb klib.
We also discussed it. If we stop publishing klib in Compose 1.9, for example, Compose 1.9 again won't be able to consume libs dependent on Compose 1.7
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.
What Ivan and I mean is that we can stop publishing the dumb klib and add a module dependency on the dumb klib, while also adding a module dependency on the latest androidx.annotation. So the dependency graph would look like this:
org.jetbrains.compose.runtime:1.9 // future version
| -> org.jetbrains.compose.annotation-internal:1.8.0 // dumb klib. The version we'll publish with CM 1.8.0.
| -> androidx.annotation:1.9.1
| -> androidx.annotation:2.0 // 2.0 is just an example of future version
This way androidx.annotation:2.0 would be chosen.
Right?
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.
Why would you like to keep commonMain the same
We aim to restore the state of all files in the fork that we don't maintain. It reduces merge/build issues.
We have 3 options:
- delete all classes from
commonMain
, exclude the folder from merge inscripts/mergeAOSP.sh
- somehow remove classes from publication on the
build.gradle
level (rename sourcesets?) - create additional
collection-compatibility-stub
, etc with the same group, excludecollection
fromsettings.gradle
I vote for the latest - seems easy, and doesn't add issues.
collection/collection/build.gradle
Outdated
implementation(libs.kotlinTest) | ||
implementation(libs.kotlinTestAnnotationsCommon) | ||
implementation(libs.kotlinCoroutinesCore) | ||
api("androidx.collection:collection:1.5.0-beta02") |
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.
compose/runtime/runtime/build.gradle
Outdated
implementation(project(":annotation:annotation")) | ||
implementation(project(":collection:collection")) | ||
implementation(project(":performance:performance-annotation")) | ||
implementation("androidx.annotation:annotation:1.9.1") | ||
implementation("androidx.collection:collection:1.5.0-beta02") |
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.
It will be possible after https://youtrack.jetbrains.com/issue/KT-61096/KLIB-Resolve-Drop-KLIB-resolve-inside-the-Kotlin-compiler is solved.
As we discussed privately, this won't solve the issue. We should publish it until we decide to drop backward compatibility.
A case explaining the issue:
App -> Compose 1.8 -> android.compose.annotation 1.5
App -> SomeLib -> Compose 1.7 -> org.jetbrains.compose.annotation-internal 1.7
Without a dumb lib:
App -> Compose 1.8 -> org.jetbrains.compose.annotation-internal 1.8
It will always lead to duplicated classes
Yes, we can stop publishing the dumb klib.
We also discussed it. If we stop publishing klib in Compose 1.9, for example, Compose 1.9 again won't be able to consume libs dependent on Compose 1.7
compose/runtime/runtime/build.gradle
Outdated
@@ -116,9 +116,16 @@ if(AndroidXComposePlugin.isMultiplatformEnabled(project)) { | |||
commonMain.dependencies { | |||
implementation(libs.kotlinStdlibCommon) | |||
implementation(libs.kotlinCoroutinesCore) | |||
implementation(project(":performance:performance-annotation")) | |||
|
|||
// Keep both project and module dependency for pseudo-relocated modules. |
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.
It looks like we don't have to add dependencies on androidx
artifacts until we remove the project dependency?
- Every project now depends on our dumb artifact
- Every project depends on
androidx
artifact via transitive dependency - When we decide to remove the dumb artifact (breaking binary compatibility for old versions), we just replace the project dependency by
androidx
.
P.S. If something in my logic is wrong, and we still have to do it, we have to do it everywhere, not only in compose:runtime
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.
Currently we don't need to add a module dependency on "androidx.collection/annotation:version".
It will be added transitively thanks to the dependecy on the stub project/module.
So I removed it for now.
But when we decide to stop publishing the stub, we'll need to add an explicit version dependency.
To avoid breaking compilation compatibility with already published klibs depending on our klibs, we can't stop publishing an artifact with old coordinates:
org.jetbrains.compose.annotation-internal
andorg.jetbrains.compose.collection-internal
.Here is why: https://youtrack.jetbrains.com/issue/KT-61096/KLIB-Resolve-Drop-KLIB-resolve-inside-the-Kotlin-compiler
TL;DR: All klibs have an explicit dependencies list. Kotlin expects that those klibs (with expected names) will be provided for compilation. If some klibs have
org.jetbrains.compose.annotation-internal
in that list, then the compilation will crash if there is no klib with such a name provided to compilation.But the actual content in the klibs doesn't matter.
Therefore we keep publishing empty klibs. And add a dependency on androidx artifacts.
A bit of context:
Initially, we thought we can simply publish a .pom file with relocation info - basically a redirecting link. It works - gradle correctly resolves the dependencies and chooses the androidx artifact.
But Kotlin klibs resolver doesn't care about artifact relocations or substitutions (in gradle/maven perspective), Kotlin doesn't know about them.
The publication structure didn't change
The difference is that the .klib files are smaller now: 3-4 Kbytes.
The gradle metadata contains the dependency:
And for example compose runtime depends on both old and new coordinates: