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

feat: add app.runningUnderRosettaTranslation to detect running under rosetta #26444

Merged
merged 4 commits into from
Nov 13, 2020

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Nov 11, 2020

As in title and notes, simple API, code is straight from apples docs.

Docs reference for rosetta detection: https://developer.apple.com/documentation/apple_silicon/about_the_rosetta_translation_environment?language=objc#3616845

Notes: Added new app.runningUnderRosettaTranslation property to detect when running under rosetta on Apple silicon

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 11, 2020
docs/api/app.md Outdated Show resolved Hide resolved
shell/browser/api/electron_api_app.cc Outdated Show resolved Hide resolved
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Nov 12, 2020
@MarshallOfSound MarshallOfSound changed the title feat: add app.isRunningUnderRosettaTranslation to detect running under rosetta feat: add app.runningUnderRosettaTranslation to detect running under rosetta Nov 12, 2020
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

I'm not able to come up with a 2nd use case so maybe this is moot -- but runningUnderRosettaTranslation() is such a single-purpose call API that I wonder if something like String? emulatorEnvironment() that returns rosetta here would be more reusable for some currently-unknown future use.

👍 on including this, whatever its final form, in 11

shell/browser/api/electron_api_app.h Outdated Show resolved Hide resolved
shell/browser/api/electron_api_app_mac.mm Outdated Show resolved Hide resolved
shell/browser/api/electron_api_app_mac.mm Outdated Show resolved Hide resolved
@MarshallOfSound
Copy link
Member Author

I'm not able to come up with a 2nd use case so maybe this is moot -- but runningUnderRosettaTranslation() is such a single-purpose call API that I wonder if something like String? emulatorEnvironment() that returns rosetta here would be more reusable for some currently-unknown future use.

Without building this is a super expansible way I don't think we can cover all the possible emulation scenarios. i.e. technically you could be under multiple layers of emulation (Rosetta --> Qemu for instance is how docker is going to work). Given the very targeted and current use case for this I don't think making it partially generic gives us too much for the future. Emulation is done differently on different platforms and each has it's own implications, i.e. I can see people being interested in a process.arch vs process.archMyProcessIsBeingTreatedAs but that isn't important for this rosetta use case where we just need to know if rosetta is in play. Even if rosetta isn't actually doing anything 🤷

ckerr
ckerr previously approved these changes Nov 12, 2020
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

api lgtm, but let's reuse chrome's impl.

@ckerr ckerr dismissed their stale review November 13, 2020 20:07

further changes post-approval

@MarshallOfSound MarshallOfSound merged commit d601963 into master Nov 13, 2020
@release-clerk
Copy link

release-clerk bot commented Nov 13, 2020

Release Notes Persisted

Added new app.runningUnderRosettaTranslation property to detect when running under rosetta on Apple silicon

@trop
Copy link
Contributor

trop bot commented Nov 13, 2020

I have automatically backported this PR to "11-x-y", please check out #26492

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants