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.

Sign up for GitHub

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

Fix objc transition for cc_binary -- smallest change #19235

Closed
wants to merge 1 commit into from
Closed

Fix objc transition for cc_binary -- smallest change #19235

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/main/starlark/builtins_bzl/common/objc/transitions.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ def _determine_single_architecture(platform_type, settings):
return DEFAULT_TVOS_CPU
return tvos_cpus[0]
if platform_type == MACOS:
macos_cpus = settings["//command_line_option:macos_cpus"]
if macos_cpus:
return macos_cpus[0]
cpu_value = settings["//command_line_option:cpu"]
Copy link
Member

Choose a reason for hiding this comment

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

doesn't CPU always have a value that would start with darwin? (unless you explicitly specified otherwise) as in if you passed --macos_cpus=arm64,x86_64 --cpu=darwin_x86_64 it should build a fat binary for both archs but with this change I don't think it would?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Keith! Good Q. Glad I get to engage with you on this.

Here's my reasoning--and I'd love it if you'd check me. Two cases:

  1. Built under a rules_apple top-level/binary target -> same fat binary behavior as before
    Why? binary target does split cpu transition on e.g. --ios_multi_cpus or --macos_cpus -> triggers picking up the CPU from apple_split_cpu in the first lines of this function, _determine_single_architecture, returning early and bypassing all the if statements around this diff.
    Hence no change; you still get your fat binary with architectures controlled by, e.g., --macos_cpus.
  2. Not configured to be built as a fat binary by a higher-level rules_apple target -> we're traversing these cases, trying to determine an architecture to fall back on.
    Current bazel behavior is, I believe, producing a single-arch archive (sometimes w/ the wrong architecture), rather than a fat binary. That's what I'm seeing in testing, and also what I'd expect from the existing code; it's only returning a single CPU.
    (Now, like you, I'm not so sure the current behavior is ideally intuitive, but that's a different issue--and not one I'm trying to tackle with this intentionally minimal fix, doubly since incoming transitions like this are constrained to be 1:1 and Bazel's general binary-vs-library MO.)

To sum the above: I think we have have the same fat binary behavior as before.

(What this does change is, in the single, thin output case, where we hit this code and aren't honoring multiple cpus anyway, which architecture gets chosen as a fallback. The change makes bazel honor the single cpu flag, whether defaulted to the current machine, configured, or specified on the command line (unless the cpu has been set to some obviously invalid value that's not for Mac (doesn't start with darwin), at which point we go down the fallbacks as before). Goal here was minimal change that made objc_library work correctly with the core cc-ecosystem while maintaining reasonable defaults and configurability for the other common use cases.)

if cpu_value.startswith(DARWIN_CPU_PREFIX):
return cpu_value[len(DARWIN_CPU_PREFIX):]
macos_cpus = settings["//command_line_option:macos_cpus"]
if macos_cpus:
return macos_cpus[0]
return DEFAULT_MACOS_CPU
if platform_type == CATALYST:
catalyst_cpus = settings["//command_line_option:catalyst_cpus"]
Expand Down