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(ios): fix pod install --project-directory=... #36096

Merged
merged 1 commit into from Feb 13, 2023

Conversation

tido64
Copy link
Collaborator

@tido64 tido64 commented Feb 8, 2023

Summary

image

Jokes aside, I think the breaking change was only introduced on the 0.71-stable branch. So no need to fix anything on main.

Changelog

[IOS] [FIXED] - Fix pod install --project-directory=...

@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 Feb 8, 2023
@kelset
Copy link
Collaborator

kelset commented Feb 9, 2023

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?

@tido64
Copy link
Collaborator Author

tido64 commented Feb 9, 2023

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 main afaict. The offending change was only introduced on the 0.71-stable branch.

@kelset
Copy link
Collaborator

kelset commented Feb 9, 2023

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 👍

@tido64
Copy link
Collaborator Author

tido64 commented Feb 9, 2023

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 ❤️

@cortinico
Copy link
Contributor

The offending change was only introduced on the 0.71-stable branch.

What's the offending change out of curiousity?

@tido64
Copy link
Collaborator Author

tido64 commented Feb 9, 2023

The offending change was only introduced on the 0.71-stable branch.

What's the offending change out of curiousity?

8f2c7ff

Comment on lines +212 to +213
package_path = File.join(Pod::Config.instance.installation_root, react_native_path, "package.json")
package = JSON.parse(File.read(package_path))
Copy link
Contributor

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? 🤔

Copy link
Collaborator Author

@tido64 tido64 Feb 10, 2023

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…

Copy link
Contributor

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... 🤦

@cipolleschi
Copy link
Contributor

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?
Would be enough to do something like:

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

?

@cortinico
Copy link
Contributor

Can we create a job in CI that tests that use case?

Yes please. We broke this too often that it's really worth to just add a small test in one of our template tests.

@tido64
Copy link
Collaborator Author

tido64 commented Feb 10, 2023

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? Would be enough to do something like:

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

?

This is what we do in react-native-test-app and it catches most cases but it's unfortunately not enough. There are community repos (AsyncStorage and Webview come to mind) that have a structure like:

root
├── example
│   ├── android
│   │   └── build.gradle
│   ├── ios
│   │   └── Podfile
│   └── App.js
├── package.json
└── src

Where you run pod install --project-directory=example/ios. This is how I found the breakage. I don't have a good suggestion for an alternative solution other than replicating the folder structure somehow.

@cortinico
Copy link
Contributor

Where you run pod install --project-directory=example/ios. This is how I found the breakage. I don't have a good suggestion for an alternative solution other than replicating the folder structure somehow.

I think we should add a template job that tests a commit of RN against create-react-native-library (which has the same structure you suggested) and use --project-directory=

That would help us cover a lot of scenarios that are library specific and that we broke in the past.

@kelset
Copy link
Collaborator

kelset commented Feb 13, 2023

I think we should add a template job that tests a commit of RN against create-react-native-library (which has the same structure you suggested) and use --project-directory=

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

@cipolleschi cipolleschi merged commit ad1ddc2 into 0.71-stable Feb 13, 2023
@cipolleschi cipolleschi deleted the tido/fix-project-dir branch February 13, 2023 11:04
@cortinico
Copy link
Contributor

@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

@tido64
Copy link
Collaborator Author

tido64 commented Feb 13, 2023

I think we should add a template job that tests a commit of RN against create-react-native-library (which has the same structure you suggested) and use --project-directory=

That would help us cover a lot of scenarios that are library specific and that we broke in the past.

I think this is a good suggestion. It will catch most scenarios like you said.

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. p: Microsoft Partner: Microsoft Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants