Skip to content

Detect ARM macOS arch with sysctl hw.optional.arm64 #67970

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

Jump to bottom
Merged
jmagman merged 6 commits into from
Oct 13, 2020
Merged

Detect ARM macOS arch with sysctl hw.optional.arm64 #67970

jmagman merged 6 commits into from
Oct 13, 2020

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Oct 12, 2020 •
edited
Loading

Description

Detect whether we're running on an x86 or ARM Mac. uname -m returns x86_64 on an ARM Mac from flutter since it's running in Rosetta.

Instead, switch to sysctl hw.optional.arm64, which returns sysctl hw.optional.arm64: 1 on an ARM mac in both Rosetta and non-Rosetta mode. On non-ARM this command fails sysctl: unknown oid 'hw.optional.arm64' and and exits 1.

Create a new HostPlatform.darwin_arm value, and expose it in OperatingSystemUtils.

Related Issues

#65976

Tests

Added a test for x86 and ARM.

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@jmagman jmagman added the platform-ios iOS applications specifically label Oct 12, 2020
@jmagman jmagman requested a review from jonahwilliams October 12, 2020 23:19
@jmagman jmagman self-assigned this Oct 12, 2020
@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 12, 2020
@google-cla google-cla bot added the cla: yes label Oct 12, 2020
@jmagman
Copy link
Member Author

jmagman commented Oct 12, 2020

This is also how homebrew decided to do it:
https://github.com/Homebrew/brew/blob/2b4f3646b7a19c0690c017d67412ab59da0c8b72/Library/Homebrew/extend/os/mac/hardware/cpu.rb#L123

jonahwilliams
jonahwilliams approved these changes Oct 12, 2020
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM, nice!

One potential follow up is to expose this in a way that we can use for host architecture detection, and then prevent flutter run and redirect to some canned issue or documentation.

@jmagman
Copy link
Member Author

jmagman commented Oct 13, 2020

LGTM, nice!

One potential follow up is to expose this in a way that we can use for host architecture detection, and then prevent flutter run and redirect to some canned issue or documentation.

At the point we can detect the physical architecture, I think we can prepend /usr/bin/arch -arm64 and the commands will run natively, not in Rosetta. But yeah, we maybe need to bail for the simulator and tell users to run from Xcode.

jmagman
jmagman commented Oct 13, 2020
View reviewed changes
packages/flutter_tools/lib/src/base/os.dart Outdated
/// Physical underlying architecture.
///
/// On ARM return arm64, even when this process is running in Rosetta.
DarwinArch get hardwareArchitecture {
Copy link
Member Author

@jmagman jmagman Oct 13, 2020

Choose a reason for hiding this comment

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

Per your suggestion @jonahwilliams I'm going to think one step ahead--this will be needed outside just the name to detect how certain commands will need to be run.

Created a new MacOSUtils (not _MacOSUtils since I'll want to grab the arch from other places) and expose the arch.

WDYT?

Copy link
Contributor

@jonahwilliams jonahwilliams Oct 13, 2020

Choose a reason for hiding this comment

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

It would be nice to not need to know the specific subtype of os utils in order to call this function.

Instead of exposing a DarwinArch (since that is really a target architecture, we can't target 32bit arm), you could add darwin_arm64 something to the HostPlatform (

flutter/packages/flutter_tools/lib/src/build_info.dart

Line 399 in 345188d

enum HostPlatform {
) And then have a getter on os utils to return the current host platform. If we eventually support linux arm and windows arm this will come in handy too.

You could then augment that with a helper/extension for isArm/isX86

Copy link
Member Author

@jmagman jmagman Oct 13, 2020

Choose a reason for hiding this comment

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

Instead of exposing a DarwinArch (since that is really a target architecture, we can't target 32bit arm), you could add darwin_arm64 something to the HostPlatform

That's way better.

jonahwilliams
jonahwilliams approved these changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

jonahwilliams
jonahwilliams reviewed Oct 13, 2020
View reviewed changes
packages/flutter_tools/lib/src/artifacts.dart Outdated
HostPlatform _currentHostPlatformAsHost(Platform platform) {
if (platform.isMacOS) {
return HostPlatform.darwin_x64;
return HostPlatform.darwin_x86;
Copy link
Contributor

@jonahwilliams jonahwilliams Oct 13, 2020

Choose a reason for hiding this comment

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

I think renaming this is going to inadvertently break some things

Copy link
Member Author

@jmagman jmagman Oct 13, 2020

Choose a reason for hiding this comment

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

CI agrees.

@jmagman jmagman merged commit 4dffc85 into flutter:master Oct 13, 2020
@jmagman jmagman deleted the arch-check branch October 13, 2020 19:05
This was referenced Oct 27, 2020
Closed
Closed
jmagman added a commit to jmagman/flutter that referenced this pull request Oct 28, 2020
@jmagman
Detect ARM macOS arch with sysctl hw.optional.arm64 (flutter#67970)
8f36667
@jmagman jmagman mentioned this pull request Oct 28, 2020
Closed
@jmagman jmagman mentioned this pull request Nov 12, 2020
Merged
@jmagman jmagman mentioned this pull request Dec 3, 2020
Closed
@jmagman jmagman added the platform-host-arm Building on an ARM-based platform label Aug 20, 2021
This was referenced Apr 4, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmagman jmagman

Labels

platform-host-arm Building on an ARM-based platform platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jmagman @jonahwilliams