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

[RN][iOS] Add matrix for Debug/Release, New/Legacy Architecture, (No)Hermes #34469

Closed
wants to merge 1 commit into from

Conversation

cipolleschi
Copy link
Contributor

@cipolleschi cipolleschi commented Aug 22, 2022

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.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Aug 22, 2022
@cipolleschi cipolleschi force-pushed the feat/add-matrix-tests-for-ios-template branch 3 times, most recently from e08f595 to 41fd8e3 Compare August 22, 2022 13:44
@react-native-bot react-native-bot added Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Aug 22, 2022
@analysis-bot
Copy link

analysis-bot commented Aug 22, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 3d82f7e
Branch: main

@analysis-bot
Copy link

analysis-bot commented Aug 22, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,617,874 -16,553
android hermes armeabi-v7a 7,032,126 -14,434
android hermes x86 7,917,927 -18,430
android hermes x86_64 7,891,556 -16,857
android jsc arm64-v8a 9,495,283 -16,634
android jsc armeabi-v7a 8,272,914 -14,284
android jsc x86 9,433,074 -18,374
android jsc x86_64 10,026,064 -16,632

Base commit: 3d82f7e
Branch: main

@cipolleschi cipolleschi force-pushed the feat/add-matrix-tests-for-ios-template branch 11 times, most recently from 7c7d814 to be383f4 Compare August 28, 2022 09:17
@cipolleschi cipolleschi changed the title feat: add matrix for Debug/Release and New/Legacy architecture [RN][iOS] Add matrix for Debug/Release and New/Legacy Architecture Aug 28, 2022
@cipolleschi cipolleschi marked this pull request as ready for review August 28, 2022 10:09
@cipolleschi cipolleschi force-pushed the feat/add-matrix-tests-for-ios-template branch 2 times, most recently from 7c39181 to 711e06e Compare August 28, 2022 10:22
@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cipolleschi cipolleschi force-pushed the feat/add-matrix-tests-for-ios-template branch 3 times, most recently from ec62323 to d755c60 Compare August 28, 2022 11:46
@cipolleschi cipolleschi force-pushed the feat/add-matrix-tests-for-ios-template branch 8 times, most recently from 959b662 to c99c689 Compare August 28, 2022 16:57
@cipolleschi cipolleschi changed the title [RN][iOS] Add matrix for Debug/Release and New/Legacy Architecture [RN][iOS] Add matrix for Debug/Release, New/Legacy Architecture, (No)Hermes Aug 28, 2022
default: "OldArch"
hermes:
type: string
default: "Hermes"
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@cipolleschi cipolleschi force-pushed the feat/add-matrix-tests-for-ios-template branch from c99c689 to f639f5a Compare August 28, 2022 18:32
@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cipolleschi cipolleschi force-pushed the feat/add-matrix-tests-for-ios-template branch from f639f5a to ae3f627 Compare August 31, 2022 13:17
@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cipolleschi cipolleschi force-pushed the feat/add-matrix-tests-for-ios-template branch from ae3f627 to c263c35 Compare August 31, 2022 13:21
@cipolleschi cipolleschi force-pushed the feat/add-matrix-tests-for-ios-template branch from c263c35 to fc60ecf Compare August 31, 2022 13:24
@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @cipolleschi in 4352459.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Aug 31, 2022
@yungsters yungsters deleted the feat/add-matrix-tests-for-ios-template branch September 29, 2022 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Facebook Partner: Facebook Partner Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants