-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Remove armeabi-v7a from Apple crosstool #13449
Remove armeabi-v7a from Apple crosstool #13449
Conversation
8394eb7
to
b1c19f1
Compare
example failure if you remove this https://buildkite.com/bazel/bazel-bazel-github-presubmit/builds/8780#9ad3b275-6f59-48f9-b0d7-38c4a6793694 there's also this comment Line 98 in 8a42645
|
bd096d1
to
57fe560
Compare
"watchos_armv7k": ["@platforms//os:ios", "@platforms//cpu:arm"], | ||
"watchos_arm64_32": ["@platforms//os:ios", "@platforms//cpu:aarch64"], | ||
"tvos_arm64": ["@platforms//os:ios", "@platforms//cpu:aarch64"], | ||
} |
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.
This was only used in 1 file so I moved it there since it also slightly diffs now in keys vs the archs lists
@@ -19,5 +39,6 @@ OSX_DEVELOPER_PLATFORM_CPUS = [ | |||
toolchain = "@local_config_cc//:cc-compiler-" + arch, | |||
toolchain_type = "@bazel_tools//tools/cpp:toolchain_type", | |||
) | |||
for arch in OSX_TOOLS_ARCHS for cpu in OSX_DEVELOPER_PLATFORM_CPUS | |||
for arch in OSX_TOOLS_ARCHS + ["armeabi-v7a"] |
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.
we add this manually here so we can exclude it from the OSX_TOOLS_ARCHS
so it doesn't get an autocreated toolchain in the other loops, but we still need to reference the manually created one here, so we keep it in the OSX_TOOLS_CONSTRAINTS
dictionary
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? Does something reference it?
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.
Yep. Without this you get this error when running android tests https://buildkite.com/bazel/bazel-bazel-github-presubmit/builds/8780#9ad3b275-6f59-48f9-b0d7-38c4a6793694
57fe560
to
71b948f
Compare
71b948f
to
c5a3f87
Compare
c5a3f87
to
dc92d48
Compare
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.
Overall much cleaner, thanks!
@@ -183,6 +145,9 @@ def _impl(ctx): | |||
else: | |||
fail("Unreachable") | |||
|
|||
host_system_name = "x86_64-apple-macosx" |
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.
Apologies if this is addressed elsewhere, but what about the arm variant? Or is that not supported yet?
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.
do you know how this value is actually used? it was hardcoded before so i guess it doesn't entirely matter, but yea I was thinking the same thing
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.
It looks like it is now vestigial. https://cs.opensource.google/search?q=lang:java%20host_system_name%20-f:Test.java&sq=&ss=bazel
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.
Nice, I can remove this entirely (probably separately) but that might be a breaking change?
dc92d48
to
bf05326
Compare
abi_version = "armeabi-v7a" | ||
elif (ctx.attr.cpu == "darwin_x86_64"): | ||
if ctx.attr.cpu == "darwin_x86_64": | ||
abi_libc_version = "darwin_x86_64" |
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 also wonder if this condition should be updated for Apple Silicon
@keith it looks like this may have broken some things in rules_cc: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2047#9b7530e6-d708-4852-938d-321d0af6e90c |
This is required since bazelbuild/bazel#13449
Thanks! bazelbuild/rules_cc#105 |
TensorFlow is failing with Bazel@HEAD due to this change: https://buildkite.com/bazel/bazel-at-head-plus-disabled/builds/1143#a46ef18c-b4dc-43c1-9118-4a7620e72c1a Any idea how to fix it? |
Welp, looks like they use rules_cc's toolchain https://github.com/tensorflow/tensorflow/blob/cbc1ddc5b52ac13a5d2563f7b5661e72173d7084/tensorflow/workspace0.bzl#L106 but also reference some files from this repo, so there is an incompatibility here. I've submitted tensorflow/tensorflow#53440 to just drop rules_cc |
@meteorcloudy this is fixed on tensorflow HEAD now, thanks for pinging this! |
I'm curious if the hard NDK requirement changes makes this no longer necessary Previous context: bazelbuild#13449
This public repository is seemingly dead as far as using it as a replacement cc toolchain. Because of this, and tensorflow's use of some files under @bazel_tools, this can lead to incompatibilities bazelbuild/bazel#13449 (comment) Since this repo isn't active, and doesn't provide anything over the default bazel infra that tf needs, we can just drop it.
This is still needed in general, but instead of baking it into this crosstool we can use the separate setup for it.