-
Notifications
You must be signed in to change notification settings - Fork 43
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
Default to only modern architectures #448
Conversation
* Adding an unrecognized new architecture here will fail. | ||
* By default, only common modern iOS architectures will be built: | ||
* ios_arm64, ios_armv7, ios_x86_64. You may choose to add any of the remaining | ||
* entries from NativeCompilation.ALL_SUPPORTED_ARCHS to support all possible |
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.
It would be better to enumerate the other alternatives as well:
...
entries from NativeCompilation.ALL_SUPPORTED_ARCHS (ios_i386 and ios_armv7s)
to support all possible iOS architectures. Listing any new architectures outside of
ALL_SUPPORTED_ARCHS will fail the build.
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.
done
@advayDev1 - I agree this is much better. Thanks for the suggestion. |
LGTM |
You'll just need to fix up the unit tests... you can copy some of the changes from my PR. |
LGTM |
@advayDev1 You should add the overview comment that @brunobowden had in his request. It shows which phone type has which architecture. |
@confile - done |
Default to only modern architectures
Better fix for #443 (compare vs. #444).
I think this is much simpler, makes regular users faster, and does not take away power users' ability to use all architectures.
cc: @confile