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

Remove armeabi-v7a from Apple crosstool #13449

Conversation

keith
Copy link
Member

@keith keith commented May 8, 2021

This is still needed in general, but instead of baking it into this crosstool we can use the separate setup for it.

@google-cla google-cla bot added the cla: yes label May 8, 2021
@keith keith force-pushed the ks/remove-armeabi-v7a-from-apple-crosstool branch 2 times, most recently from 8394eb7 to b1c19f1 Compare May 8, 2021 19:52
@keith
Copy link
Member Author

keith commented May 8, 2021

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

# Android tooling requires a default toolchain for the armeabi-v7a cpu.

@keith keith force-pushed the ks/remove-armeabi-v7a-from-apple-crosstool branch 2 times, most recently from bd096d1 to 57fe560 Compare May 8, 2021 20:29
"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"],
}
Copy link
Member Author

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"]
Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@keith keith force-pushed the ks/remove-armeabi-v7a-from-apple-crosstool branch from 57fe560 to 71b948f Compare May 8, 2021 20:42
@keith keith force-pushed the ks/remove-armeabi-v7a-from-apple-crosstool branch from 71b948f to c5a3f87 Compare May 11, 2021 19:32
@keith keith marked this pull request as ready for review May 11, 2021 19:32
@keith keith force-pushed the ks/remove-armeabi-v7a-from-apple-crosstool branch from c5a3f87 to dc92d48 Compare May 11, 2021 19:33
Copy link
Contributor

@trybka trybka left a 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!

tools/osx/crosstool/cc_toolchain_config.bzl Outdated Show resolved Hide resolved
@@ -183,6 +145,9 @@ def _impl(ctx):
else:
fail("Unreachable")

host_system_name = "x86_64-apple-macosx"
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

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?

@keith keith force-pushed the ks/remove-armeabi-v7a-from-apple-crosstool branch from dc92d48 to bf05326 Compare May 11, 2021 20:11
abi_version = "armeabi-v7a"
elif (ctx.attr.cpu == "darwin_x86_64"):
if ctx.attr.cpu == "darwin_x86_64":
abi_libc_version = "darwin_x86_64"
Copy link
Member Author

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

@bazel-io bazel-io closed this in a7e0e29 May 21, 2021
@keith keith deleted the ks/remove-armeabi-v7a-from-apple-crosstool branch May 21, 2021 01:29
@trybka
Copy link
Contributor

trybka commented May 27, 2021

@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

keith added a commit to keith/rules_cc that referenced this pull request May 27, 2021
@keith
Copy link
Member Author

keith commented May 27, 2021

Thanks! bazelbuild/rules_cc#105

@meteorcloudy
Copy link
Member

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?

@keith
Copy link
Member Author

keith commented Dec 15, 2021

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

@keith
Copy link
Member Author

keith commented Jan 4, 2022

@meteorcloudy this is fixed on tensorflow HEAD now, thanks for pinging this!

keith added a commit to keith/bazel that referenced this pull request Feb 20, 2022
I'm curious if the hard NDK requirement changes makes this no longer
necessary

Previous context: bazelbuild#13449
ClusterH pushed a commit to ClusterH/tensorflow_source that referenced this pull request May 22, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants