[REGRESSION] - grpc kotlin code generation breaking other things

[edeandrea]

Describe the bug

It seems that #44552 has broken some things. The superheroes CI has been broken since March 2 (see #23612 (comment) for the details).

The grpc-locations application uses grpc and is written in Kotlin. After this change the application no longer compiles.

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

Steps to reproduce:

1. Build Quarkus main locally
2. Clone https://github.com/quarkusio/quarkus-super-heroes
3. cd grpc-locations
4. ./mvnw clean verify -Dquarkus.platform.group-id=io.quarkus -Dquarkus.platform.version=999-SNAPSHOT

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

[quarkus-bot]

/cc @alesj (grpc), @cescoffier (grpc), @geoand (kotlin)

[edeandrea]

@PhilKes ^^^

[PhilKes]

I guess this was to be expected, since the newly generated Kotlin GRPC code requires com.google.protobuf:protobuf-kotlin as a compile-time dependency.
I discussed how to handle this in #44552 (comment) with @cescoffier, and added the corresponding hint in the docs:

quarkus/docs/src/main/asciidoc/grpc-generation-reference.adoc

Line 177 in 43c95ab

IMPORTANT: When using Gradle, you also have to include `com.google.protobuf:protobuf-kotlin` as a `compileOnly` dependency in your project.

I wasn't aware of the Quarkus Super Heroes project unfortunately, otherwise I would have opened an issue there
So you can either disable the Kotlin code generation as you suggested in #23612 (comment) or you have to add the protobuf-kotlin compile-time dependency as described in the docs.

We could also add a dependency check for the protobuf-kotlin dependency like I suggested in #44552 (comment), which in your case would lead to the Kotlin code not being generated and therefore the build not failing, without changing any settings.

[edeandrea]

Thanks for the quick response @PhilKes !

I guess this was to be expected, since the newly generated Kotlin GRPC code requires com.google.protobuf:protobuf-kotlin as a compile-time dependency.

This is a breaking change, so we also need to make sure this is documented in the release notes for whichever release this change becomes part of (cc: @gsmet).

Is the com.google.protobuf:protobuf-kotlin dependency version managed by the Quarkus Platform BOM? If not, it probably should be, otherwise users will be guessing at which version they should use.

We could also add a dependency check for the protobuf-kotlin dependency like I suggested in #44552 (comment), which in your case would lead to the Kotlin code not being generated and therefore the build not failing, without changing any settings.

I think this would be better than flat-out breaking people. I think it should be as transparent to the end user as possible, as thats one of the Quarkus core values.

I discussed how to handle this in #44552 (comment) with @cescoffier, and added the corresponding hint in the docs

The hint mentions Gradle, not Maven.

I wasn't aware of the Quarkus Super Heroes project unfortunately

@maxandersen / @cescoffier / @insectengine / @holly-cummins After 3-4 years of the Superheroes being around, how are contributors not aware of it? Its the "PetClinic" of Quarkus and should be used for testing new things and verifying things. This is why we have the nightly ecosystem job in place. We need to do better about getting contributors familiar with it. I'm open to ideas.

[PhilKes]

This is a breaking change, so we also need to make sure this is documented in the release notes for whichever release this change becomes part of (cc: @gsmet).

Is the com.google.protobuf:protobuf-kotlin dependency version managed by the Quarkus Platform BOM? If not, it probably should be, otherwise users will be guessing at which version they should use.

Yes I added the according entry into the bom with the most recent version:

quarkus/bom/application/pom.xml

Lines 6262 to 6267 in 43c95ab

	<dependency>
	<groupId>com.google.protobuf</groupId>
	<artifactId>protobuf-kotlin</artifactId>
	<version>${protobuf-kotlin.version}</version>
	<scope>compile</scope>
	</dependency>

We could also add a dependency check for the protobuf-kotlin dependency like I suggested in #44552 (comment), which in your case would lead to the Kotlin code not being generated and therefore the build not failing, without changing any settings.

I think this would be better than flat-out breaking people. I think it should be as transparent to the end user as possible, as thats one of the Quarkus core values.

I agree, I can a open a PR for it

I discussed how to handle this in #44552 (comment) with @cescoffier, and added the corresponding hint in the docs

The hint mentions Gradle, not Maven.

I'm not 100% sure about that, initially I had the hint not including "When using Gradle", but then @cescoffier wanted me to include it (#44552 (comment)), so I thought there was some reason it wasnt needed for maven, but maybe it was just a misunderstanding. 😅

I wasn't aware of the Quarkus Super Heroes project unfortunately

@maxandersen / @cescoffier / @insectengine / @holly-cummins After 3-4 years of the Superheroes being around, how are contributors not aware of it? Its the "PetClinic" of Quarkus and should be used for testing new things and verifying things. This is why we have the nightly ecosystem job in place. We need to do better about getting contributors familiar with it. I'm open to ideas.

Really sorry about that, I will definitely use it in the future.
I guess I personally would expect this sort of information either as a comment from the Quarkus-Bot in the PR itself or maybe mentioning it in CONTRIBUTING.md#before-you-contribute that you should test your changes with the quarkus-super-heroes project. Without such a "manual" hint I would expect the PR build pipeline to run all necessary tests I guess.

[cescoffier]

I had the impression that the dependency was only required for Gradle. If it is x required for Maven too, it should be declared as dependency.

[gsmet]

Really sorry about that, I will definitely use it in the future

No need to be sorry. Quarkus is a huge project. It’s hard to understand exactly the impact of all of all patches.

That’s why we have all sorts of CI runs. And I really don’t think we should put this burden on the shoulders of occasional contributors.

This suggests that maybe it needs to be a conditional dependency when Kotlin is around.

[edeandrea]

Really sorry about that, I will definitely use it in the future.

No need to be sorry at all! I'm only mentioning it (& tagging a few others) to try to figure out how we can advertise the Superheroes better and get more people familiar with it. I like your suggestion of including it in the contributing guide - I hadn't thought of that. I'm open to other suggestions too!

[PhilKes]

Really sorry about that, I will definitely use it in the future

No need to be sorry. Quarkus is a huge project. It’s hard to understand exactly the impact of all of all patches.

That’s why we have all sorts of CI runs. And I really don’t think we should put this burden on the shoulders of occasional contributors.

This suggests that maybe it needs to be a conditional dependency when Kotlin is around.

So should com.google.protobuf:protobuf-kotlin be declared as a dependency of quarkus-kotlin or quarkus-grpc-codegen?
Or add a check for protobuf-kotlin being present (like containsQuarkusKotlin()), and if its not present the kotlin code generation is simply skipped?

[geoand]

I will have a look at the conditional dependency mechanism that @gsmet mentioned

[geoand]

@cescoffier before actually implementing this, I would like to know what quarkus-grpc-stubs is supposed to do.
I am asking because it used quarkus-grpc-codegen as a dependency.

[cescoffier]

grpc stubs contains the utility classes used by the gRPC stub we generate. I'm surprised by the dependency on grpc-codegen as it should be the opposite. The generated code depends on grpc stub, not the opposite. (@alesj do you know why we have that dependency?)

[geoand]

Thanks.

I'll just proceed then as my fix shouldn't affect anything.

[cescoffier]

Ok, now I "remember" .... It's a bit of a mess. gRPC Stub contains a few standard proto files. We want to generate the stub for these, so we need to depend on the grpc-codegen. It's even worse... we can't "just" generate; we need to run the post-processing too, as gRPC standard generation still uses javax.generated.

[geoand]

#46698 is the implementation of the conditional dependency idea.

[cescoffier]

As it seems conditional dependency do not work, what would be the alternative?

Should we have a specific dependency for Kotlin? Like quarkus-grpc-codegen-Kotlin?

[geoand]

Yeah, we'll probably have to do that and document it (and also maybe update the project generator)