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.

Sign up for GitHub

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

Jump to bottom

[Java] Cleanup protobuf-maven-plugin usage #40843

Closed
laurentgo opened this issue Mar 27, 2024 · 1 comment
Closed

[Java] Cleanup protobuf-maven-plugin usage #40843

laurentgo opened this issue Mar 27, 2024 · 1 comment
Assignees
@laurentgo
Labels
Component: Java Type: enhancement
Milestone
16.0.0

Comments

@laurentgo
Copy link
Collaborator

Describe the enhancement requested

Arrow uses protobuf-maven-plugin in several modules but not in the recommended way. There are multiple issues like:

  • use of compile/compile-custom instead of test-compile/test-compile-custom for test protobufs (causing test classes to be added to the main artifact)
  • forcing the output location which causes different execution to overwrite each other (each execution should have their own output directory). When done properly, setting clearOutputDirectory property to false is not necessary anymore
  • adding test-compile goal when no test protobufs are present.

The protobuf-maven-plugin is also declared multiple times instead of it managed at the top level

Component(s)

Java
laurentgo added a commit to laurentgo/arrow that referenced this issue Mar 27, 2024
@laurentgo
apacheGH-40843: [Java] Cleanup protobuf-maven-plugin usage
a1b7651 
* Add protobuf-maven-plugin plugin to top level pom.xml under
  pluginManagement to define version and common configuration for all
  modules
* Remove unnecessary executions of test-compile goal when no test
  protobufs are present
* Remove use of outputDirectory and clearOutputDirectory and let the plugin
  choose it for each execution (the default output directory is based on
  the phase (main vs test) and the language/plugin-id)
* Replace use of compile/compile-custom goals with
  test-compile/test-compile-custom when generating test protobufs
@github-actions github-actions bot mentioned this issue Mar 27, 2024
Merged
lidavidm pushed a commit that referenced this issue Mar 28, 2024
@laurentgo
GH-40843: [Java] Cleanup protobuf-maven-plugin usage (#40844)
a9b2cc2 
### Rationale for this change

`protobuf-maven-plugin` usage in Arrow codebase does not follow plugins best practices like sharing the same output directory for different execution or not using test goals for generating test classes

### What changes are included in this PR?

* Add protobuf-maven-plugin plugin to top level pom.xml under pluginManagement to define version and common configuration for all modules
* Remove unnecessary executions of test-compile goal when no test protobufs are present
* Remove use of outputDirectory and clearOutputDirectory and let the plugin choose it for each execution (the default output directory is based on the phase (main vs test) and the language/plugin-id)
* Replace use of compile/compile-custom goals with test-compile/test-compile-custom when generating test protobufs

### Are these changes tested?

As those changes are in the build system, they are covered by the build framework and tests run as part of the build

### Are there any user-facing changes?
None
* GitHub Issue: #40843

Authored-by: Laurent Goujon <[email protected]>
Signed-off-by: David Li <[email protected]>
@lidavidm lidavidm added this to the 16.0.0 milestone Mar 28, 2024
@lidavidm
Copy link
Member

Issue resolved by pull request 40844
#40844

tolleybot pushed a commit to tmct/arrow that referenced this issue May 2, 2024
@laurentgo @tolleybot
apacheGH-40843: [Java] Cleanup protobuf-maven-plugin usage (apache#40844
d6c52fe 
)

### Rationale for this change

`protobuf-maven-plugin` usage in Arrow codebase does not follow plugins best practices like sharing the same output directory for different execution or not using test goals for generating test classes

### What changes are included in this PR?

* Add protobuf-maven-plugin plugin to top level pom.xml under pluginManagement to define version and common configuration for all modules
* Remove unnecessary executions of test-compile goal when no test protobufs are present
* Remove use of outputDirectory and clearOutputDirectory and let the plugin choose it for each execution (the default output directory is based on the phase (main vs test) and the language/plugin-id)
* Replace use of compile/compile-custom goals with test-compile/test-compile-custom when generating test protobufs

### Are these changes tested?

As those changes are in the build system, they are covered by the build framework and tests run as part of the build

### Are there any user-facing changes?
None
* GitHub Issue: apache#40843

Authored-by: Laurent Goujon <[email protected]>
Signed-off-by: David Li <[email protected]>
tolleybot pushed a commit to tmct/arrow that referenced this issue May 4, 2024
@laurentgo @tolleybot
apacheGH-40843: [Java] Cleanup protobuf-maven-plugin usage (apache#40844
0864e9e 
)

### Rationale for this change

`protobuf-maven-plugin` usage in Arrow codebase does not follow plugins best practices like sharing the same output directory for different execution or not using test goals for generating test classes

### What changes are included in this PR?

* Add protobuf-maven-plugin plugin to top level pom.xml under pluginManagement to define version and common configuration for all modules
* Remove unnecessary executions of test-compile goal when no test protobufs are present
* Remove use of outputDirectory and clearOutputDirectory and let the plugin choose it for each execution (the default output directory is based on the phase (main vs test) and the language/plugin-id)
* Replace use of compile/compile-custom goals with test-compile/test-compile-custom when generating test protobufs

### Are these changes tested?

As those changes are in the build system, they are covered by the build framework and tests run as part of the build

### Are there any user-facing changes?
None
* GitHub Issue: apache#40843

Authored-by: Laurent Goujon <[email protected]>
Signed-off-by: David Li <[email protected]>
rok pushed a commit to tmct/arrow that referenced this issue May 8, 2024
@laurentgo @rok
apacheGH-40843: [Java] Cleanup protobuf-maven-plugin usage (apache#40844
bc559dd 
)

### Rationale for this change

`protobuf-maven-plugin` usage in Arrow codebase does not follow plugins best practices like sharing the same output directory for different execution or not using test goals for generating test classes

### What changes are included in this PR?

* Add protobuf-maven-plugin plugin to top level pom.xml under pluginManagement to define version and common configuration for all modules
* Remove unnecessary executions of test-compile goal when no test protobufs are present
* Remove use of outputDirectory and clearOutputDirectory and let the plugin choose it for each execution (the default output directory is based on the phase (main vs test) and the language/plugin-id)
* Replace use of compile/compile-custom goals with test-compile/test-compile-custom when generating test protobufs

### Are these changes tested?

As those changes are in the build system, they are covered by the build framework and tests run as part of the build

### Are there any user-facing changes?
None
* GitHub Issue: apache#40843

Authored-by: Laurent Goujon <[email protected]>
Signed-off-by: David Li <[email protected]>
rok pushed a commit to tmct/arrow that referenced this issue May 8, 2024
@laurentgo @rok
apacheGH-40843: [Java] Cleanup protobuf-maven-plugin usage (apache#40844
8ed961f 
)

### Rationale for this change

`protobuf-maven-plugin` usage in Arrow codebase does not follow plugins best practices like sharing the same output directory for different execution or not using test goals for generating test classes

### What changes are included in this PR?

* Add protobuf-maven-plugin plugin to top level pom.xml under pluginManagement to define version and common configuration for all modules
* Remove unnecessary executions of test-compile goal when no test protobufs are present
* Remove use of outputDirectory and clearOutputDirectory and let the plugin choose it for each execution (the default output directory is based on the phase (main vs test) and the language/plugin-id)
* Replace use of compile/compile-custom goals with test-compile/test-compile-custom when generating test protobufs

### Are these changes tested?

As those changes are in the build system, they are covered by the build framework and tests run as part of the build

### Are there any user-facing changes?
None
* GitHub Issue: apache#40843

Authored-by: Laurent Goujon <[email protected]>
Signed-off-by: David Li <[email protected]>
vibhatha pushed a commit to vibhatha/arrow that referenced this issue May 25, 2024
@laurentgo @vibhatha
apacheGH-40843: [Java] Cleanup protobuf-maven-plugin usage (apache#40844
b13a7d8 
)

### Rationale for this change

`protobuf-maven-plugin` usage in Arrow codebase does not follow plugins best practices like sharing the same output directory for different execution or not using test goals for generating test classes

### What changes are included in this PR?

* Add protobuf-maven-plugin plugin to top level pom.xml under pluginManagement to define version and common configuration for all modules
* Remove unnecessary executions of test-compile goal when no test protobufs are present
* Remove use of outputDirectory and clearOutputDirectory and let the plugin choose it for each execution (the default output directory is based on the phase (main vs test) and the language/plugin-id)
* Replace use of compile/compile-custom goals with test-compile/test-compile-custom when generating test protobufs

### Are these changes tested?

As those changes are in the build system, they are covered by the build framework and tests run as part of the build

### Are there any user-facing changes?
None
* GitHub Issue: apache#40843

Authored-by: Laurent Goujon <[email protected]>
Signed-off-by: David Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@laurentgo laurentgo

Labels
Component: Java Type: enhancement
Projects
None yet
Milestone
16.0.0
Development

No branches or pull requests
2 participants
@lidavidm @laurentgo