-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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-43377: [Java][CI] Java-Jars CI is Failing with a linking error on macOS #43385
Conversation
@github-actions crossbow submit -g java |
|
Revision: 02eb021 Submitted crossbow builds: ursacomputing/crossbow @ actions-5dda3b0a91 |
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 reviewed it quickly. Thank you for looking into the issue caused by my PR.
Additionally, according to the link, it seems that the I think this should be addressed beyond just Java. Should I create an issue for this? |
That would be wonderful @llama90
|
FYI: #43386 (comment) |
@github-actions crossbow submit -g java java-jars |
Revision: 4b467bd Submitted crossbow builds: ursacomputing/crossbow @ actions-9387c4cfe5 |
@github-actions crossbow submit java-jars |
Revision: 4b467bd Submitted crossbow builds: ursacomputing/crossbow @ actions-834fce646a
|
I don't think changing For example, in https://github.com/ursacomputing/crossbow/actions/runs/10055540527/job/27792410574:
@kou by chance do you know what might be happening here? |
@danepitkin I was merely testing an idea, in the time period the change happened the direct change I could find was the change in the CIs. And I agree, the issue is with the test library which cannot be linked. |
It seems that this was happen since 2024-07-16: https://github.com/ursacomputing/crossbow/actions/runs/9953458018/job/27502447467 We may be able to find a cause of this by diffing these logs. |
@kou I looked into the history of merged PRs and sort of tracked from where it started occurring. It seems like we haven't made significant changes to the build or C++ components from Java PRs. At this point #43109 (comment) the |
@github-actions crossbow submit -g cpp |
Just saw this filed #43400, this is the same issue. |
Revision: 4b467bd Submitted crossbow builds: ursacomputing/crossbow @ actions-214fbb71e3 |
@github-actions crossbow submit -g java |
Revision: 5879052 Submitted crossbow builds: ursacomputing/crossbow @ actions-0883833aa3 |
@github-actions crossbow submit java-jars |
Revision: 51f0764 Submitted crossbow builds: ursacomputing/crossbow @ actions-04d5e09e65
|
@github-actions crossbow submit java-jars |
Revision: 85c1244 Submitted crossbow builds: ursacomputing/crossbow @ actions-6f3ab99e36
|
@github-actions crossbow submit -g java |
Revision: 10c59a8 Submitted crossbow builds: ursacomputing/crossbow @ actions-96d6d76c00 |
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, amazing work! I have always used bundled GTest when developing on MacOS, so I think this is good to include in our CI.
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 9174bb7. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
@@ -83,7 +83,7 @@ jobs: | |||
- { runs_on: ["macos-13"], arch: "x86_64"} | |||
- { runs_on: ["macos-14"], arch: "aarch_64" } | |||
env: | |||
MACOSX_DEPLOYMENT_TARGET: "10.15" | |||
MACOSX_DEPLOYMENT_TARGET: "14.0" |
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.
@vibhatha That's quite the bump, I am pretty sure we also want to support less recent macos.
Specify the minimum version of the target platform (e.g. macOS or iOS) on which the target binaries are to be deployed.
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.
Ah, my bad for merging this quickly. I was too eager to get java-jars working again. Let's revert that part of the diff since it's not necessary.
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 was too eager to get java-jars working again.
No worry, it's a good goal^^ We can likely bump it to 11 or even 12 (as 11 is also eol... but we don't have a support policy so...)
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.
@assignUser this was an artifact from my previous failed attempt to fix this issue. I have not reverted that. But I will revert it shortly.
|
||
# We want to use the bundled googletest for static linking. Since | ||
# both BUNDLED and brew options are enabled, it could cause a conflict | ||
# when there is a version mismatch. | ||
# We uninstall googletest to ensure using the bundled googletest. | ||
brew uninstall googletest |
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.
Could you move this part before brew bundle --file=arrow/java/Brewfile
?
Because googletest
is in cpp/Brewfile
not java/Brewfile
.
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.
Sure @kou I will make a follow up PR shortly.
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.
Rationale for this change
For
googletest
, we have installation via BUNDLED and brew, a version mismatch from one of these options could cause conflicts and linking related issues. Preferring BUNDLED version, this PR uninstalls the brew installation ofgoogletest
.What changes are included in this PR?
Removing brew installation of
googletest
Are these changes tested?
Yes.
Are there any user-facing changes?
No