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.

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

Change the order of atomicfu and kotlin plugins in the classpath #3984

Merged
merged 3 commits into from
Dec 14, 2023

Conversation

mvicsokolova
Copy link
Contributor

@mvicsokolova mvicsokolova commented Dec 12, 2023

A rather obscure problem was revealed while compiling coroutines against the current develop atomicfu branch before 0.23.2 release.

Reproducer:
kotlinx-atomicfu:
develop branch: clean build publishToMavenLocal
kotlinx.coroutines:
obscure-atomicfu-application-order branch: :kotlinx-coroutines-core:compileKotlinJvm -Patomicfu_version=0.23.1-SNAPSHOT
Reproduced with Kotlin 1.9.20, 1.9.21 and 2.0.0-dev-*

e: Module kotlinx.atomicfu cannot be found in the module graph

Note that this error is thrown before atomicfu-gradle-plugin is applied.

The error started to reproduce after these changes in kotlinx-atomicfu#377: (atomicfu compiler plugin should be manually added to the classpath by the user).

Changing the order of kotlin-gradle-plugin and atomicfu-gradle-plugin in the classpath fixes the problem and the train build is now green.

What changes is the compile classspath:
Before the fix atomicfu-jvm is not included in the classpath:

:kotlinx-coroutines-core:compileKotlinJvm Kotlin compiler args: -Xallow-no-source-files -classpath /Users/Maria.Sokolova/.m2/repository/org/jetbrains/kotlin/kotlin-stdlib/1.9.21/kotlin-stdlib-1.9.21.jar:.......

After changing the order atomicfu-jvm is in the classpath:

:kotlinx-coroutines-core:compileKotlinJvm Kotlin compiler args: -Xallow-no-source-files -classpath /Users/Maria.Sokolova/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib/1.9.20/e58b4816ac517e9cc5df1db051120c63d4cde669/kotlin-stdlib-1.9.20.jar.....:/Users/Maria.Sokolova/.m2/repository/org/jetbrains/kotlinx/atomicfu-jvm/0.23.2-SNAPSHOT/atomicfu-jvm-0.23.2-SNAPSHOT.jar

…ugin to the classpath.

 in the global buildscript: when atomicfu-gradle-plugin is not added to the classpath first, it is not applied at all.
 When kotlin-gradle-plugin is added first, atomicfu-gradle-plugin is not applied at all and this error is thrown: "e: Module kotlinx.atomicfu cannot be found in the module graph".

This behaviour started to reproduce after this change in kotlinx-atomicfu (Kotlin/kotlinx-atomicfu#377).

The problem was revealed while running a train build against kotlin-community/dev branches before kotlinx-atomicfu 0.23.2.
@@ -57,9 +57,9 @@ buildscript {
}

dependencies {
classpath "org.jetbrains.kotlinx:atomicfu-gradle-plugin:$atomicfu_version"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add a comment that this line is exactly here for a reason. Otherwise, I promise to forget about this PR when I start reordering all dependencies in the reverse alphabetical order for fun.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@mvicsokolova
Copy link
Contributor Author

mvicsokolova commented Dec 13, 2023

Maybe we can merge this change now, and investigate the actual reason of this behaviour as a separate task 👀 ?
Kotlin/kotlinx-atomicfu#384

@mvicsokolova
Copy link
Contributor Author

I'm going to investigate the root of this problem, most probably it will require the fix on atomicfu side, so we can postpone this change for now

@dkhalanskyjb
Copy link
Collaborator

If this change is invisible to the end user of coroutines (is it?) and a proper fix can't be added quickly (can it?), why not merge this workaround so that the CI is not needlessly red?

The only thing missing now is that it can be unclear when we can swap the dependencies again. I think the comment needs a clear link to the issue in atomicfu. When the issue is closed, the outcome should be specified there, like: "Now the order is no longer important" or "We documented why the order will always be like that," so anyone can understand what's happening and why.

@mvicsokolova
Copy link
Contributor Author

This fix invisible for users and it's true that it blocks the CI build.

The reason I wanted to postpone merging is because, the initial purpose of this commit was to check that the train runs fine with the last changes in atomicfu, and after that I wanted to release atomicfu. But now I can not release it without the proper fix to this problem anyway, because it may be reproduced on user projects as well. Though providing this fix may take time, because the reason is not clear yet.

I've created the issue in atomicfu: Kotlin/kotlinx-atomicfu#384

@dkhalanskyjb
Copy link
Collaborator

I've created the issue in atomicfu

Yeah, I saw that, I mean that build.gradle should also link to it.

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

I think it's good to go. @mvicsokolova, should we merge this?

@mvicsokolova
Copy link
Contributor Author

Yes, I'll merge it

@mvicsokolova mvicsokolova merged commit 605ec56 into develop Dec 14, 2023
1 check passed
@mvicsokolova mvicsokolova deleted the obscure-atomicfu-application-order branch December 14, 2023 09:49
woainikk pushed a commit that referenced this pull request Jan 4, 2024
This commit changes the order of adding atomicfu and kotlin gradle plugin to the classpath.

When `kotlin-gradle-plugin` is added first, `atomicfu-gradle-plugin` is not applied at all and this error is thrown: "e: Module kotlinx.atomicfu cannot be found in the module graph". The change of order fixes the issue. Should be properly investigated.

This behaviour started to reproduce after this change in kotlinx-atomicfu (Kotlin/kotlinx-atomicfu#377).

Here is the corresponding issue in atomicfu: Kotlin/kotlinx-atomicfu#384

(cherry picked from commit 605ec56)
woainikk pushed a commit that referenced this pull request Jan 4, 2024
This commit changes the order of adding atomicfu and kotlin gradle plugin to the classpath.

When `kotlin-gradle-plugin` is added first, `atomicfu-gradle-plugin` is not applied at all and this error is thrown: "e: Module kotlinx.atomicfu cannot be found in the module graph". The change of order fixes the issue. Should be properly investigated.

This behaviour started to reproduce after this change in kotlinx-atomicfu (Kotlin/kotlinx-atomicfu#377).

Here is the corresponding issue in atomicfu: Kotlin/kotlinx-atomicfu#384

(cherry picked from commit 605ec56)
woainikk pushed a commit that referenced this pull request Jan 9, 2024
This commit changes the order of adding atomicfu and kotlin gradle plugin to the classpath.

When `kotlin-gradle-plugin` is added first, `atomicfu-gradle-plugin` is not applied at all and this error is thrown: "e: Module kotlinx.atomicfu cannot be found in the module graph". The change of order fixes the issue. Should be properly investigated.

This behaviour started to reproduce after this change in kotlinx-atomicfu (Kotlin/kotlinx-atomicfu#377).

Here is the corresponding issue in atomicfu: Kotlin/kotlinx-atomicfu#384

(cherry picked from commit 605ec56)
woainikk pushed a commit that referenced this pull request Jan 9, 2024
This commit changes the order of adding atomicfu and kotlin gradle plugin to the classpath.

When `kotlin-gradle-plugin` is added first, `atomicfu-gradle-plugin` is not applied at all and this error is thrown: "e: Module kotlinx.atomicfu cannot be found in the module graph". The change of order fixes the issue. Should be properly investigated.

This behaviour started to reproduce after this change in kotlinx-atomicfu (Kotlin/kotlinx-atomicfu#377).

Here is the corresponding issue in atomicfu: Kotlin/kotlinx-atomicfu#384

(cherry picked from commit 605ec56)
woainikk pushed a commit that referenced this pull request Jan 18, 2024
This commit changes the order of adding atomicfu and kotlin gradle plugin to the classpath.

When `kotlin-gradle-plugin` is added first, `atomicfu-gradle-plugin` is not applied at all and this error is thrown: "e: Module kotlinx.atomicfu cannot be found in the module graph". The change of order fixes the issue. Should be properly investigated.

This behaviour started to reproduce after this change in kotlinx-atomicfu (Kotlin/kotlinx-atomicfu#377).

Here is the corresponding issue in atomicfu: Kotlin/kotlinx-atomicfu#384

(cherry picked from commit 605ec56)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants