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

Kotlin version configured from GitHub Actions workflow is not correctly applied #1048

Closed
SimonMarquis opened this issue Feb 10, 2023 · 7 comments · Fixed by #1056 or #1154
Closed

Kotlin version configured from GitHub Actions workflow is not correctly applied #1048

SimonMarquis opened this issue Feb 10, 2023 · 7 comments · Fixed by #1056 or #1154

Comments

@SimonMarquis
Copy link
Contributor

CI uses the following matrix to test Kotlin versions:

kotlin-version: [ 1.5.31, 1.6.21, 1.7.22, 1.8.0 ]

and propagate them as Gradle project properties:

-Pkotlin.version=${{ matrix.kotlin-version }}

MockK uses this property here:

fun Project.kotlinVersion() = findProperty("kotlin.version")?.toString() ?: Deps.Versions.kotlinDefault

but this method is unused in this project.
And therefore I suspect all jobs are using the same Kotlin version.


Also, I tried to run the same command as in the workflow but with an invalid Kotlin version, and it still ran the tests sucessfully:

gradlew check --stacktrace -Pkotlin.version=0.0.0 -Pkotlin.ir.enabled=true -PjavaToolchainTestVersion=17
@SimonMarquis
Copy link
Contributor Author

kotlin.ir.enabled seems to be no longer used as well.

@aSemy
Copy link
Contributor

aSemy commented Feb 19, 2023

Good spot! Removing kotlin.ir.enabled would really help with CI as it would half the number of tests.

The dependencies versions handling was never tidied up after the big refactoring in #855

I've made a PR - let's see if the tests pass...

@SimonMarquis
Copy link
Contributor Author

Kotlin-IR will be removed in #1153, but the main issue persist: Project.kotlinVersion() is never used.

@SimonMarquis
Copy link
Contributor Author

@Raibaz is this really meant to be used on CI? I'm wondering what was this trying to achieve as we can only ship a single artifact, targeting a single Kotlin version 🤔

@Raibaz
Copy link
Collaborator

Raibaz commented Sep 12, 2023

Yep it was meant to be used on CI to make sure master was building fine and tests were passing against several different Kotlin versions; specifically the tests in clientTests, which depend on MockK as if they were a client project.

It is true we ship a single artifact targeting a single Kotlin version, but we also want to ensure backwards compatibility with older versions, and that was what I was trying to ensure on CI.

Usage of Project.kotlinVersion() must have been mistakenly dropped when we refactored gradle files to .kts, as @aSemy mentioned, see here.

This being said, we can probably live without kotlin.ir.enabled since the IR is now enabled by default on all latest versions, but I'd still like CI to build against at least a few different versions (which is not happening now)

@SimonMarquis
Copy link
Contributor Author

Understood. Then maybe we should simply take advantage of the Kotlin BoM and only provide the version once. I'll try to come up with something unless you have a specific solution in mind.

@Raibaz
Copy link
Collaborator

Raibaz commented Sep 12, 2023

I don't have a specific solution in mind, so any help on this (as on any other topic) is greatly appreciated <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants