-
Notifications
You must be signed in to change notification settings - Fork 124
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
Gradle/java (google proto plugin, our codegen as protoc plugin) #41
Conversation
Refs #6 |
plugin-tester-java/build.gradle
Outdated
artifact = 'com.google.protobuf:protoc:3.4.0' | ||
} | ||
plugins { | ||
akkagrpc { |
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.
akkaGrpc?
case -1 ⇒ resultSoFar | ||
case b ⇒ read(Array.concat(resultSoFar, buffer.take(b))) | ||
} | ||
} |
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.
val baos = new ByteArrayOutputStream(math.max(64, in.available()))
val buffer = Array.ofDim[Byte](32 * 1024)
var bytesRead = System.in.read(buffer)
while (bytesRead > 0) {
baos.write(buffer, 0, bytesRead)
bytesRead = System.in.read(buffer)
}
val bytes: Array[Byte] = baos.toByteArray
Seems to be good enough IMO
// res.close() | ||
|
||
// Probably not efficient, but this is probably not the bottleneck anyway: | ||
outBytes.foreach(byte ⇒ System.out.write(byte.toInt)) |
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.
val bos = new BufferedOutputStream(System.out)
bos.write(outBytes)
Likely already good enough :)
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.
meh checked it writes in one to one flushes anyway, but a bit nicer anyway (then flush/close on it)
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.
LGTM, not sure if we really need the extra project or if we should just use codegen-common
for that.
import scala.collection.JavaConverters._ | ||
|
||
class JavaServerCodeGenerator extends CodeGenerator { | ||
def name = "grpc-akka-javadsl" |
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.
Should we use "akka-grpc-javadsl" to be consistent with the overall naming?
} | ||
|
||
ext { | ||
akkaGrpcVersion = '286bc6ae+20180227-1623' |
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.
We need to get that from somewhere so it doesn't need to be manually changed all the time.
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.
The best thing I can come up with is taking it from an environment variable... (and perhaps default to the name sbt-dynver generates for a non-dirty repo). Perhaps something for a separate task though?
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.
Yep, separate tasks sounds fine for me.
import scala.annotation.tailrec | ||
import akka.http.grpc._ | ||
|
||
object Main extends App { |
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.
I think we can put that into codegen-common
(and call it just codegen
).
|
||
// For now we hard-code the Java Server code generator here, | ||
// but options can be passed in via `request.getParameter` to | ||
// make this more flexible in the future |
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.
Exactly, let's create a ticket to implement that.
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.
Not sure if a ticket is needed, we can add parameters as soon as we need them? (already added one in #44)
Renaming |
|
||
val inBytes: Array[Byte] = { | ||
val baos = new ByteArrayOutputStream(math.max(64, System.in.available())) | ||
val buffer = Array.ofDim[Byte](32 * 1024) |
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.
Should be new Array[Byte](32 * 1024)
. Doesn't matter here but in general ofDim
is a performance problem.
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.
(Because it uses a ClassTag to make new Array[T](n)
work which compiles down to using reflection for creating the array).
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.
Ah right, forgot about that.
…that So far it seems protobuf-gradle-plugin correctly invokes our codegen plugin and our codegen plugin can parse the input, but protoc from protobuf-gradle-plugin cannot parse our plugin output.
Because it uses a `ClassTag` to make `new Array[T](n)` work which compiles down to using reflection for creating the array
import akka.NotUsed; | ||
import akka.stream.javadsl.Source; | ||
|
||
interface @service.name |
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.
If you use @{service.name}
you can put the brace on the same line.
|
||
interface @service.name | ||
{ | ||
@for(method <- service.methods) { |
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.
In my PR I cleaned up indentation in the other generated files even if it looks a bit weirder in the generator files.
@@ -6,6 +6,7 @@ addSbtPlugin("com.dwijnand" % "sbt-dynver" % "2.1.0") | |||
addSbtPlugin("com.typesafe.sbt" % "sbt-twirl" % "1.3.13") | |||
addSbtPlugin("com.eed3si9n" % "sbt-assembly" % "0.14.6") | |||
addSbtPlugin("com.eed3si9n" % "sbt-buildinfo" % "0.8.0") | |||
addSbtPlugin("net.virtual-void" % "sbt-dependency-graph" % "0.9.0") |
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.
Is that really needed? ;)
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.
Best plugin evaaarrr ;-)
@@ -0,0 +1,40 @@ | |||
package akka.grpc.gen |
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.
Left over?
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.
LGTM, can be merged from my side once the left-overs are removed. Whitespace of generated files can be fixed later.
We can now successfully invoke our protoc plugin, and the (trivial) java helloworld implementation successfully compiles against the generated Java interface.