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: Podfile.lock paths are no longer absolute #1203

Merged
merged 6 commits into from Jun 19, 2020
Merged

fix: Podfile.lock paths are no longer absolute #1203

merged 6 commits into from Jun 19, 2020

Conversation

grabbou
Copy link
Member

@grabbou grabbou commented Jun 17, 2020

Summary:

Small regression introduced by a previous change to the file. reactNativePath is absolute (as returned by the CLI configuration) in order to be used in various places.

The path used by Podfile has to be relative to its location, hence the change. I am leaving the object as a return value to avoid changing RN files and make it easy to customize in the future.

Context: The output of running use_native_modules is used here: https://github.com/facebook/react-native/tree/master/template/ios/Podfile#L9

@grabbou
Copy link
Member Author

grabbou commented Jun 17, 2020

Once running pod install, you will notice relative paths are again used in Podfile.lock.

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

You'll likely need to update ruby tests

Copy link
Member

@alloy alloy left a comment

Choose a reason for hiding this comment

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

Yup, that looks like the right fix, thanks 👌

Could you add a test case that would actually fail without this change, though? Seems too easy to regress on this.

@grabbou
Copy link
Member Author

grabbou commented Jun 19, 2020

Thanks, @alloy, I've added a simple test suite. Let me know if that works.

Unfortunately, it turned out to be rather complex change and I had to perform some changes to the logic of the testing script itself. Not really proud of them, but I don't think there was a better way.

Let me know if you have any other suggestions or this is good to go.

@thymikee thymikee requested a review from alloy June 19, 2020 10:44
Copy link
Member

@alloy alloy left a comment

Choose a reason for hiding this comment

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

Looks great to me! 👌

@alloy alloy merged commit e949e23 into master Jun 19, 2020
@thymikee thymikee deleted the fix/cp-paths branch June 19, 2020 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants