-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-38264: [Java][Packaging] Add BOM file #38336
Conversation
Thanks @jduo ! I'm not a BOM file expert, does it make sense to also pin Arrow's 3rd party deps in here as well? And is it possible to pin 3rd party deps based on JDK version? (Java 8 support requires pinning some deps to outdated versions e.g. Mockito). Maybe a follow up PR would be better to add this, though. |
I took a look at BOMs for a few projects:
and they all only included internally-built modules in their BOM files rather than also including 3rd party dependencies. This sort of makes sense though, if the BOM is intended to describe what libraries are being shipped. |
Makes sense to me, I'm good with the PR as-is then. LGTM. Thanks! |
The release script is failing though. I'm not quite sure what the issue is yet. Seems to be complaining about |
Perhaps there's a list of files that a versioning script updates that needs to be updated to include the BOM pom. |
I see the problem. In utils-prepare.sh at https://github.com/jduo/arrow/blob/4c329d0da2ec5ce7d24134ac568ec40619358c08/dev/release/utils-prepare.sh#L68C3-L68C20 , the script uses mvn versions:set on the parent pom, which cascades to all child modules. However the Bill of Materials POM is explicitly not a child, but rather a side-module that the parent imports. The solution is either to run mvn versions:set -DprocessAllModules which includes updating any aggregated modules, or to simply also run against the BOM file directly. |
New packages also need to be added somewhere in ci/ so that they actually get released |
Append all the new artifacts here Lines 930 to 942 in 02ad5ae
|
@github-actions crossbow submit java |
Revision: 67cef64 Submitted crossbow builds: ursacomputing/crossbow @ actions-4c0762070e |
the java-jars job is the important one for the release as those artifact are used for the release. Looking at the raw log it seems the bom module is not being build in the final step? https://github.com/ursacomputing/crossbow/actions/runs/6579276168/job/17875990318#step:6:11 |
It looks like maven does build it: |
<parent> | ||
<groupId>org.apache</groupId> | ||
<artifactId>apache</artifactId> | ||
<version>18</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using apache 30 here ?
FYI, I'm working on a PR to have reproducible build (with several deps and plugins updates).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess because that's what our root POM uses still:
Lines 15 to 19 in 67cef64
<parent> | |
<groupId>org.apache</groupId> | |
<artifactId>apache</artifactId> | |
<version>18</version> | |
</parent> |
I don't think we have any reason to stick to it if there's a much newer version, but if there are potential major changes we could open a separate PR to bump things afterwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point :)
I propose to keep apache 18 in this PR.
I will do another PR to upgrade to apache 30 (coupling with other updates as said in my previous comment, I should be able to submit several PRs tonight or tomorrow my time 😄 ).
Thanks !
@github-actions crossbow submit java |
|
@github-actions crossbow submit java |
|
|
@github-actions crossbow submit java |
Revision: f526fd8 Submitted crossbow builds: ursacomputing/crossbow @ actions-765ba35b65 |
@danepitkin @lidavidm @assignUser this PR is now green on crossbow. The issue was that I had the artifact group set to org.apache instead of org.apache.arrow and one of the CI scripts was only copying artifacts installed to ~/.m2/org/apache/arrow. I've checked the BOM artifact to use org.apache.arrow now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I'll wait for someone with java background to merge :)
java/bom/pom.xml
Outdated
<dependency> | ||
<groupId>org.apache.arrow</groupId> | ||
<artifactId>arrow-vector</artifactId> | ||
<version>${project.version}</version> | ||
<type>test-jar</type> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still a test-jar vector dependency here, I think we don't want to re-export this to other projects outside of Arrow
Add Bill of Materials to simplify dependency management. Utilize the BOM internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 006f387. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
### Rationale for this change Provide an easy way for users to import Arrow maven packages. ### What changes are included in this PR? Add a BOM module and utilize this module internally for importing Arrow dependencies. ### Are these changes tested? Yes. ### Are there any user-facing changes? Users can import the BOM dependency now. * Closes: apache#38264 Authored-by: James Duong <[email protected]> Signed-off-by: David Li <[email protected]>
### Rationale for this change Provide an easy way for users to import Arrow maven packages. ### What changes are included in this PR? Add a BOM module and utilize this module internally for importing Arrow dependencies. ### Are these changes tested? Yes. ### Are there any user-facing changes? Users can import the BOM dependency now. * Closes: apache#38264 Authored-by: James Duong <[email protected]> Signed-off-by: David Li <[email protected]>
### Rationale for this change Provide an easy way for users to import Arrow maven packages. ### What changes are included in this PR? Add a BOM module and utilize this module internally for importing Arrow dependencies. ### Are these changes tested? Yes. ### Are there any user-facing changes? Users can import the BOM dependency now. * Closes: apache#38264 Authored-by: James Duong <[email protected]> Signed-off-by: David Li <[email protected]>
Rationale for this change
Provide an easy way for users to import Arrow maven packages.
What changes are included in this PR?
Add a BOM module and utilize this module internally for importing Arrow dependencies.
Are these changes tested?
Yes.
Are there any user-facing changes?
Users can import the BOM dependency now.