-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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(ios): fix pod install --project-directory=...
#36096
Conversation
oh wait I just realized this PR is directly for 0.71-stable; @tido64 did you replicate this change for main branch as a separate PR, or there's no need there? |
It's not needed on |
sounds good - I've added it to the discussion, the CI issues on iOS are related to a problem with state of branch post release, so nothing to worry about. We'll be able to squash&merge when preparing next patch release 👍 |
Thanks ❤️ |
What's the offending change out of curiousity? |
|
package_path = File.join(Pod::Config.instance.installation_root, react_native_path, "package.json") | ||
package = JSON.parse(File.read(package_path)) |
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 done like this, it breaks RNTester, unfortunately. Should we find a better way to handle this? 🤔
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.
How does it break RNTester? I printed out the output and it looks correct to me?
Generating Pods project
+"/~/react-native/packages/rn-tester/../../package.json"
Setting REACT_NATIVE build settings
Setting CLANG_CXX_LANGUAGE_STANDARD to c++17 on /~/react-native/packages/rn-tester/RNTesterPods.xcodeproj
Pod install took 12 [s] to run
Integrating client project
Pod installation complete! There are 67 dependencies from the Podfile and 55 total pods installed.
The installation also succeeded…
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.
my bad the RNTester and CircleCI issue is due to hermes-engine versions... 🤦
On the joke, I think it is very spot on, actually. Can we create a job in CI that tests that use case? I think is important that we make sure we don't regress. PErhaps we can move the discussion in another place, but what would be a good example/way to test it? npx react-native init NewApp --skip-install
cd NewApp # to move into the NewApp folder
yarn install # to install yarn dependencies
pod install --project-directory ios/ # to keep it tested ? |
Yes please. We broke this too often that it's really worth to just add a small test in one of our template tests. |
This is what we do in
Where you run |
I think we should add a template job that tests a commit of RN against That would help us cover a lot of scenarios that are library specific and that we broke in the past. |
@cipolleschi @cortinico please remember that this PR is against 0.71 to address the specific problem, if we want to do what you propose it should be handled as a separate work in main branch |
Yup I was just brainstorming ideas to prevent this from happening again, if that could help @tido64 |
I think this is a good suggestion. It will catch most scenarios like you said. |
Summary
Jokes aside, I think the breaking change was only introduced on the
0.71-stable
branch. So no need to fix anything onmain
.Changelog
[IOS] [FIXED] - Fix
pod install --project-directory=...