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

Don't compile protos in extracted-protos #22

Open
SillyFreak opened this issue Jan 26, 2015 · 4 comments
Open

Don't compile protos in extracted-protos #22

SillyFreak opened this issue Jan 26, 2015 · 4 comments

Comments

@SillyFreak
Copy link

To my understanding, the extracted-protos/{main,test} directories are only used to make protos from dependencies available to protoc. These protos were already compiled in the dependency projects, so the class files do already exist. It's therefore only necessary to compile the protos from the source directory, not thise from the extracted directory.

For example, let's say you have the following protos:

/project/src/main/proto/messages.proto
  ...
/project/src/test/proto/messages_test.proto
  import "messages.proto";
  ...

For this to work, I need

dependencies {
    testProtobuf fileTree("src/main/proto")
}

The plugin outputs the following Java files:

/project/build/generated-sources/main/Messages.java
/project/build/generated-sources/test/MessagesTest.java
/project/build/generated-sources/test/Messages.java

The latter file is redundant, and e.g. in Eclipse leads to errors being shown: "The type Messages is already defined"

When the referenced proto isn't from the same project, the freshly compiled class file will likely shadow the one from the library, which could lead to problems when the library version at runtime is different from the one at compile time.

The executed protoc command should be changed as follows:

#instead of
protoc -Isrc/test/proto -Ibuild/extracted-protos/test --java_out=build/generated-sources/test src/test/proto/messages_test.proto build/extracted-protos/test/messages.proto
#use
protoc -Isrc/test/proto -Ibuild/extracted-protos/test --java_out=build/generated-sources/test src/test/proto/messages_test.proto

i.e., do not list the input files in extracted-protos


Are there any cases where it is usually necessary to compile protos in extracted-protos? Is it the right approach to list such protos as a dependency to the project being built?

@aantono
Copy link
Owner

aantono commented Jan 26, 2015

I am not sure that I agree with this interpretation. In your build you have 2 proto sources, one, living in the source-tree, thus under src/test/proto and another declared as a dependency. The problem being is that you are defining a proto dependency of a main proto as a test dependency, whereas, because the test configuration depeneds on main, all your classes in main are already available to test, thus the classes generated from the messages.proto. Any protos you declare as dependencies for the configuration, get extracted into the build/extracted-protos/ directory, as they should.

Also, according to the recommendations, it is a bad practice to bundle pre-compiled proto generated java classes and distribute them as dependencies, as they depend on a fixed and exact version of Google's protobuf-lib-java and will cause far more conflicts if mismatched. The recommended approach is to always generate the java stubs for protos directly inside the build, and that is why the plugin supports ability to define tar/zip/etc dependencies for proto archived artifacts to be used/shared between different projects.

@SillyFreak
Copy link
Author

because the test configuration depeneds on main, all your classes in main are already available to test

Yes, but the protos aren't. If I skip testProtobuf fileTree("src/main/proto"), I get

messages.proto: File not found.
messages_test.proto: Import "messages.proto" was not found or had errors.

(I know this is rather artificial, in my real project both main and test depend on the same library proto, but the result is the same: I need the testProtobuf import and also have the proto in the main configuration)

Also, according to the recommendations, it is a bad practice to bundle pre-compiled proto generated java classes and distribute them as dependencies, as they depend on a fixed and exact version of Google's protobuf-lib-java and will cause far more conflicts if mismatched.

I was not aware of this recommendation, following that might be part of the solution - it doesn't solve the issue of protos in both main and test, though.

If I may ask, does that mean that libraries should never package the compiled protos? Doesn't that mean that using protobuf can never be transparent to a consuming project?

Maybe a useful proposal would be to do the following change:

  • get rid of the testProtobuf dependency (in my project)
  • add -Isrc/main/proto -Ibuild/extracted-protos/main to generateTestProto, but don't compile the files in these directories

What do you think?

@aantono
Copy link
Owner

aantono commented Feb 5, 2015

To answer the question of packaging - YES, protos are like old CORBA idl files, you don't want to precompile and redistribute them, as they are ALWAYS subject to change, that code is not real, it is a generated mapping on top of some serialization. The compatibility guaranteed is the wire-format compatibility, not the java code generation (it actually changes with every version of protoc, even the patched ones). That is one of the reasons why in protoc 2.3 Google has added the code generation plugin support to protoc, in order to allow people to inject bits and pieces of custom, explicit (real) code into the generated one.

So you can using the include directive to just include files you don't care about compiling from elsewhere... That can be done by setting the include on the ProtobufCompile - https://github.com/aantono/gradle-plugin-protobuf/blob/master/src/main/groovy/ws/antonov/gradle/plugins/protobuf/ProtobufCompile.groovy#L21

@SillyFreak
Copy link
Author

Thanks for the response, adding the includes worked as I expected, so that's definitely a workaround for now. I did it in the least flexible way possible - only for the test configuration, and only with default paths, but that's the code if anybody needs it for their project:

generateTestProto.include("src/main/proto")
generateTestProto.include("build/extracted-protos/main")

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

2 participants