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

Jump to bottom

Update Kotlin to 1.9.0. #1081

Merged
merged 6 commits into from
Jul 26, 2023
Merged

Update Kotlin to 1.9.0. #1081

merged 6 commits into from
Jul 26, 2023

Conversation

colinrtwhite
Copy link
Collaborator

https://kotlinlang.org/docs/whatsnew19.html

Did a quick check with the world clock sample and everything seems to be working correctly.

@JakeWharton
Copy link
Member

What caused that? Please tell me it wasn't Hermit...

@colinrtwhite
Copy link
Collaborator Author

@JakeWharton Whenever I run the kotlinUpgradeYarn task it replaces it with our Artifactory references - not sure why. It happens with Redwood and other repos so it's not specific to Zipline.

@colinrtwhite
Copy link
Collaborator Author

colinrtwhite commented Jul 21, 2023 •
edited
Loading

The last failing test is a weird one:

app.cash.zipline.SetTimeoutTest.happyPath[macosX64] FAILED
    kotlin.AssertionError at null:-1

it only fails on Darwin platforms on CI. It passes locally on my M1 machine.

EDIT: Nevermind the test is known to be flaky.

@colinrtwhite
Copy link
Collaborator Author

I think this PR is good to merge once we can also update Redwood.

@JakeWharton
Copy link
Member

Weird failure. Do we need to downgrade the JDK to one that's more tolerant of Kotlin's reflective shenanigans?

@JakeWharton
Copy link
Member

By fixing one flake did I break a bunch of others? I can fix those too, I guess...

@colinrtwhite
Copy link
Collaborator Author

colinrtwhite commented Jul 25, 2023 •
edited
Loading

@JakeWharton Looks like your fix on #1083 fixes the remaining failing tests.

@JakeWharton
Copy link
Member

Excellent! The console one was proper broken so I'm surprised the timeout one was the one which failed all the time.

@JakeWharton
Copy link
Member

Let's try whack-a-mole downgrading the JDK version by one until one works, I guess?

@colinrtwhite
Copy link
Collaborator Author

@JakeWharton I think we're already using JDK 11 (via jvmToolchain(11)), though, which is the minimum for AGP.

@JakeWharton
Copy link
Member

JVM toolchains only apply to tasks which fork a JVM, like JavaCompile or Test. The JVM in which Gradle itself is running is whatever is in JAVA_HOME which looks to be 20. I don't see any YouTrack issues about it, though, which is weird. Let me pull and look locally.

@JakeWharton
Copy link
Member

I cannot reproduce locally. Also, I misread the underlying problem it's actually this:

 Execution failed for task ':zipline:commonizeCInterop'.
> java.io.FileNotFoundException: /Users/runner/work/zipline/zipline/zipline/build/kotlinTransformedCInteropMetadataLibraries/nativeMain/.zipline-nativeMain.cinteropLibraries (No such file or directory)

@colinrtwhite
Copy link
Collaborator Author

colinrtwhite commented Jul 26, 2023 •
edited
Loading

Hmm JDK 17 also fails. I'll try JDK 11. Could it also be a cklib issue given it's related to cinteropLibraries?

EDIT: Actually AGP now requires JDK 17 so that won't work.

EDIT2: Seems like this might work: Kotlin/dokka#2977 (comment). It did!

@JakeWharton
Copy link
Member

Yeah the JDK issue was a red herring caused by me reading the failure on mobile.

Nice find on the Dokka thing, though. Also, Dokka 🤦‍♂️

JakeWharton
JakeWharton approved these changes Jul 26, 2023
View reviewed changes
Copy link
Member

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

Let's do this!

@colinrtwhite colinrtwhite merged commit d42cba9 into trunk Jul 26, 2023
6 checks passed
@colinrtwhite colinrtwhite deleted the colin/kotlin_1_9 branch July 26, 2023 17:55
@IgnatBeresnev
Copy link

Also, Dokka 🤦‍♂️

we're not doing it on purpose, I swear 🥲

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakeWharton JakeWharton JakeWharton approved these changes

@swankjesse swankjesse Awaiting requested review from swankjesse

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@colinrtwhite @JakeWharton @IgnatBeresnev