-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Support protoc Kotlin code generation #44552
Conversation
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This looks promising, thanks for the PR.
I suggested some minor doc changes and more importantly it seems some Gradle tests are failing and I think that's the important bit:
2024-11-17T17:13:44.2059374Z Options for KOTLIN DAEMON: IncrementalCompilationOptions(super=CompilationOptions(compilerMode=INCREMENTAL_COMPILER, targetPlatform=JVM, reportCategories=[0, 3], reportSeverity=2, requestedCompilationResults=[0], kotlinScriptExtensions=[]), areFileChangesKnown=false, modifiedFiles=null, deletedFiles=null, classpathChanges=NotAvailableForNonIncrementalRun, workingDir=/home/runner/work/quarkus/quarkus/integration-tests/gradle/target/kotlin-grpc-project12669661681070306965/build/kotlin/compileKotlin/cacheable, multiModuleICSettings=MultiModuleICSettings(buildHistoryFile=/home/runner/work/quarkus/quarkus/integration-tests/gradle/target/kotlin-grpc-project12669661681070306965/build/kotlin/compileKotlin/local-state/build-history.bin, useModuleDetection=false), usePreciseJavaTracking=true, icFeatures=IncrementalCompilationFeatures(withAbiSnapshot=false, preciseCompilationResultsBackup=true, keepIncrementalCompilationCachesInMemory=true, enableUnsafeIncrementalCompilationForMultiplatform=false), outputFiles=[/home/runner/work/quarkus/quarkus/integration-tests/gradle/target/kotlin-grpc-project12669661681070306965/build/classes/kotlin/main, /home/runner/work/quarkus/quarkus/integration-tests/gradle/target/kotlin-grpc-project12669661681070306965/build/kotlin/compileKotlin/cacheable, /home/runner/work/quarkus/quarkus/integration-tests/gradle/target/kotlin-grpc-project12669661681070306965/build/kotlin/compileKotlin/local-state])
2024-11-17T17:13:44.2061194Z e: file:///home/runner/work/quarkus/quarkus/integration-tests/gradle/target/kotlin-grpc-project12669661681070306965/build/classes/java/quarkus-generated-sources/grpc/io/quarkus/example/HelloMsgKt.kt:15:17 Annotation argument must be a compile-time constant.
2024-11-17T17:13:44.2063191Z e: file:///home/runner/work/quarkus/quarkus/integration-tests/gradle/target/kotlin-grpc-project12669661681070306965/build/classes/java/quarkus-generated-sources/grpc/io/quarkus/example/HelloMsgKt.kt:15:37 Unresolved reference 'kotlin'.
2024-11-17T17:13:44.2065030Z e: file:///home/runner/work/quarkus/quarkus/integration-tests/gradle/target/kotlin-grpc-project12669661681070306965/build/classes/java/quarkus-generated-sources/grpc/io/quarkus/example/HelloMsgKt.kt:16:24 Unresolved reference 'kotlin'.
2024-11-17T17:13:44.2065363Z Finished executing kotlin compiler using DAEMON strategy
@cdsap I'm not sure if it's related to this change but I have seen the following in the logs and it doesn't look like something we would want:
|
@gsmet I tested with a version without the configuration cache changes (3.15.1), I'm observing the same log: Note that I'm executing the build like: Will confirm with the team |
3227bb7
to
8c47257
Compare
Thanks for the feedback. I am aware of the failing tests, as I described in the PR description I am not sure what the problem is since I dont have much experienced with mixed java/kotlin generated sources. Do the generated kotlin sources have to be in another folder than the generated java code? (see my original PR description) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Any feedback on this, @geoand, @alesj, @cescoffier? 🙂 |
The PR looks good, but the Gradle issue is problematic. |
@PhilKes did you look at the gradle issue? |
@cescoffier I looked at it, the generated // Generated by the protocol buffer compiler. DO NOT EDIT!
// source: hello.proto
//...
public object HelloMsgKt {
@kotlin.OptIn(com.google.protobuf.kotlin.OnlyForUseByGeneratedProtoCode::class)
@com.google.protobuf.kotlin.ProtoDslMarker
public class Dsl private constructor(
//... Now I am not sure how to proceed, should I add a check for the |
Does it need to be available at runtime or only at build time? If it's only build time, we can "just" document that when using Gradle this dependency is necessary. |
8c47257
to
f10e6d2
Compare
This comment has been minimized.
This comment has been minimized.
f10e6d2
to
ab286fd
Compare
This comment has been minimized.
This comment has been minimized.
Indeed it's only needed at build-time so I added it as a |
This comment has been minimized.
This comment has been minimized.
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.
Just a minor comment on the doc
ab286fd
to
45c6907
Compare
This comment has been minimized.
This comment has been minimized.
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.
Thanks!
Let's see what the CI says. |
45c6907
to
4c047a3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@cescoffier looks like there is something wrong with the build pipeline?
|
Hum, no, on Windows we need to use the file approach to workaround that issue. So, arguments are written in a file and passed as @file if I remember correctly |
4c047a3
to
9199d38
Compare
Status for workflow
|
Status for workflow
|
Well the build succeeded now after an update via ebase |
Thanks! |
Hello. I think this is the culprit of what we're seeing in #23612 (comment) In the superheroes sample application, the Setting I've created #46675 to track this. |
Closes #39127
This PR adds support to generate Kotlin DSLs/Extensions for protoc via adding the
kotlin_out
parameter to theprotoc
command:io.quarkus:quarkus-kotlin
is presentquarkus.generate-code.grpc.kotlin.generate
propertyTo test it I reused the
kotlin-grpc-project
integration-test to use the generated DSL forHelloMsg
.Now currently when running the
KotlinGRPCProjectBuildTest
,compileKotlin
fails with:HelloMsgKt.kt
I am guessing this is because the generated kotlin code should not be in
build/classes/java
but ratherbuild/classes/kotlin
?But if we provide a different output path for the
kotlin_out
than forjava_out
we would need a way to add this extra path also to the compile sources, which is done inCodeGenerator.init()
, right?