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.

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

Add darwin_arm64 java_tools #16960

Closed
wants to merge 8 commits into from
Closed

Conversation

hvadehra
Copy link
Member

@hvadehra hvadehra commented Dec 8, 2022

Build a fat universal binary for java_tools_prebuilt on darwin

Work towards: bazelbuild/java_tools#57 and #13944

@hvadehra hvadehra force-pushed the hvadehra-java_tools_darwin_arm64 branch 14 times, most recently from 0432054 to 0d68448 Compare December 13, 2022 12:46
@hvadehra hvadehra force-pushed the hvadehra-java_tools_darwin_arm64 branch from 0d68448 to 0c7c49f Compare December 13, 2022 13:01
@hvadehra hvadehra requested a review from comius December 13, 2022 13:04
@hvadehra hvadehra marked this pull request as ready for review December 13, 2022 13:05
@sgowroji sgowroji added team-Rules-Java Issues for Java rules awaiting-review PR is awaiting review from an assigned reviewer labels Dec 13, 2022
@hvadehra hvadehra force-pushed the hvadehra-java_tools_darwin_arm64 branch 3 times, most recently from a8de756 to 33d4262 Compare December 13, 2022 13:19
meteorcloudy pushed a commit to bazelbuild/continuous-integration that referenced this pull request Dec 13, 2022
java_tools for darwin will be built as a fat universal binary after
bazelbuild/bazel#16960 so it needs to use the
osx toolchain.
@hvadehra hvadehra force-pushed the hvadehra-java_tools_darwin_arm64 branch from 33d4262 to ae0e759 Compare December 13, 2022 14:17
Copy link
Contributor

@comius comius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to build a fat binary and move away from the previously used pattern where 3 prebuilt repositories are released? Technically you should be able to extend for the 4th repository, build for that platform and select the correct prebuilt.

That option looks less complex as the current one, because it doesn't involve additional rule and this makes it probably easier to maintain.

src/darwin_universal_binary.bzl Outdated Show resolved Hide resolved
@@ -0,0 +1,54 @@
def _universal_split_transition_impl(ctx, attr):
return {
"x86_64" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't support platformization, which we'll need to update when we turn it on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also set --platforms flag to match the cpu. This then won't cause problems when we flip --incompatible_enable_cc_toolchain_resolution

@hvadehra
Copy link
Member Author

hvadehra commented Dec 21, 2022

Is there a reason to build a fat binary and move away from the previously used pattern where 3 prebuilt repositories are released? Technically you should be able to extend for the 4th repository, build for that platform and select the correct prebuilt.

That's what I tried first, but it was a more invasive change. If we were to have add a new repo for apple silicon, I think we should rename the existing one to "darwin_intel" or "darwin_x86_64" (leaving it as "darwin" would be misleading/incorrect). So I tried that, and it ended up getting very messy. There are a lot of assumptions in code and tests that there is only a single darwin platform (i.e. platform == OS).

That option looks less complex as the current one, because it doesn't involve additional rule and this makes it probably easier to maintain.

I feel the opposite. Going down that route seems less extensible in general, requiring a lot more updates whenever something changes. At least, not without significant refactoring of all BUILD, BUILD.tools, BUILD.java_tools, etc

@@ -0,0 +1,54 @@
def _universal_split_transition_impl(ctx, attr):
return {
"x86_64" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also set --platforms flag to match the cpu. This then won't cause problems when we flip --incompatible_enable_cc_toolchain_resolution

src/darwin_universal_binary.bzl Outdated Show resolved Hide resolved
@hvadehra hvadehra force-pushed the hvadehra-java_tools_darwin_arm64 branch from ae0e759 to fc931ff Compare February 14, 2023 16:08
Copy link
Contributor

@comius comius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps move dub.bzl to tools/singlejar, so that it's harder to discover.

@hvadehra hvadehra deleted the hvadehra-java_tools_darwin_arm64 branch February 28, 2023 13:06
copybara-service bot pushed a commit that referenced this pull request Mar 15, 2023
*** Reason for rollback ***

Conflicts with #17762

Will instead go with the alternative idea discussed in this PR: #17767

*** Original change description ***

Add darwin_arm64 java_tools

Build a fat universal binary for java_tools_prebuilt on darwin

Work towards: bazelbuild/java_tools#57 and #13944

Closes #16960.

PiperOrigin-RevId: 516799739
Change-Id: I2ff5f615bd7c23e38a334bf836c31ed964443c31
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
Build a fat universal binary for java_tools_prebuilt on darwin

Work towards: bazelbuild/java_tools#57 and bazelbuild#13944

Closes bazelbuild#16960.

PiperOrigin-RevId: 512683379
Change-Id: Ie9db26c729a301fbb22f17dd15065861f3198f57
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
*** Reason for rollback ***

Conflicts with bazelbuild#17762

Will instead go with the alternative idea discussed in this PR: bazelbuild#17767

*** Original change description ***

Add darwin_arm64 java_tools

Build a fat universal binary for java_tools_prebuilt on darwin

Work towards: bazelbuild/java_tools#57 and bazelbuild#13944

Closes bazelbuild#16960.

PiperOrigin-RevId: 516799739
Change-Id: I2ff5f615bd7c23e38a334bf836c31ed964443c31
fmeum pushed a commit to fmeum/continuous-integration that referenced this pull request Dec 10, 2023
java_tools for darwin will be built as a fat universal binary after
bazelbuild/bazel#16960 so it needs to use the
osx toolchain.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Rules-Java Issues for Java rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants