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

[fix] change Yoga module name capitalisation in podspec to fix build issue #34058

Closed
wants to merge 1 commit into from

Conversation

kelset
Copy link
Collaborator

@kelset kelset commented Jun 23, 2022

Summary

This is just a quick PR that replicates the fix that @ackerleytng proposed here #33648; I'm not sure how to test the fix so I just went ahead and made the PR so that we can at least test it on CI and against internal + quickly merge and cherry pick it for 0.69.1.

Fixes #33648
Fixes #33976

Changelog

[iOS] [Fixed] - change Yoga module name capitalisation in podspec to fix build issue

Test plan

Verify against CI + internal CI, and maybe use the commit-ly to allow folks to verify the fix for themselves before we cherry pick?

@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. Contributor A React Native contributor. p: Microsoft Partner: Microsoft Partner labels Jun 23, 2022
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jun 23, 2022
@kelset kelset added the Platform: iOS iOS applications. label Jun 23, 2022
@mikehardy
Copy link
Contributor

🤦 mentally taking notes: "as a release tester, I want to verify things work on case-sensitive filesystems" so...the matrix of all test things should be expanded to run once on case-sensitive fs and once on case-insensitive. Ouch!

@@ -27,7 +27,7 @@ Pod::Spec.new do |spec|
spec.authors = 'Facebook'
spec.source = source

spec.module_name = 'yoga'
spec.module_name = 'Yoga'
spec.header_dir = 'yoga'
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no special knowledge here but it seems suspicious to have header_dir lowercase while module name is upper? Makes the spidey-sense tingle but it may actually be lower for the files 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah tbh I was wondering if we should change both

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but wanted to get CI to run first, and get Riccardo's take ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

The CI is red btw, and it seems related to this change

@renchap
Copy link

renchap commented Jun 24, 2022

I am using a case-insensitive FS and I also get the second issue this is supposed to fix (#33976), even after applying the patch (and reinstalling the pods).

Is it possible that this is not the proper fix, at least for the second linked issue? We did more investigation in the comments and have another possible cause for #33976.

@kelset
Copy link
Collaborator Author

kelset commented Jun 24, 2022

sounds good @renchap, thanks for sharing! Let's focus on this PR then #34064 and see if it can get merged & cherry picked, after which if both issues are solved we can close this one off 👍

@cipolleschi
Copy link
Contributor

Changing the name of the module breaks all the Swift imports (Swift file are import yoga but now the module is called Yoga so they should import Yoga). I guess that a proper solution to the issue is to make the pod case-insensitive in a sense that we use Yoga with the capital Y everywhere properly.
This require an effort from Meta because there are some internal changes to be applied as well.
I created a task internally to track this.

Also, this is not a fix for issue #33976. We are unable to reproduce the issue, so far. If you can create a repro, it will be very helpful!

@kelset
Copy link
Collaborator Author

kelset commented Jun 27, 2022

yeah let's close this off then, I just spun it up just in case it was a quick-fix that we could merge right away 🤣 thanks for the detailed explanation @cipolleschi !

@kelset kelset closed this Jun 27, 2022
@kelset kelset deleted the kelset/fix-yoga-capitalization branch June 27, 2022 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Microsoft Partner: Microsoft Partner Platform: iOS iOS applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
7 participants