You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "mdedetrich (via GitHub)" <gi...@apache.org> on 2023/05/09 10:52:23 UTC
[GitHub] [incubator-pekko-grpc] mdedetrich opened a new pull request, #66: Properly support SBT 1.8.2
mdedetrich opened a new pull request, #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66
As stated in https://github.com/sbt/sbt/issues/7243#issuecomment-1539778553 it seems we have an issue when it comes to resolving Scala 3 artifacts which reveals itself when using newer versions of SBT. I have found the root cause in https://github.com/apache/incubator-pekko-grpc/blob/b6210989184ba9282972ba752a8f364d694affca/sbt-plugin/src/main/scala/org/apache/pekko/grpc/sbt/PekkoGrpcPlugin.scala#L211-L219.
This PR is an attempt to remove this workaround
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#discussion_r1208112432
##########
codegen/src/main/scala/org/apache/pekko/grpc/gen/scaladsl/Serializer.scala:
##########
@@ -16,7 +16,7 @@ package org.apache.pekko.grpc.gen.scaladsl
import com.google.protobuf.Descriptors.{ Descriptor, MethodDescriptor }
import scalapb.compiler.DescriptorImplicits
-case class Serializer(name: String, init: String)
+case class Serializer(name: String, scalaType: String, init: String)
Review Comment:
This change is necessary in order to provide an explicit type to `implicit val` definitions (which are mandatory in Scala 3). Note that this isn't really a public class since its part of pekko-grpc's codegen, in other words no one should be depending on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#issuecomment-1539992607
Blocked by https://github.com/apache/incubator-pekko-http/pull/130
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#issuecomment-1565657935
@pjfanning @nvollmar @raboof @jrudolph So I have some good news and some bad news.
The good news is that I have managed to get pekko-grpc fully working with Scala 3.3. Everything works as expected and it actually solves the ugly hack mentioned at https://github.com/apache/incubator-pekko-grpc/pull/66#issue-1701845032. It also turns out that some of the issues in integrating Scala 3 into pekko-grpc was not due to the project itself but a red herring caused by pekko-http-cors publishing a broken library (as you can see in https://github.com/lomigmegard/pekko-http-cors/pull/19 they were using `cross CrossVersion.for3Use2_13` in a library which was not allowed).
The bad news is that afaik one of the scripted tests, specifically the one testing for Scala 3 i.e. https://github.com/apache/incubator-pekko-grpc/tree/main/sbt-plugin/src/sbt-test/scala3/01-basic-client-server cannot be made to work and this by a "design" of sbt and sbt scripted. Long story short there isn't any way to do a cross local publish across multiple Scala versions (i.e. https://github.com/apache/incubator-pekko-grpc/blob/main/build.sbt#L119-L124) and due to this sbt in that scripted test wont load because it can't find the Scala 3 published artifact of `pekko-runtime` (if you are wondering how it currently works, its because currently its implemented via a hack of loading the published Scala 2.13 artifact within Scala , thats what the hack at https://github.com/apache/incubator-pekko-grpc/pull/66#issue-1701845032 is doing).
The thing is that I can verify that it works with Scala 3 just by running the scripted test manually, i.e. if I on my machine do a `+publishLocal`, note the snapshot version just published and then go into `sbt-plugin/src/sbt-test/scala3/01-basic-client-server`, update the `plugin.sbt` with the just published snapshot version and then run `sbt test` it passes.
Would it be acceptable for you to just remove this `sbt-plugin/src/sbt-test/scala3` test? I can make an issue about reintroducing it back in as well as upstream bug at sbt (and I already mentioned this on discord, see https://discord.com/channels/632150470000902164/922600050989875282/1112081944369434664). Ultimately such a test doesn't really work with the concept of sbt scripted, because sbt-scripted assumes everything is run with the same scala version but with this project its a more complicated, i.e. the pekko-runtime has to be published for all Scala versions however the sbt plugin specifically can only be published for Scala 2.12 (sbt 1.x only supports 2.12).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] pjfanning commented on pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#issuecomment-1540006832
Would it make sense to remove any Scala 3 build in incubator-pekko-grpc? We can add it back when incubator-pekko-http has Scala 3 snapshots.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#discussion_r1208112594
##########
codegen/src/main/twirl/templates/ScalaCommon/Marshallers.scala.txt:
##########
@@ -35,7 +36,7 @@ import pekko.grpc.PekkoGrpcGenerated
@@PekkoGrpcGenerated
object @{service.name}Marshallers {
@for(serializer <- service.serializers) {
- implicit val @serializer.name = @{service.packageName}.@{service.name}.Serializers.@{serializer.name}
+ implicit val @serializer.name: ScalapbProtobufSerializer[@{serializer.scalaType}] = @{service.packageName}.@{service.name}.Serializers.@{serializer.name}
Review Comment:
The `serializer.scalaType` refers to the newly added field here https://github.com/apache/incubator-pekko-grpc/pull/66/files#r1208112432
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] pjfanning commented on a diff in pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#discussion_r1208114338
##########
project/Common.scala:
##########
@@ -67,20 +89,27 @@ object Common extends AutoPlugin {
version.value,
"-sourcepath",
(ThisBuild / baseDirectory).value.toString,
- "-skip-packages",
- "org.apache.pekko.pattern:" + // for some reason Scaladoc creates this
- "templates",
"-doc-source-url", {
val branch = if (isSnapshot.value) "main" else s"v${version.value}"
s"https://github.com/apache/incubator-pekko-grpc/tree/${branch}€{FILE_PATH_EXT}#L€{FILE_LINE}"
},
"-doc-canonical-base-url",
- "https://pekko.aoache.org/api/pekko-grpc/current/"),
+ "https://pekko.apache.org/api/pekko-grpc/current/") ++ (
+ if (!isScala3.value) {
+ Seq(
+ "-skip-packages",
+ "org.apache.pekko.pattern:" + // for some reason Scaladoc creates this
Review Comment:
not a big issue - but I think Scala 3 has fixed the issue that requires this skip - when I upgraded another pekko project to support Scala 3, I didn't seem to need this in Scala 3 build and I only set the skip on the Scala 2 build
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#issuecomment-1565659138
Hmm actually I think I found the root cause https://github.com/apache/incubator-pekko-grpc/blob/main/.github/workflows/build-test.yml#L108-L109
I will try and fix this, and if it doesn't work I will remove the test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#discussion_r1208114490
##########
project/Common.scala:
##########
@@ -67,20 +89,27 @@ object Common extends AutoPlugin {
version.value,
"-sourcepath",
(ThisBuild / baseDirectory).value.toString,
- "-skip-packages",
- "org.apache.pekko.pattern:" + // for some reason Scaladoc creates this
- "templates",
"-doc-source-url", {
val branch = if (isSnapshot.value) "main" else s"v${version.value}"
s"https://github.com/apache/incubator-pekko-grpc/tree/${branch}€{FILE_PATH_EXT}#L€{FILE_LINE}"
},
"-doc-canonical-base-url",
- "https://pekko.aoache.org/api/pekko-grpc/current/"),
+ "https://pekko.apache.org/api/pekko-grpc/current/") ++ (
+ if (!isScala3.value) {
+ Seq(
+ "-skip-packages",
+ "org.apache.pekko.pattern:" + // for some reason Scaladoc creates this
Review Comment:
Ill look into this in a later PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#issuecomment-1565661703
@pjfanning Wahoo! Figured out the issue, all tests to pass (didn't end up having to delete the scala 3 test). Turns out that pekko-grpc CI manually overrides the scala version and since I wasn't aware of it I wasn't doing this locally.
PR is now ready to review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#issuecomment-1565658584
Okay done, I deleted the test (unfortunately its not possible to comment out sbt scripted tests) and I am filing an issue now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich merged pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#issuecomment-1540030977
Afaik other parts from Pekko rely on grpc so it would be a missing cog in this regard.
I am fairly sure this should solve the problem, if it doesn't we can remove Scala 3
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] pjfanning commented on pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#issuecomment-1565658244
@mdedetrich disable anything that is blocking you and log a follow up issue with details
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#issuecomment-1564734887
Currently blocked by https://github.com/lomigmegard/pekko-http-cors/pull/19
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-grpc] mdedetrich commented on a diff in pull request #66: Properly support SBT 1.8.2 and Scala 3
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #66:
URL: https://github.com/apache/incubator-pekko-grpc/pull/66#discussion_r1208113793
##########
.github/workflows/build-test.yml:
##########
@@ -106,7 +106,7 @@ jobs:
- test-set: gen-java
scala-version: 2.12
- test-set: scala3
- scala-version: 2.13
+ scala-version: 3.3
Review Comment:
This is what I missed in reference to https://github.com/apache/incubator-pekko-grpc/pull/66#issuecomment-1565661703
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org