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

Feature Request: support Java protoc plugins and allow specifying main class #31

Closed
edenman opened this issue Aug 5, 2015 · 8 comments
Assignees
Milestone

Comments

@edenman
Copy link
Contributor

edenman commented Aug 5, 2015

We have an existing codegen plugin for protobuf that is invoked in a maven build thusly:

          <protocPlugins>
            <protocPlugin>
              <id>foo-protobuf</id>
              <groupId>com.foo.protobuf</groupId>
              <artifactId>foo-protobuf-plugin</artifactId>
              <version>HEAD-SNAPSHOT</version>
              <mainClass>com.foo.protobuf.compiler.plugin.PluginMain</mainClass>
            </protocPlugin>
          </protocPlugins>

If I try to add this same plugin the way this project specifies that plugins should be added:

plugins {
    foo {
      artifact = 'com.foo.protobuf:foo-protobuf-plugin:HEAD-SNAPSHOT'
    }

The build fails (target protobufToolsLocator_foo) because it's trying to download an executable artifact foo-protobuf-plugin-osx-x86_64.exe which doesn't exist (the artifact is actually foo-protobuf-plugin-HEAD-SNAPSHOT.jar).

I think I can work around this by creating a local executable that downloads the jar and runs it with the classname specified, but it would be rad if this library included this functionality.

@ejona86
Copy link
Collaborator

ejona86 commented Aug 5, 2015

The hardest part of this seems to be "support Java protoc plugins." After that, it seems it would be easy to provide a way to override the mainClass (I assume because the JAR did not set one in its metadata).

It does seem that the plugin would have to generate a .sh or .bat file to be executed by protoc.

@zhangkun83 zhangkun83 self-assigned this Aug 7, 2015
@zhangkun83 zhangkun83 changed the title Feature Request: support mainClass et al for protocPlugin Feature Request: support Java protoc plugins and allow specifying main class Aug 10, 2015
@zhangkun83
Copy link
Collaborator

So, if I don't find a .exe file in the artifact, but find a .jar file, I would assume it is a Java plugin. In that case, I would generate a .sh or .bat file to be executed by protoc.

I am not yet sure if .bat file works under windows, because the Maven protoc plugin makes use of winrun4j.

@ejona86
Copy link
Collaborator

ejona86 commented Aug 10, 2015

Yeah, it might not; it may require calling cmd.exe, which would defeat the purpose. The Maven protoc plugin logic seems sane; mimicking it would be reasonable.

@edenman
Copy link
Contributor Author

edenman commented Aug 10, 2015

FWIW, the "specify main method" thing isn't actually important to me, I realized the jar is actually executable already. Main thing is to just support .jar plugins. My current workaround:

#!/bin/sh
curl https://my-maven-server.com/content/groups/public/com/mycompany/protobuf/protobuf-plugin/1.3/protobuf-plugin-1.3-shaded.jar > protobuf-plugin-1.3-shaded.jar
java -jar protobuf-plugin-1.3-shaded.jar
rm protobuf-plugin-1.3-shaded.jar

@zhangkun83 zhangkun83 added this to the 0.8.0 milestone Aug 28, 2015
@noel-yap
Copy link
Contributor

From https://github.com/google/protobuf-gradle-plugin/blob/master/src/main/groovy/com/google/protobuf/gradle/ToolsLocator.groovy#L84, it looks to me that all that's needed is for some way to override the classifier and extension fields. This solution would also be generic in that the codegen plugin can be implemented in any language. Those wanting to use Java would then follow something like http://en.newinstance.it/2012/04/17/self-executing-jar-files/ to build the executable.

@noel-yap
Copy link
Contributor

noel-yap commented Jul 17, 2017

@zhangkun83 , I'd like to implement the above PR. One issue is that if we stick with the Gradle syntax described in https://docs.gradle.org/3.3/userguide/dependency_management.html#sub:classifiers, it would break backwards compatibility since existing users will have to start specifying explicitly the classifier part as ${os.detector.classifier} and the extension as exe.

One alternative is to create some other syntax (perhaps a field indicating the classifier and extension).

My personal preference would be to break backwards compatibility now while the version of the plugin is 0.+ rather than to introduce syntax that's inconsistent with the rest of Gradle. What do you think?

@noel-yap
Copy link
Contributor

noel-yap commented Jul 18, 2017

Actually, I see a way to maintain backwards compatibility -- developers of the codegen plugin publish with a classifier (eg jvm8) and the extension can still default to exe. When resolving the codegen plugin dependency, if a classifier isn't specified, it'll still default to using os.detector.classifier.

@wwadge
Copy link

wwadge commented Feb 20, 2019

If you're looking at a somewhat complete example, you might wish to have a look here: https://github.com/Xorlev/grpc-jersey/blob/master/build.gradle

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

5 participants