gradle-plugin-protobuf icon indicating copy to clipboard operation
gradle-plugin-protobuf copied to clipboard

Don't compile protos in extracted-protos

Open SillyFreak opened this issue 10 years ago • 4 comments

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?

SillyFreak avatar Jan 26 '15 20:01 SillyFreak

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.

aantono avatar Jan 26 '15 20:01 aantono

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?

SillyFreak avatar Jan 26 '15 21:01 SillyFreak

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

aantono avatar Feb 05 '15 00:02 aantono

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")

SillyFreak avatar Feb 05 '15 18:02 SillyFreak