[scala3] Enable scala3_sources #222

kczulko · 2024-02-06T09:50:42Z

This PR attempts to enable scala3_sources scalapb option which is described here.

pjfanning · 2024-02-06T09:55:05Z

@kczulko will this break on our Scala 2.12 build?

kczulko · 2024-02-06T09:56:59Z

I don't think so. We're using both scala2 and scala3 in our project (the same code is being compiled twice, due to moving-to-scala3 refactoring process...). I've been checking this via +publishLocal and all was fine. Also tests were passed.

mdedetrich · 2024-02-06T10:12:38Z

My gut feeling is that this should be targeting 1.1.x, @pjfanning wdyt?

pjfanning · 2024-02-06T10:31:28Z

it doesn't yet work - it seems to break on Scala 2.12 as I suspected it might https://github.com/apache/incubator-pekko-grpc/actions/runs/7797963372/job/21265803687?pr=222

[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:10843:1: name clash: <Type>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<com.google.protobuf.DescriptorProtos.ExtensionRangeOptions,?>) in com.google.protobuf.DescriptorProtos.ExtensionRangeOptions.Builder and <T>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<MessageT,T>) in com.google.protobuf.GeneratedMessageV3.ExtendableBuilder have the same erasure, yet neither overrides the other
[info] [error] @java.lang.Override
[info] [error]       public <Type> Builder clearExtension(
[info] [error]           com.google.protobuf.GeneratedMessage.GeneratedExtension<
[info] [error]               com.google.protobuf.DescriptorProtos.ExtensionRangeOptions, ?> extension) {
[info] [error]         return super.clearExtension(extension);
[info] [error]       }
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:10842:1: method does not override or implement a method from a supertype
[info] [error] @java.lang.Override
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:24389:1: name clash: <Type>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<com.google.protobuf.DescriptorProtos.FileOptions,?>) in com.google.protobuf.DescriptorProtos.FileOptions.Builder and <T>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<MessageT,T>) in com.google.protobuf.GeneratedMessageV3.ExtendableBuilder have the same erasure, yet neither overrides the other
[info] [error] @java.lang.Override
[info] [error]       public <Type> Builder clearExtension(
[info] [error]           com.google.protobuf.GeneratedMessage.GeneratedExtension<
[info] [error]               com.google.protobuf.DescriptorProtos.FileOptions, ?> extension) {
[info] [error]         return super.clearExtension(extension);
[info] [error]       }
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:24388:1: method does not override or implement a method from a supertype
[info] [error] @java.lang.Override
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:27692:1: name clash: <Type>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<com.google.protobuf.DescriptorProtos.MessageOptions,?>) in com.google.protobuf.DescriptorProtos.MessageOptions.Builder and <T>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<MessageT,T>) in com.google.protobuf.GeneratedMessageV3.ExtendableBuilder have the same erasure, yet neither overrides the other
[info] [error] @java.lang.Override
[info] [error]       public <Type> Builder clearExtension(
[info] [error]           com.google.protobuf.GeneratedMessage.GeneratedExtension<
[info] [error]               com.google.protobuf.DescriptorProtos.MessageOptions, ?> extension) {
[info] [error]         return super.clearExtension(extension);
[info] [error]       }
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:27691:1: method does not override or implement a method from a supertype
[info] [error] @java.lang.Override
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:29999:1: name clash: <Type>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<com.google.protobuf.DescriptorProtos.FieldOptions,?>) in com.google.protobuf.DescriptorProtos.FieldOptions.Builder and <T>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<MessageT,T>) in com.google.protobuf.GeneratedMessageV3.ExtendableBuilder have the same erasure, yet neither overrides the other
[info] [error] @java.lang.Override
[info] [error]       public <Type> Builder clearExtension(
[info] [error]           com.google.protobuf.GeneratedMessage.GeneratedExtension<
[info] [error]               com.google.protobuf.DescriptorProtos.FieldOptions, ?> extension) {
[info] [error]         return super.clearExtension(extension);
[info] [error]       }
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:29998:1: method does not override or implement a method from a supertype
[info] [error] @java.lang.Override
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:31569:1: name clash: <Type>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<com.google.protobuf.DescriptorProtos.OneofOptions,?>) in com.google.protobuf.DescriptorProtos.OneofOptions.Builder and <T>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<MessageT,T>) in com.google.protobuf.GeneratedMessageV3.ExtendableBuilder have the same erasure, yet neither overrides the other
[info] [error] @java.lang.Override
[info] [error]       public <Type> Builder clearExtension(
[info] [error]           com.google.protobuf.GeneratedMessage.GeneratedExtension<
[info] [error]               com.google.protobuf.DescriptorProtos.OneofOptions, ?> extension) {
[info] [error]         return super.clearExtension(extension);
[info] [error]       }
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:31568:1: method does not override or implement a method from a supertype
[info] [error] @java.lang.Override
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:32700:1: name clash: <Type>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<com.google.protobuf.DescriptorProtos.EnumOptions,?>) in com.google.protobuf.DescriptorProtos.EnumOptions.Builder and <T>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<MessageT,T>) in com.google.protobuf.GeneratedMessageV3.ExtendableBuilder have the same erasure, yet neither overrides the other
[info] [error] @java.lang.Override
[info] [error]       public <Type> Builder clearExtension(
[info] [error]           com.google.protobuf.GeneratedMessage.GeneratedExtension<
[info] [error]               com.google.protobuf.DescriptorProtos.EnumOptions, ?> extension) {
[info] [error]         return super.clearExtension(extension);
[info] [error]       }
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:32699:1: method does not override or implement a method from a supertype
[info] [error] @java.lang.Override
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:33885:1: name clash: <Type>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<com.google.protobuf.DescriptorProtos.EnumValueOptions,?>) in com.google.protobuf.DescriptorProtos.EnumValueOptions.Builder and <T>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<MessageT,T>) in com.google.protobuf.GeneratedMessageV3.ExtendableBuilder have the same erasure, yet neither overrides the other
[info] [error] @java.lang.Override
[info] [error]       public <Type> Builder clearExtension(
[info] [error]           com.google.protobuf.GeneratedMessage.GeneratedExtension<
[info] [error]               com.google.protobuf.DescriptorProtos.EnumValueOptions, ?> extension) {
[info] [error]         return super.clearExtension(extension);
[info] [error]       }
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:33884:1: method does not override or implement a method from a supertype
[info] [error] @java.lang.Override
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:35008:1: name clash: <Type>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<com.google.protobuf.DescriptorProtos.ServiceOptions,?>) in com.google.protobuf.DescriptorProtos.ServiceOptions.Builder and <T>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<MessageT,T>) in com.google.protobuf.GeneratedMessageV3.ExtendableBuilder have the same erasure, yet neither overrides the other
[info] [error] @java.lang.Override
[info] [error]       public <Type> Builder clearExtension(
[info] [error]           com.google.protobuf.GeneratedMessage.GeneratedExtension<
[info] [error]               com.google.protobuf.DescriptorProtos.ServiceOptions, ?> extension) {
[info] [error]         return super.clearExtension(extension);
[info] [error]       }
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:35007:1: method does not override or implement a method from a supertype
[info] [error] @java.lang.Override
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:36322:1: name clash: <Type>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<com.google.protobuf.DescriptorProtos.MethodOptions,?>) in com.google.protobuf.DescriptorProtos.MethodOptions.Builder and <T>clearExtension(com.google.protobuf.GeneratedMessage.GeneratedExtension<MessageT,T>) in com.google.protobuf.GeneratedMessageV3.ExtendableBuilder have the same erasure, yet neither overrides the other
[info] [error] @java.lang.Override
[info] [error]       public <Type> Builder clearExtension(
[info] [error]           com.google.protobuf.GeneratedMessage.GeneratedExtension<
[info] [error]               com.google.protobuf.DescriptorProtos.MethodOptions, ?> extension) {
[info] [error]         return super.clearExtension(extension);
[info] [error]       }
[info] [error] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/google/protobuf/DescriptorProtos.java:36321:1: method does not override or implement a method from a supertype
[info] [error] @java.lang.Override
[info] [info] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/github/xds/service/orca/v3/OrcaLoadReportRequest.java: Some input files use or override a deprecated API.
[info] [info] /tmp/sbt_d17428cc/target/scala-2.12/pekko-grpc/main/com/github/xds/service/orca/v3/OrcaLoadReportRequest.java: Recompile with -Xlint:deprecation for details.
[info] [error] (Compile / compileIncremental) javac returned non-zero exit code
[info] [error] Total time: 66 s (01:06), completed Feb 6, 2024 10:15:54 AM

mdedetrich · 2024-02-06T10:33:22Z

it doesn't yet work - it seems to break Scala 2.12 as I suspected it might https://github.com/apache/incubator-pekko-grpc/actions/runs/7797963372/job/21265803687?pr=222

Probably need to pass some flags to scalacOptions so that the scala 2.12 compiler can accept the scala 3 source style

kczulko · 2024-02-06T12:12:47Z

Probably need to pass some flags to scalacOptions so that the scala 2.12 compiler can accept the scala 3 source style

The errors are coming from java files. For now I don't get it what could cause this issue. Do you have some suggestions?

mdedetrich · 2024-02-06T12:18:50Z

Probably need to pass some flags to scalacOptions so that the scala 2.12 compiler can accept the scala 3 source style

The errors are coming from java files. For now I don't get it what could cause this issue. Do you have some suggestions?

So that file is generated by googles own protobuf compiler so you shouldn't even manually be modifying it. Maybe we need to update the google protobuf compiler as well?

kczulko · 2024-02-06T12:25:29Z

As I'm bumping the scalapb compilerplugin I suspect the incompatibility may be here? https://github.com/scalapb/ScalaPB/blob/71befffd3db412d158fabb807ded56920fa0030f/project/Dependencies.scala#L7

pjfanning · 2024-02-06T12:57:51Z

yes - a big bump in io.grpc libs causes us issues - eg #190

kczulko · 2024-02-07T08:59:50Z

@pjfanning
Regarding failed test case... There's the protobuf-javascript issue. Do you want to workaround it somehow? It would require more work to get fixed. I've tried with golang instead of js. As a nixos user, I added protoc-gen-go to my nix shell (so that it appears in PATH) and slightly modified 06-compatibility-plugins test case. With nix it's easy however if you'd like to avoid nix, then it requires more code to determine developer's OS and whole download/unzip/(add-to-path | configure-build.sbt) logic. Or some shenanigans could be done on the .github workflow files but I would rather want to keep coherent configuration for both local and CI envs.

Next thing - I don't know why Link Validator build step failed :)

pjfanning · 2024-02-07T09:08:45Z

The Link Validator tests are very brittle. It calls all the linked sites and many of them fail on occasion.
The compatibility test is something we can probably rework or remove. Let me discuss it with other contributors.

pjfanning · 2024-02-07T15:32:45Z

@mdedetrich is it ok to remove the compatibility test that is failing and log a new issue to replace it? grpc refactored the javascript support and the test needs to be rewritten or replaced if we want to upgrade grpc libs.

kczulko · 2024-02-09T11:00:02Z

@pjfanning @mdedetrich
Please let me know if there's anything you want from me to push it forward.

Thanks in advance,
Karol

raboof · 2024-02-12T09:48:32Z

With nix it's easy however if you'd like to avoid nix, then it requires more code to determine developer's OS and whole download/unzip/(add-to-path | configure-build.sbt) logic. Or some shenanigans could be done on the .github workflow files but I would rather want to keep coherent configuration for both local and CI envs

since this is an sbt integration test, I think it's OK if there's some prerequisites for running it (such as putting protoc-gen-go on the PATH). If we can add that to the GHA script and document that the user needs to do that themselves (however it needs to be done on their OS), then I think we're OK.

kczulko · 2024-02-13T18:31:03Z

@raboof

Ok then. Let me try to do it. I'll use this gh action together with this go install command. However, I'm not allowed to check how this will work under github actions env since contributors have to wait for the CI run approval. Therefore I'd like to ask you (the contributors) to eventually fix all the shortcomings (as you can probably do it without waiting for approvals).

Best regards,
Karol

raboof · 2024-02-20T12:34:53Z

codegen/src/main/scala/org/apache/pekko/grpc/gen/scaladsl/ScalaCompatConstants.scala

+package org.apache.pekko.grpc.gen.scaladsl
+
+private[scaladsl] class ScalaCompatConstants(emitScala3Sources: Boolean = false) {
+  // val WildcardType: String = if (emitScala3Sources) "?" else "_"


I guess this can be dropped

raboof · 2024-02-20T12:36:48Z

runtime/src/main/scala/org/apache/pekko/grpc/internal/PekkoDiscoveryNameResolverProvider.scala

-    new PekkoDiscoveryNameResolver(discovery, defaultPort, targetUri.getAuthority, portName, protocol, resolveTimeout)
-  }
+  override def newNameResolver(targetUri: URI, args: NameResolver.Args): PekkoDiscoveryNameResolver =
+    new PekkoDiscoveryNameResolver(discovery, defaultPort, serviceName, portName, protocol, resolveTimeout)


Is this change related/required for the switch to the new io.grpc version and introduction of scala3_sources?

It looks like authority is no longer passed when newNameResolver is called.

Hmm, that is slightly scary (I assume the assert was there for a reason), but if you're confident this is a correct fix then 👍

raboof · 2024-02-20T12:37:25Z

runtime/src/main/mima-filters/1.1.x.backwards.excludes/io.grpc-upgrade.backwards.excludes

+# under the License.
+
+# Upgrade to io.grpc 1.60 caused this bin compat issue
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.grpc.internal.PekkoDiscoveryNameResolverProvider.this")


👍 since internal

raboof · 2024-02-20T12:37:43Z

interop-tests/src/test/scala/org/apache/pekko/grpc/scaladsl/NonBalancingIntegrationSpec.scala

@@ -184,8 +183,7 @@ class NonBalancingIntegrationSpec(backend: String)

      val failure =
        client.sayHello(HelloRequest(s"Hello friend")).failed.futureValue.asInstanceOf[StatusRuntimeException]
-      failure.getStatus.getCode should be(Code.UNAVAILABLE)
-      client.closed.failed.futureValue shouldBe a[ClientConnectionException]
+      failure.getStatus.getCode should (equal(Code.UNKNOWN).or(equal(Code.UNAVAILABLE)))


strange but no problem

raboof · 2024-02-20T12:39:46Z

Great work here!

I'm confused why the Validate and test CI workflow (from build-test.yml) isn't running here

samueleresca · 2024-02-20T12:49:55Z

sbt-plugin/src/sbt-test/gen-scala-server/06-compatibility-plugins/project/ProtocJSPlugin.scala

-    Seq(Compile, Test).flatMap(inConfig(_)(PB.targets += PB.gens.js -> resourceManaged.value / "js"))
+    Seq(Compile, Test).flatMap(inConfig(_)(
+      Seq(
+        PB.targets += PB.gens.go -> resourceManaged.value / "go")))


Should the ProtocJSPlugin object renamed to something ProtocGoPlugin?

raboof · 2024-02-20T13:04:43Z

the GHA error seems to be 'every step must define a uses or run key' - though this is strange since it seems you do.

samueleresca · 2024-02-20T13:13:31Z

the GHA error seems to be 'every step must define a uses or run key' - though this is strange since it seems you do.

I think that error is caused by the Install go & go-protobuf step defined with 2 "-":

      - name: Install go & go-protobuf
      - uses: actions/setup-go@v5
      ...

raboof · 2024-02-20T13:14:18Z

the GHA error seems to be 'every step must define a uses or run key' - though this is strange since it seems you do.

I think that error is caused by the Install go & go-protobuf step defined with 2 "-":
      - name: Install go & go-protobuf
      - uses: actions/setup-go@v5
      ...

ah, indeed, sharp eyes!

raboof · 2024-02-20T13:19:10Z

.github/workflows/build-test.yml

+        with:
+          go-version: '^1.20'
+        run: go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.32.0
+


Ah looks like we have to split this into two: "a step cannot have both the uses and run keys"

kczulko · 2024-02-20T13:29:40Z

Dear Lord... I think it's ok now :D

raboof · 2024-02-20T13:40:22Z

It seems the link validator isn't wrong here, and the license report is linking to http://www.jetbrains.org for a new org.jetbrains:annotations:13.0 dependency. We might want to add http://www.jetbrains.org to the non-https-whitelist in ./scripts/link-validator.conf?

Enable scala3_sources

64c5eb9

kczulko added 2 commits February 6, 2024 11:04

Add missing class

94a4831

Add header

597122c

pjfanning added this to the v1.1.0 milestone Feb 6, 2024

kczulko and others added 2 commits February 6, 2024 16:01

Attempt to fix tests

ba21794

bin compat issue

d67c5cc

grpc 1.61.1

f22d398

pjfanning mentioned this pull request Feb 8, 2024

[DRAFT] io.grpc 1.59 #190

Closed

pjfanning mentioned this pull request Feb 11, 2024

remove compatibility test #229

Closed

Refactor 06-compatibility-plugins test

277bed4

raboof reviewed Feb 20, 2024

View reviewed changes

samueleresca reviewed Feb 20, 2024

View reviewed changes

kczulko added 2 commits February 20, 2024 13:58

Attempt to fix gh action yaml

743025d

Rename plugin

831d696

Fix gh action yml

8eaac08

raboof reviewed Feb 20, 2024

View reviewed changes

kczulko added 2 commits February 20, 2024 14:23

Seperate step for installing protoc-gen-go

98d651f

Remove hyphen

b3a6769

Fix link validator and ProtocGoPlugin.scala

d07db68

raboof approved these changes Feb 20, 2024

View reviewed changes

raboof merged commit d154013 into apache:main Feb 20, 2024
16 checks passed

raboof mentioned this pull request Feb 20, 2024

add new compatibility test #228

Closed

pjfanning mentioned this pull request May 15, 2024

Generate scala code compliant with -Xsource:3 #307

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[scala3] Enable scala3_sources #222

[scala3] Enable scala3_sources #222

kczulko commented Feb 6, 2024

pjfanning commented Feb 6, 2024

kczulko commented Feb 6, 2024 •

edited

Loading

mdedetrich commented Feb 6, 2024

pjfanning commented Feb 6, 2024 •

edited

Loading

mdedetrich commented Feb 6, 2024

kczulko commented Feb 6, 2024

mdedetrich commented Feb 6, 2024

kczulko commented Feb 6, 2024

pjfanning commented Feb 6, 2024

kczulko commented Feb 7, 2024 •

edited

Loading

pjfanning commented Feb 7, 2024

pjfanning commented Feb 7, 2024

kczulko commented Feb 9, 2024

raboof commented Feb 12, 2024

kczulko commented Feb 13, 2024

raboof Feb 20, 2024

raboof Feb 20, 2024

kczulko Feb 20, 2024

raboof Feb 20, 2024

raboof Feb 20, 2024

raboof Feb 20, 2024

raboof commented Feb 20, 2024

samueleresca Feb 20, 2024

raboof commented Feb 20, 2024

samueleresca commented Feb 20, 2024

raboof commented Feb 20, 2024

raboof Feb 20, 2024

kczulko commented Feb 20, 2024

raboof commented Feb 20, 2024

[scala3] Enable scala3_sources #222

[scala3] Enable scala3_sources #222

Conversation

kczulko commented Feb 6, 2024

pjfanning commented Feb 6, 2024

kczulko commented Feb 6, 2024 • edited Loading

mdedetrich commented Feb 6, 2024

pjfanning commented Feb 6, 2024 • edited Loading

mdedetrich commented Feb 6, 2024

kczulko commented Feb 6, 2024

mdedetrich commented Feb 6, 2024

kczulko commented Feb 6, 2024

pjfanning commented Feb 6, 2024

kczulko commented Feb 7, 2024 • edited Loading

pjfanning commented Feb 7, 2024

pjfanning commented Feb 7, 2024

kczulko commented Feb 9, 2024

raboof commented Feb 12, 2024

kczulko commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raboof commented Feb 20, 2024

Choose a reason for hiding this comment

raboof commented Feb 20, 2024

samueleresca commented Feb 20, 2024

raboof commented Feb 20, 2024

Choose a reason for hiding this comment

kczulko commented Feb 20, 2024

raboof commented Feb 20, 2024

kczulko commented Feb 6, 2024 •

edited

Loading

pjfanning commented Feb 6, 2024 •

edited

Loading

kczulko commented Feb 7, 2024 •

edited

Loading