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

Upgrade to Kotlin 1.7 #26772

Conversation

GavinRay97
Copy link
Contributor

Seems to work, tested with a basic app from Quarkus codestarts using local 999-SNAPSHOT with Kotlin 1.7.10

Code_-_Insiders_WgelFAjM9L.mp4

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle labels Jul 18, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 18, 2022

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should not end up with dot

This message is automatically generated by a bot.

@GavinRay97 GavinRay97 changed the title Actually make Kotlin 1.7 work, for really-reals. Actually make Kotlin 1.7 work, for really-reals Jul 18, 2022
@famod
Copy link
Member

famod commented Jul 18, 2022

@GavinRay97 cool, thanks - I've pinged the ones who are the experts for this matter.

Can you please resolve the conflict? Thanks!

@gsmet @geoand probably too late for 2.11?

@gsmet gsmet force-pushed the feature/kotlin-1.7.0-with-gradle-api-change branch from c43dc32 to 5a30b1b Compare July 18, 2022 10:49
@gsmet
Copy link
Member

gsmet commented Jul 18, 2022

I force pushed a rebase to fix a conflict.

@gsmet
Copy link
Member

gsmet commented Jul 18, 2022

@famod if people think there is value and we can get this in by tonight, I could be convinced to include it in 2.11.

@@ -461,6 +463,32 @@ private static void initProjectModule(Project project, WorkspaceModule.Mutable m
});
});

project.getTasks().withType(KotlinJvmCompile.class, t -> {
Copy link
Member

Choose a reason for hiding this comment

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

@glefloch could you have a look at this change?

@famod famod linked an issue Jul 18, 2022 that may be closed by this pull request
@famod
Copy link
Member

famod commented Jul 18, 2022

Btw, this should fix #26290. Would be nice if anyone can confirm.

Copy link
Member

@evanchooly evanchooly left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me and reflects what I'd been investigating as well. It should the CLI bug as well. That's the angle that I took when starting my own investigations.

@evanchooly
Copy link
Member

The thread I was gonna pull on today was the possibility that the new compiler plugging uses a different source set and that's why they weren't showing up. Might save us the scanning time.

@quarkus-bot

This comment has been minimized.

@GavinRay97
Copy link
Contributor Author

GavinRay97 commented Jul 18, 2022

Okay so the Gradle tests are failing because I forgot to add the Kotlin Gradle library to whichever Maven POM needs it:

  • Caused by: java.lang.NoClassDefFoundError: org/jetbrains/kotlin/gradle/tasks/KotlinJvmCompile

That makes sense

What's strange through is that ./mvnw -Dquickly has all these IT's shown as passed for me locally 🤔

image

@famod
Copy link
Member

famod commented Jul 18, 2022

quickly doesn't run any ITs at all which is why it's comparably fast. 😉

@GavinRay97
Copy link
Contributor Author

GavinRay97 commented Jul 18, 2022

LOL that'd make sense then I suppose 😂

Do you know which Maven POM I would need to add the kotlin-gradle-model library to by chance?

Copy link
Member

@evanchooly evanchooly left a comment

Choose a reason for hiding this comment

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

a bit more testing then

Copy link
Member

@glefloch glefloch left a comment

Choose a reason for hiding this comment

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

Thanks for this @GavinRay97. I think you can add kotlin-gradle-model dependency to the gradle-model module.

@GavinRay97
Copy link
Contributor Author

GavinRay97 commented Jul 20, 2022

I tried to get this working last night -- I added the kotlin-gradle-plugin-model to both:

  • devtools/gradle/gradle-model/pom.xml
  • devtools/gradle/gradle-application-plugin/pom.xml

But I still get:

Execution failed for task ':quarkusGenerateCode'.
> ClassNotFound: org/jetbrains/kotlin/gradle/tasks/KotlinJvmCompile 

My guess is maybe it's actually something with the Gradle dependencies and not Maven
Should I declare the Gradle library as api instead of implementation, or add it to a higher-level package than the one it's in? 🤔

@GavinRay97
Copy link
Contributor Author

I'm a bit stumped on this -- I've tried adding the dependency to every pom.xml and build.gradle file in all subprojects under gradle:

image

It still fails? 🤔

image

@GavinRay97
Copy link
Contributor Author

Who would be the right person to ask about this?

I think the error is a red herring, if you apply the changes and create a new project, Quarkus works great.
So this must be something about the way that the tests are run

Maybe some kind of Gradle CLI invocation with a specific classpath and build.gradle set?

@geoand
Copy link
Contributor

geoand commented Jul 28, 2022

@gastaldi @ia3andy any ideas?

@GavinRay97
Copy link
Contributor Author

GavinRay97 commented Jul 28, 2022

@ebullient @glefloch Apologies for the ping -- git blame shows you folks as primary contributors to:

  • devtools/cli/src/test/java/io/quarkus/cli/CliProjectGradleTest.java

Would either one of you be able to explain to me how the classpath + libraries are set for these tests?
It doesn't seem to come from the build.gradle or pom.xml of any project in quarkus-devtools-all

At this point, I started modifying even random project dependencies to try to get it to work:

image

When the tests for CliProjectGradleTest.java are run, I can SEE that kotlin-gradle-model-api is on the classpath, it's part of the java command...

image

So I'm really confused to what's happening here and running out of ideas =/
I'd love to get this merged if possible

Thank you for your patience 🙏

@evanchooly
Copy link
Member

one simple formatting fix and I think we're good to go @GavinRay97

@GavinRay97
Copy link
Contributor Author

GavinRay97 commented Aug 1, 2022

Just clocked out for the day, will submit the formatted commit and try to figure out the squash again 👍

EDIT: For some reason when I do get rebase -i main, I only see a single commit:

image

@GavinRay97 GavinRay97 force-pushed the feature/kotlin-1.7.0-with-gradle-api-change branch from 568f639 to dd1b396 Compare August 1, 2022 23:33
@GavinRay97
Copy link
Contributor Author

GavinRay97 commented Aug 1, 2022

Oh my god I just un-did some of my previous commits somehow
Now it's broken and it won't build locally

image

image

@geoand
Copy link
Contributor

geoand commented Aug 2, 2022

Oh my god I just un-did some of my previous commits somehow

Git reflog to the rescue!

@GavinRay97
Copy link
Contributor Author

Me staring at Git

image

@GavinRay97
Copy link
Contributor Author

Okay, officially requesting review again, whenever someone has time

The easiest way to tell whether this is working is probably to do something like this:

@Path("/hello")
class GreetingResource {

    @GET
    @Produces(MediaType.TEXT_PLAIN)
    fun hello() = "Hello from RESTEasy Reactive, running on Kotlin v${KotlinVersion.CURRENT}"
}

image

@gastaldi gastaldi force-pushed the feature/kotlin-1.7.0-with-gradle-api-change branch from a579dcf to c5d5eaf Compare August 2, 2022 18:34
@gastaldi gastaldi force-pushed the feature/kotlin-1.7.0-with-gradle-api-change branch from c5d5eaf to 931f7cc Compare August 2, 2022 18:34
@quarkus-bot quarkus-bot bot added this to To do in Quarkus Documentation Aug 2, 2022
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

@GavinRay97 I squashed and force-pushed your changes in a single commit. Here is what I did:

  • git pull to update my main branch with the latest changes
  • hub pr checkout 26772 (Using the Hub CLI)
  • git rebase -i HEAD~4 and marked the extra commits with an f (fixup)
  • git rebase main to rebase with the latest main

Important: Make sure your pull.rebase setting is true before doing that:

git config --global pull.rebase true

@evanchooly
Copy link
Member

looks like heap issues on CI

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 3, 2022

Failing Jobs - Building 931f7cc

Status Name Step Failures Logs Raw logs
Gradle Tests - JDK 11 Build Failures Logs Raw logs
✔️ Gradle Tests - JDK 11 Windows

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.devmode.CompositeBuildWithDependenciesDevModeTest.main line 24 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

@geoand
Copy link
Contributor

geoand commented Aug 3, 2022

CI looks good.

That test actually fails pretty often

@gsmet
Copy link
Member

gsmet commented Aug 9, 2022

@evanchooly I think you were happy with the latest state? Can you approve the PR if it's fine for you? I'd really like to have for 2.12.

@evanchooly
Copy link
Member

Is everyone OK with that test failure?

@gastaldi
Copy link
Contributor

gastaldi commented Aug 9, 2022

That is a flaky test AFAIK

Quarkus Documentation automation moved this from To do to Reviewer approved Aug 9, 2022
@evanchooly evanchooly merged commit 2d47775 into quarkusio:main Aug 9, 2022
Quarkus Documentation automation moved this from Reviewer approved to Done Aug 9, 2022
@quarkus-bot quarkus-bot bot added this to the 2.12 - main milestone Aug 9, 2022
@evanchooly
Copy link
Member

May the gods have mercy on us all.

@gsmet gsmet changed the title Actually make Kotlin 1.7 work, for really-reals Upgrade to Kotlin 1.7 Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/gradle Gradle release/noteworthy-feature
Development

Successfully merging this pull request may close these issues.

Command Mode Applications don't work with Kotlin 1.7
8 participants