-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[RN][iOS] Add matrix for Debug/Release, New/Legacy Architecture, (No)Hermes #34469
Conversation
e08f595
to
41fd8e3
Compare
Base commit: 3d82f7e |
Base commit: 3d82f7e |
7c7d814
to
be383f4
Compare
7c39181
to
711e06e
Compare
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
ec62323
to
d755c60
Compare
959b662
to
c99c689
Compare
default: "OldArch" | ||
hermes: | ||
type: string | ||
default: "Hermes" |
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.
would it be possible to add a flipper or not (since I think flipper in release mode had a problem recently, and release mode issues are typically not discovered until later in the RC cycle after the first few build checks)
And of course my favorite we add a use_frameworks check (with possibly both linkages - so it's actually a tri-state, no frameworks, frameworks+dynamic, frameworks+static)
These are the variations I'm currently aware of
I expect failure in hermes+frameworks+dynamic, and flipper+frameworks+any and newarch+frameworks+any, but on the good side, the other permutations are all working I think
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.
As discussed offline (but leaving the reply here for future doc), yes.
I plan to add the Flipper Yes/No, excluding all the WithFlipper+Release
because flipper is not added by default in Release.
For use framework, yes... it is possible, but requires some more changes also in the podfiles...
I'll probably add them in a separate PR, though. Just not to add too many things at once.
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.
Totally agree with incremental improvement in all things, and specifically here. But as a brainstorm for a future PR: I also just remembered macCatalyst builds. That was an odyssey getting them working again after they broke in 0.63, would be nice to have a canary here that would tell us if they broke again.
c99c689
to
f639f5a
Compare
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
f639f5a
to
ae3f627
Compare
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
ae3f627
to
c263c35
Compare
c263c35
to
fc60ecf
Compare
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @cipolleschi in 4352459. When will my fix make it into a release? | Upcoming Releases |
Summary
This PR is the dual of the Matrix Tests we added to the Android Template a couple of weeks ago. It adds the same tests to iOS, to verify that the template builds with both architectures and with both configurations (Debug/Release).. And it tests that the template works with both Hermes and without it.
Changelog
[iOS] [Added] - Test iOS template with both architectures and configurations
Test Plan
CI is green in all the 8 new jobs.