Should not commit sources generated from protobuf descriptions

shvachko commented 9 years ago

Currently we commit both .proto definitions and the generated Java sources. We should add a build target, which would generate Java sources from .proto under target/generated-sources. Then we can remove the Java files from the source tree. Note: add Apache license to grfa.proto as a part of this.

octo47 commented 9 years ago

I'd rather commit generated files. The reason is that we need to have a specific version of protoc on every host where we are compiling our project. I think it is better just to have profile to run protoc when needed (and may run it everytime on jenkins to be sure).

shvachko commented 9 years ago

Yes compiling sources will require protoc. The package target should add them to the appropriate jars. So users will not need to compile or have protoc. I don't see any reason to keep transient code in the repo. It is the same as committing generated jars in the repo or compiled libraries. You can, but what's the point. Besides if there are any differences in how protoc generates Java code for different providers/versions of Java or OSs, then there is no other way.

octo47 commented 9 years ago

I told about developers. That become not possible to just download and compile giraffa. One need to install protobuf expected version. Just not very convent in comparison of very rare changes of .proto files. External dependency is not good. But I'm ok with that. Just pointed out that it would introduce external dependency for build process.

weilintsaiWand commented 8 years ago

I spent some time on this issue but I am not sure if I am on the right track. For now I add some lines in giraffa-core/pom.xml in maven-antrun's generate-sourcesphase

<exec executable="protoc">
  <arg value="-I=${basedir}/src/main/proto" />
  <arg value="--java_out=${generated.sources}/java"/>
  <arg value="${basedir}/src/main/proto/grfa.proto"/>
</exec>

The java file can be generated as target/generated-sources/java/org/apache/giraffa/GiraffaProtos.java After removing original src/main/java/org/apache/giraffa/GiraffaProtos.java We can use mvn clean install to build the project.

However, there are two problems I can not solve now

It still use protoc to compile java file. Hence, users still need install protoc on their machine. Konstantin mentioned "The package target should add them to the appropriate jars. " But I have no idea how to do it. Can any one give me some hints?
Even though the mvn install runs smoothly. The IDE reports bunch of error while loading the file because GiraffaPBHelper.java imports GiraffaProtos.java and the later file is not ready yet. Any suggestion?

weilintsaiWand commented 8 years ago

Quick update for the IDE problem I mentioned above. It turns out to be my IDE project setting problem. The problem disappeared after I deleted the setting file and reload the project to IDE again. I test the code with both Eclipse and Intellij could load the generated java file under target/generated-sources/java/org/apache/giraffa/ correctly.

weilintsaiWand commented 8 years ago

What I did in this patch

Delete GiraffaProtos.java from source folder
Instead, the GiraffaProtos.java will be generated under
target/generated-sources/java/org/apache/giraffa/ in maven-antrun's generate-sources phase
Developer needs build the project and then open IDE. Otherwise, IDE will report import errors.

Need further discussion part

Plamen and I have a short discussion about how we deal with this issue properly. We believe we should talk about it further before commit code. Below are some concerns need to be addressed.

Do we need protoc (protobuf compiler)

If we don't put GiraffaProtos.java in repository. Developers need install protoc themselves. No matter users generate GiraffaProtos.java manually or by mvn command, the protoc is required. That introduce additional dependency.

Where Java files to be generated

Current approach

It's in target/generated-sources/java/org/apache/giraffa/

Plamen's proposal

To put in src/java/org/apache/giraffa/ and to edit gitignore to ignore this file.

My personal opinion

If we don't way to install protoc, we should stick on original approach.
I would like to put generated file in target/generated-sources/java/org/apache/giraffa/ so that we don't need maintain gitignore file. Otherwise, we need change gitignore when there's new generated file.

shvachko commented 8 years ago

A requirement for protoc compiler should be fine for developers. Other projects do that: https://wiki.apache.org/hadoop/HowToContribute#Build_Tools

We can link this guide from Giraffa wiki: https://wiki.apache.org/hadoop/ProtocolBuffers

shvachko commented 8 years ago

Works for me. Both command-line and Eclipse builds worked.

shvachko commented 8 years ago

You got two +1 (Cos and me). Should be able to push changes to trunk. Please follow the code sharing instructions

GiraffaFS / giraffa