-
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 plugin for scala and java #154
Conversation
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.
Looks quite clean! We should make whether you want to generate client, server or both configurable - but that can be in a next PR.
if (req.getParameter.toLowerCase.contains("language=scala")) ScalaServerCodeGenerator.run(req) | ||
else JavaServerCodeGenerator.run(req) | ||
if (req.getParameter.toLowerCase.contains("language=scala")) ScalaBothCodeGenerator.run(req) | ||
else JavaBothCodeGenerator.run(req) |
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 guess we want to make this configurable - ticketify?
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.
} | ||
|
||
group = "com.lightbend.akka.grpc" | ||
version versionDetails().gitHashFull.substring(0, 8) |
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.
👍 for now at least 👍
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.
Yea. We might wanna tag v0.0.1
soon just to be able to find all the places that need to be changed.
plugin-tester-java/build.gradle
Outdated
} | ||
|
||
dependencies { | ||
classpath 'com.lightbend.akka.grpc:gradle-plugin:0.0.1' |
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 call it akka-grpc-gradle-plugin?
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.
Yea. I think that way it will align with other plugins better.
.travis.yml
Outdated
@@ -6,11 +6,13 @@ scala: | |||
jobs: | |||
include: | |||
- stage: publishLocal | |||
script: sbt ++$TRAVIS_SCALA_VERSION publishLocal | |||
script: sbt ++$TRAVIS_SCALA_VERSION publishLocal publishM2 |
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.
Hard to say how this pans out... also it doesn't seem like it's being picked up - which is strange, does that mean publishLocal
didn't actually have any effect either?
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.
Ahh, from https://docs.travis-ci.com/user/build-stages/#Data-persistence-between-stages-and-jobs:
If your jobs need to share files (e.g., using build artifacts from the “Test” stage for deployment in the subsequent “Deploy” stage), you need to use an external storage mechanism such as S3 and a remote scp server.
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.
Right - so we could perhaps use the pattern from https://docs.travis-ci.com/user/build-stages/warm-cache/ to warm up the ivy cache (when we configure it as a cache) but not rely on it for the pipeline. Makes sense. For now remove the publishLocal stage then?
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.
Yea. Will remove the stage.
@@ -55,6 +58,9 @@ lazy val scalapbProtocPlugin = Project( | |||
art.withClassifier(Some("assembly")) | |||
}, | |||
mainClass in assembly := Some("akka.grpc.scalapb.Main"), | |||
assemblyOption in assembly := (assemblyOption in assembly).value.copy( | |||
prependShellScript = Some(sbtassembly.AssemblyPlugin.defaultShellScript) | |||
), |
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.
Weird that this didn't seem necessary before... but 👍
|
||
dependencies { | ||
// to bring in protobuf dependency trasitively | ||
implementation 'com.google.protobuf:protobuf-gradle-plugin:0.8.5' |
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.
positively trasitive
Jenkins: looks like there's a link from paradox to |
65271c7
to
d8b2b8d
Compare
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.
👍 almost there 😄
.travis.yml
Outdated
- script: cd plugin-tester-java && ./gradlew --include-build ../gradle-plugin clean compileJava | ||
- script: cd plugin-tester-scala && ./gradlew --include-build ../gradle-plugin clean compileScala | ||
- script: sbt publishLocal && cd plugin-tester-java && sbt compile | ||
- script: sbt publishLocal && cd plugin-tester-scala && sbt compile |
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 the sbt tests might even work without publishLocal
, since we use a ProjectRef
here
- find $HOME/.ivy2 -name "ivydata-*.properties" -print -delete | ||
- find $HOME/.sbt -name "*.lock" -print -delete | ||
|
||
cache: |
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.
curious how much of a change this will make!
project.gradle.addListener(this) | ||
final boolean isScala = project.plugins.hasPlugin("scala") | ||
|
||
project.configure(project) { |
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.
This looks pretty cool
plugin-tester-java/build.gradle
Outdated
} | ||
|
||
dependencies { | ||
classpath 'com.lightbend.akka.grpc:akka-grpc-gradle-plugin:0.0.1' |
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.
Maybe add a comment here explaining that this will be replaced with the 'link' to the currently-checked-out-version?
@@ -1,5 +1,5 @@ | |||
//#full-service-impl | |||
package io.grpc.examples | |||
package io.grpc.examples.helloworld |
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.
Do we still want to move these files when we configure flat_package
by default?
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.
Yes, if we want to have the same semantics as java files.
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 that makes sense as a default
@@ -1,7 +1,7 @@ | |||
syntax = "proto3"; | |||
|
|||
option java_multiple_files = true; | |||
option java_package = "io.grpc.examples"; | |||
option java_package = "io.grpc.examples.helloworld"; |
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.
This is more consistent with upstream though, so OK... we should get our helloworld.proto
s consistent again though :)
Generates both server and client side.
Fixes #6 and #42