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

Add support for Apple Silicon (arm64) binaries #46

Merged
merged 5 commits into from
Mar 18, 2022

Conversation

amatosov-rbx
Copy link
Contributor

Include architecture name in macOS platform keywords
Add arm64 foreman build

Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

Looks like a reasonable approach! After this lands I'll get a PR going to bump the version, then we can cut a release, and publish to crates.io.

static PLATFORM_KEYWORDS: &[&str] = &["macos-x86_64", "darwin-x86_64", "macos", "darwin"];

#[cfg(all(target_os = "macos", target_arch = "aarch64"))]
static PLATFORM_KEYWORDS: &[&str] = &["macos-arm64", "darwin-arm64", "macos-x86_64", "darwin-x86_64", "macos", "darwin"];
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming some of these won't work for M1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

x86_64 apps work with Rosetta2 (with caveats), which most users will have installed, because we are in the transitory period right now https://support.apple.com/en-us/HT211861
The primary caveat is that dependencies installation can be tricky for x86_64. E.g. if x86_64 binary depends on OpenSSL, user would need to install x86_64 version of OpenSSL.
Thus we should prefer to use arm binaries.
If x86_64 binary has no runtime dependecies, it should work just fine if Rosetta2 is installed

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case I think it's all good because foreman will try to find the first asset in that list, which will make it prioritize the correct specific binaries. Thanks for your contribution 👍

Copy link
Contributor

@oltrep oltrep left a comment

Choose a reason for hiding this comment

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

Can you add an entry to the changelog? Then it's good to go :shipit:

@ZoteTheMighty ZoteTheMighty merged commit 75bea11 into Roblox:master Mar 18, 2022
@amatosov-rbx amatosov-rbx deleted the feature/appleSiliconArchs branch May 2, 2022 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants