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

Add BuildInfo so the Akka gRPC version becomes available for specifying dependencies #303

Open
ennru opened this issue Sep 30, 2020 · 9 comments

Comments

@ennru
Copy link
Contributor

ennru commented Sep 30, 2020

eg for

    libraryDependencies += "com.lightbend.play" %% "play-grpc-runtime" % "$project.version$",
    libraryDependencies += "com.lightbend.play" %% "lagom-javadsl-grpc-testkit" % "$project.version$" % Test
@raboof
Copy link
Member

raboof commented Sep 30, 2020

👍

In akka-grpc this BuildInfo lives in the codegen subproject, so that it's available for all plugins (not just sbt)

For the runtime library in sbt, we could even pull it in automatically via the code generator (we do the same in akka-grpc)

@ennru
Copy link
Contributor Author

ennru commented Oct 5, 2020

Added #306, not sure if you had something smarter in mind.

@ignasi35
Copy link
Member

ignasi35 commented Nov 2, 2020

After reviewing the code in #306 I think we're taking the wrong approach.

I think Play and Lagom users should extract the version of Akka and Akka-HTTP from their main framework (lagom/play) and not play-grpc. This leaves only the versions of akka-grpc and grpc itself which may already be extractred from Akka-gRPC's BuildInfo.

So, instead of adding BuildInfo in play-grpc (like we did in #306) I would add akkaHttpVersion on Play's PlayVersion.

Usage would look like this: (taken from https://github.com/playframework/play-samples/tree/2.8.x/play-scala-grpc-example)

// plugins.sbt
enablePlugins(BuildInfoPlugin)
val playGrpcV = "0.9.1
buildInfoKeys := Seq[BuildInfoKey]("playGrpcVersion" -> playGrpcV)
buildInfoPackage := "play.scala.grpc.sample"

// build.sbt
val CompileDeps = Seq(
  guice,
  // play-grpc from this build (see plugins.sbt)
  "com.lightbend.play"      %% "play-grpc-runtime"   % BuildInfo.playGrpcVersion, 
  "com.typesafe.akka"       %% "akka-discovery"      % PlayVersion.akkaVersion,
  "com.typesafe.akka"       %% "akka-http"           % PlayVersion.akkaHttpVersion,
  // Test Database
  "com.h2database" % "h2" % "1.4.199"
)

val TestDeps = Seq(
  "com.lightbend.play"      %% "play-grpc-scalatest" % BuildInfo.playGrpcVersion % Test, 
  "com.lightbend.play"      %% "play-grpc-specs2"    % BuildInfo.playGrpcVersion % Test, 
  "com.typesafe.play"       %% "play-test"           % PlayVersion.current     % Test,
  "com.typesafe.play"       %% "play-specs2"         % PlayVersion.current     % Test,
  "org.scalatestplus.play"  %% "scalatestplus-play"  % "5.0.0" % Test, 
)

@ignasi35
Copy link
Member

ignasi35 commented Nov 2, 2020

Raised playframework/playframework#10519

@ennru
Copy link
Contributor Author

ennru commented Nov 3, 2020

@ignasi35 The important bit was to communicate the Akka gRPC version. You are right that the other versions should come from more relevant sources.

@raboof
Copy link
Member

raboof commented Nov 3, 2020

I think Play and Lagom users should extract the version of Akka and Akka-HTTP from their main framework (lagom/play) and not play-grpc

Why? Since we're currently actively working on akka-grpc, I think there is a good likelyhood that akka-grpc will use newer akka-http features, and thus require a newer akka-http version than play/lagom currently requires. Since Akka HTTP generally maintains binary compatibility that should work fine, right? (unless exceptional cases where play used internal API's or we dropped the ball in Akka HTTP)

@ignasi35
Copy link
Member

ignasi35 commented Nov 3, 2020

Why? Since we're currently actively working on akka-grpc, I think there is a good likelyhood that akka-grpc will use newer akka-http features, and thus require a newer akka-http version than play/lagom currently requires. Since Akka HTTP generally maintains binary compatibility that should work fine, right? (unless exceptional cases where play used internal API's or we dropped the ball in Akka HTTP)

Correct. An I'm not against akka-grpc having a BuildInfo that includes the Akka-HTTP versions used to compile that akka-grpc version. My point is that a play/lagom user opting into using play-grpc should not get yet another set of versions to choose from (the proposed PlayGrpcBuildInfo discussed here).

I agree that akka-grpc is more actively maintained and I like the idea of allowing the user to choose between AkkaGrpcBuildInfo.akkaHttpVersion and PlayVersion.akkaHttpVersion as long as they are aware of the risks.

My comment was only about PlayGrpcBuildInfo introducing what looks like an alias of AkkaGrpcBuildInfo.

@raboof
Copy link
Member

raboof commented Nov 3, 2020

I like the idea of allowing the user to choose between AkkaGrpcBuildInfo.akkaHttpVersion and PlayVersion.akkaHttpVersion as long as they are aware of the risks.

Aah, yeah. That makes sense to me, PlayGrpcBuildInfo.akkaHttpVersion would be redundant 👍

@ignasi35
Copy link
Member

ignasi35 commented Nov 3, 2020

(this is a slightly different discussion)

I was even thinking the play-grpc build could reuse the versions from akka-grpc or play using something lilke:

    val akka     = AkkaGrpcBuildInfo.akkaVersion
    val akkaHttp = AkkaGrpcBuildInfo.akkaHttpVersion

or

    val akka     = PlayVersion.akkaVersion
    val akkaHttp = PlayVersion.akkaHttpVersion

in

val akka = "2.6.5"
val akkaHttp = "10.1.12"

So we lock versions a bit more. I'm not sure it is a good idea, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants