-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
DynamicLinks: Skip retrieving web locale when running targeting below iOS14 (only on simulator & Apple Silicon) #7989
Conversation
Generated by 🚫 Danger |
// Perform an early exit if the process is running under Rosetta translation and targeting | ||
// under iOS 14. | ||
if (processIsTranslated() && !systemVersionAtLeastiOS14) { | ||
[self handleExecutionError:nil]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling the delegate method in the early exit case avoids interrupting the dynamic links flow and the locale property in the delegate will be set to empty string. See here
// and -1 when an error occurs. | ||
// From: | ||
// https://developer.apple.com/documentation/apple-silicon/about-the-rosetta-translation-environment | ||
int processIsTranslated() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this C function static so that it does not pollute the global symbol namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL... thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with static fix.
Deferring approval to @eldhosembabu
Coverage ReportAffected SDKs
Test Logs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the changelog
Manually tested fix on M1 machine and it successfully ran without crashing.
Fixes #7618