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

ci: use pod install --project-directory when testing template #37996

Closed
wants to merge 1 commit into from

Conversation

tido64
Copy link
Collaborator

@tido64 tido64 commented Jun 21, 2023

Summary:

Exercise pod install --project-directory=ios when building the generated iOS project to make sure we don't regress in the future.

See also #37992, #35754, #34215, #33909

Changelog:

[INTERNAL] [ADDED] - Exercise pod install --project-directory=ios when testing the iOS template

Test Plan:

CI should pass.

@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: Microsoft Partner: Microsoft Partner labels Jun 21, 2023
@tido64 tido64 force-pushed the tido/test-pod-install-project-directory branch from 09ed06e to 62ad520 Compare June 21, 2023 11:26
@analysis-bot
Copy link

analysis-bot commented Jun 21, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,041,897 -4
android hermes armeabi-v7a 8,292,541 -4
android hermes x86 9,559,000 -1
android hermes x86_64 9,400,630 -2
android jsc arm64-v8a 9,601,081 -2
android jsc armeabi-v7a 8,729,056 -1
android jsc x86 9,689,077 -6
android jsc x86_64 9,934,788 -2

Base commit: eaafc26
Branch: main

@tido64
Copy link
Collaborator Author

tido64 commented Jun 21, 2023

Seems to be failing as expected:
image

I'll rebase when #37992 lands.

@tido64 tido64 force-pushed the tido/test-pod-install-project-directory branch from 62ad520 to 09044fc Compare June 23, 2023 10:41
@tido64 tido64 marked this pull request as ready for review June 23, 2023 11:29
@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.

Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

doing this will run the pod install command for ALL the template tests from the parent directory.

Could there be a scenario where this works but running pod install from the ios folder (i.e.: without the --project-directory parameter) fails?

In other words, if we proceed with this, could we end up breaking pod install from ios?

My gut says no, as using --project-directory seems like a more general case than running pod install from a specific folder, but I think it's worth double-checking.

@cipolleschi
Copy link
Contributor

(requesting change to discuss the point above, mainly)

@cipolleschi
Copy link
Contributor

/rebase

@github-actions github-actions bot force-pushed the tido/test-pod-install-project-directory branch from 09044fc to 6b938c7 Compare June 26, 2023 09:05
@tido64
Copy link
Collaborator Author

tido64 commented Jun 26, 2023

In other words, if we proceed with this, could we end up breaking pod install from ios?

I haven't run through all possible cases, but for what it's worth, we've always used this in RNTA and have not encountered a case where pod install broke and we didn't catch it. If you want to, we could probably add another job where we use plain pod install?

@cipolleschi
Copy link
Contributor

Yeah, I supposed that that was the case.
I'd say let's keep it like this, for now. I don't think we will ever encounter breakages, so let's keep it simple. We can always add another job in a second step! ;)

@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
Copy link
Contributor

/rebase

@github-actions github-actions bot force-pushed the tido/test-pod-install-project-directory branch from 6b938c7 to 2e108c8 Compare June 26, 2023 14:40
@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.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jun 26, 2023
@github-actions
Copy link

This pull request was successfully merged by @tido64 in e1fd4a8.

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

@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in e1fd4a8.

@kelset kelset deleted the tido/test-pod-install-project-directory branch June 27, 2023 09:23
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: Microsoft Partner: Microsoft Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants