[Maven-project] Incorporate maven-proto-plugin into build of Maven projects by Thespica · Pull Request #535 · apache/incubator-graphar

Thespica · 2024-07-07T09:37:57Z

Reason for this PR

What changes are included in this PR?

Add maven-proto-plugin into pom.xml of java-info.

When the maven project is built, this plugin will compile protobuf into java source code into java-info/src/main/java/org/apache/graphar/proto.

Potential problem:

Are generated-code located in java-info/src/main/java/org/apache/graphar/proto suitable?
Once the code was generated, developer should remove the code manually to avoid compile error.

Are these changes tested?

No

Are there any user-facing changes?

SemyonSinchenko · 2024-07-07T09:47:51Z

If protobuf is in maven build already let's remove it from buf.gen.yaml: https://github.com/apache/incubator-graphar/blob/main/buf.gen.yaml#L33

SemyonSinchenko · 2024-07-07T09:49:43Z

maven-projects/info/pom.xml

        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+        <os.plugin.version>1.6.2</os.plugin.version>
+        <protobuf.plugin.version>0.6.1</protobuf.plugin.version>
+        <protobuf.version>3.21.1</protobuf.version>


I would suggest to pin the version not only for Maven builds but globally for the whole GraphAr. Otherwise we may face problem one day if we will try to serialize from java and, for example, read from cpp

Yes, we can follow our doc and pin it at 1.32.0

1.32.0 is the version of buf, not protoc.

Th actual version of protobuf that we are using is 3.27.1

(explanation: we are on proto3 syntax and the version of protobuf plugin for a minor version of protoc itself)

I mean, I have a strong feeling, that we need to pin it globally. Otherwise it may become hard to maintain it in two different places (Maven and buf.gen.yaml)

1.32.0 is the version of buf, not protoc.

Th actual version of protobuf that we are using is 3.27.1

(explanation: we are on proto3 syntax and the version of protobuf plugin for a minor version of protoc itself)

I mean, I have a strong feeling, that we need to pin it globally. Otherwise it may become hard to maintain it in two different places (Maven and buf.gen.yaml)

agree, we need to unify the protobuf version in project.

So we need to decide which version of buf and protoc shoud we pin.

The current protoc version is 3.27.1; at least we may pin it in Maven properties

If we are not going to use buf for generatign Java code, we do not need to touch buf version from this PR

SemyonSinchenko · 2024-07-07T09:51:54Z

maven-projects/info/pom.xml

+        </extensions>
+        <plugins>
+            <plugin>
+                <groupId>org.xolstice.maven.plugins</groupId>


While this xolostice plugin is more popular at the moment, I like this one more: https://github.com/ascopes/protobuf-maven-plugin

I found it more human-friendly and easier to work with.

There is a good motivation: https://github.com/ascopes/protobuf-maven-plugin?tab=readme-ov-file#why-do-we-need-another-plugin

With this plugin we do not need to have os-plugin

This plugin requires maven 3.8.2, which means we have to upgrade the requirement of maven version if we accept the newer plugin.

I would update the Maven. I see no reason to keep older version without a reason

acezen · 2024-07-08T02:39:13Z

Maybe we can put the location of generated code to maven-projects/src/main/java/org/apache/graphar/proto/? since we want the generated code can be a independent package like arrow java's flatbuf

acezen · 2024-07-08T02:39:50Z

Why it would raise compile error if not delete the generated code?

Thespica · 2024-07-08T13:01:00Z

maven-projects/src/main/java/org/apache/graphar/proto/

With modifications, the code well generated at maven-projects/proto/src/main/java/org/apache/graphar/proto, and will be clean automatically when regenerating.

SemyonSinchenko · 2024-07-08T13:08:55Z

Guys, why do we need both, buf-java plugin and maven-protobuf plugin? For me, if we do not want to store the code in the repository, let's remove java-generation from buf and leave only the maven plugin. Or did I miss something?

acezen · 2024-07-08T15:49:08Z

Guys, why do we need both, buf-java plugin and maven-protobuf plugin? For me, if we do not want to store the code in the repository, let's remove java-generation from buf and leave only the maven plugin. Or did I miss something?

hi, Sem, you are not miss anything. We do need leave only the maven plugin. @Thespica, could you delete the java-generation from buf if we use maven plugin?

acezen · 2024-07-09T06:37:58Z

maven-projects/src/main/java/org/apache/graphar/proto/

With modifications, the code well generated at maven-projects/proto/src/main/java/org/apache/graphar/proto, and will be clean automatically when regenerating.

Sorry for spell the path wrong, I suggest we can put the generated code to maven-projects/format/src/main/java/org/apache/graphar/proto, since it's the generated code for format.

Thespica · 2024-07-09T06:43:37Z

maven-projects/src/main/java/org/apache/graphar/proto/

With modifications, the code well generated at maven-projects/proto/src/main/java/org/apache/graphar/proto, and will be clean automatically when regenerating.

Sorry for spell the path wrong, I suggest we can put the generated code to maven-projects/format/src/main/java/org/apache/graphar/proto, since it's the generated code for format.

I think it's okay to naming module as proto, since the package name is proto.

acezen

I have checked the compilation. LGTM, can you add compile process to format CI?

Thespica · 2024-07-09T09:08:11Z

Hi @SemyonSinchenko

I have transfered to the newer plugin, could you review again?

SemyonSinchenko · 2024-07-09T14:36:58Z

It looks cool. Thank you!

I still have some concerns. It seems to me that we should use a maven-shade plugin to shade protobuf and rename generated classes... Otherwise, users may face dependency hell. But it is out of scoop for the current PR. Let's go step by step.

SemyonSinchenko · 2024-07-09T14:37:33Z

.github/workflows/format.yml

+      - name: Java Code Generate
+        run: |
+          pushd maven-projects/proto
+          mvn clean compile


[Minor] why compile, not generate-sources?

…ojects (apache#535) * Add proto-maven-plugin for java-info * upgrade proto version adn patched optinal * split proto into a single package * modify proto java package name * use ascopes maven plugin and delete java code gen in buf.gen.yaml * Add ci for Java code-gen * modify code-gen command

Thespica added 2 commits July 6, 2024 17:47

Add proto-maven-plugin for java-info

b49ee31

upgrade proto version adn patched optinal

c7169a5

Thespica requested review from SemyonSinchenko and acezen July 7, 2024 09:37

SemyonSinchenko reviewed Jul 7, 2024

View reviewed changes

Thespica added 2 commits July 8, 2024 20:54

split proto into a single package

6d8e15b

modify proto java package name

5eacd7e

use ascopes maven plugin and delete java code gen in buf.gen.yaml

f20f6a8

acezen approved these changes Jul 9, 2024

View reviewed changes

Add ci for Java code-gen

ff1f52c

SemyonSinchenko approved these changes Jul 9, 2024

View reviewed changes

modify code-gen command

8b81b6e

Thespica merged commit 98f3850 into apache:main Jul 10, 2024

Conversation

Thespica commented Jul 7, 2024

Reason for this PR

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

SemyonSinchenko commented Jul 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acezen Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acezen commented Jul 8, 2024

Uh oh!

acezen commented Jul 8, 2024

Uh oh!

Thespica commented Jul 8, 2024

Uh oh!

SemyonSinchenko commented Jul 8, 2024

Uh oh!

acezen commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acezen commented Jul 9, 2024

Uh oh!

Thespica commented Jul 9, 2024

Uh oh!

acezen left a comment

Choose a reason for hiding this comment

Uh oh!

Thespica commented Jul 9, 2024

Uh oh!

SemyonSinchenko commented Jul 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

acezen Jul 8, 2024 •

edited

Loading

acezen commented Jul 8, 2024 •

edited

Loading