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

Switch default CPU to use host #1154

Merged
merged 2 commits into from
Nov 29, 2021
Merged

Conversation

keith
Copy link
Member

@keith keith commented May 10, 2021

When building on an Apple Silicon machine, these CPUs would still have
been for x86_64. This flips the default to match the host machine instead.
Users can still pass the platform specific cpu flags to override them.

Closes bazelbuild/bazel#14284.

@google-cla google-cla bot added the cla: yes label May 10, 2021
@keith
Copy link
Member Author

keith commented May 10, 2021

so there's still weird behavior here, and we need this alongside bazelbuild/bazel#13440

With just this change, the path to the output has the right arch in it, but the binary has the wrong arch:

% file bazel-out/applebin_macos-darwin_arm64-fastbuild-ST-fb66f80159e9/bin/examples/macos/CommandLineSwift/CommandLineSwift
bazel-out/applebin_macos-darwin_arm64-fastbuild-ST-fb66f80159e9/bin/examples/macos/CommandLineSwift/CommandLineSwift: Mach-O 64-bit executable x86_64

and without this change, but with that bazel default change you get the right arch but the wrong path:

% file bazel-out/applebin_macos-darwin_x86_64-fastbuild-ST-3790a2650836/bin/examples/macos/CommandLineSwift/CommandLineSwift
bazel-out/applebin_macos-darwin_x86_64-fastbuild-ST-3790a2650836/bin/examples/macos/CommandLineSwift/CommandLineSwift: Mach-O 64-bit executable arm64

When building on an Apple Silicon machine, this CPU would still have
been for x86_64, when you are building a macOS target explicitly, you
can pass `--macos_cpus` (although that can also be inconvenient), but
when the binary is a host binary there wasn't a way to override that.
@keith keith force-pushed the ks/switch-default-macos-cpu-to-use-host branch from ba33d70 to 5d48ea3 Compare November 17, 2021 22:45
@keith keith changed the title Switch default macOS CPU to use host Switch default CPU to use host Nov 17, 2021
@keith
Copy link
Member Author

keith commented Nov 17, 2021

I no longer see the issue mentioned above with 5.0.0rc2

@keith keith marked this pull request as ready for review November 17, 2021 22:47
@lyft-lint-bot
Copy link

Lyft integration job started: https://buildkite.com/lyft/rules-apple/builds/152 (must be Lyft employee to view)

return "ios_x86_64"
if platform_type == "macos":
if cpu:
return "darwin_{}".format(cpu)
macos_cpus = settings["//command_line_option:macos_cpus"]
if macos_cpus:
return "darwin_{}".format(macos_cpus[0])
cpu_value = settings["//command_line_option:cpu"]
if cpu_value.startswith("darwin_"):
Copy link
Member

Choose a reason for hiding this comment

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

On Intel Macs, unless --cpu=darwin_x86_64 is passed, this defaults to darwin.

Suggested change
if cpu_value.startswith("darwin_"):
if cpu_value.startswith("darwin"):

Copy link
Collaborator

Choose a reason for hiding this comment

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

But would we want to return darwin, or fall through to the darwin_x86_64 case? I think we want to preserve old behavior for Intel.

Copy link
Member Author

Choose a reason for hiding this comment

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

hrm yea that's kinda weird either way. I guess in that case we can continue letting it fallthrough as it was before, but we should try to eliminate that ambiguous arch identifier anyways

@brentleyjones
Copy link
Collaborator

brentleyjones commented Nov 29, 2021

I tested this and it works. I think we should merge it in order to fix bazelbuild/bazel#14284.

@keith keith merged commit 99e5b63 into master Nov 29, 2021
@keith keith deleted the ks/switch-default-macos-cpu-to-use-host branch November 29, 2021 18:37
chiragramani added a commit to uber-common/rules_apple that referenced this pull request Feb 2, 2022
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.

--ios_multi_cpus defaults to x86_64 on M1 Macs
5 participants