[PATCH] D109839: Add support for targeting macOS arm64 with bazel

Tue Sep 21 11:54:10 PDT 2021

dfm added inline comments.

================
Comment at: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel:21
+# Support for targeting macOS arm64 architecture with `bazel<4.2`. This
+# implementation is lifted from `src/conditions/BUILD` in the bazel source.
+config_setting(
----------------
dfm wrote:
> GMNGeoffrey wrote:
> > dfm wrote:
> > > GMNGeoffrey wrote:
> > > > We already use `@bazel_tools//src/conditions:foo` in config.bzl. Looking at the history of src/conditions/BUILD, there's some bug in Bazel that means that cross-compiling from darwinn x86 to darwinn arm is broken and this is worked around in src/conditions by adding a `--darwin_arm64` flag (https://github.com/bazelbuild/bazel/issues/12655 and https://github.com/bazelbuild/bazel/commit/52457b18bd), but only after Bazel 4.0 (the version we're currently on). Is that cross-compiling something you actually need? If not, I'd prefer we use the src/conditions constraints for now (at least until such a time as we decide to define all our own config settings)
> > > Thanks for your response! Yeah, that's exactly what we're looking to do (I came to this when trying to build binaries for a Python library using x86 hardware), although the existing implementation wouldn't work even when building on an arm machine. I also tried your suggested change (using `@bazel_tools//src/conditions:darwin_arm64`) and I was still getting the wrong compiler flags when building locally, but I'm far from an expert in any of this though so there's a good shout that I was doing something wrong. I understand the hesitation to copy this over and I'd be happy to look into alternative ideas if you have suggestions. Thanks again!
> > Ah in that case, I think we should probably upgrade to fix the Bazel bug :-) (in addition to adding the right selects in config.bzl). I would've done it already except that support for `rbe_autoconfig` was dropped for versions >4.0.0. I posted on bazel-discuss about how that squares with it being an LTS release.
> > 
> > So to clarify, with these edits you can cross-compile from darwin x86_64 to arm? That makes sense to me given the linked issue in Bazel that was worked around. How blocking is this for you? Like can you patch this in locally and then continue on your way for a bit while we fiddle around with using a newer version of Bazel? Can you just build with Bazel >4.2 (ignore the .bazelversion file) after adding the patch to use the src/conditions configs? I think the default Bazel install these days has a shell wrapper that automatically uses the version in the .bazelrc file, but if you specifically use a bazel-4.2 binary then it should work.
> That sounds like a good plan. It's definitely not a major blocker, so definitely no pressure. I'll play around with bazel versions and confirm my claims about cross-compiling tomorrow, just to be sure. Thanks!
Just a quick update on this. I've done some more playing around and I can confirm that cross-compiling from darwin x86_64 to arm works with bazel version 4.2.1 (and probably others, but that's what I tested) using only the proposed change to `config.bzl`, without introducing these config settings. Is there a reason to not update the conditions to `darwin_arm64` and `darwin_x86_64` instead of plain `darwin`?

(Side note: I couldn't get any of this to work with the `--cpu=darwin_arm64` and needed to use the `--platforms` instead; I remain confused by this, but maybe this is the expected behavior.)

================
Comment at: utils/bazel/llvm-project-overlay/llvm/config.bzl:78-79
     "@bazel_tools//src/conditions:windows": native_arch_defines("X86", "x86_64-pc-win32"),
-    "@bazel_tools//src/conditions:darwin": native_arch_defines("X86", "x86_64-unknown-darwin"),
+    "//llvm:macos_arm64": native_arch_defines("AArch64", "arm64-apple-darwin"),
+    "//llvm:macos_x86_64": native_arch_defines("X86", "x86_64-apple-darwin"),
     "@bazel_tools//src/conditions:linux_aarch64": native_arch_defines("AArch64", "aarch64-unknown-linux-gnu"),
----------------
GMNGeoffrey wrote:
> 
This should be

```
+     "@bazel_tools//src/conditions:darwin_arm64": native_arch_defines("AArch64", "arm64-apple-darwin"),
+     "@bazel_tools//src/conditions:darwin_x86_64": native_arch_defines("X86", "x86_64-apple-darwin"),
```

(rather than `macos`) but, otherwise, it looks like this would be a sufficient change to support both platforms.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109839/new/

https://reviews.llvm.org/D109839