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

Use relative installation root instead of absolute to avoid embedding absolute paths in pods project #33187

Closed
wants to merge 3 commits into from

Conversation

danilobuerger
Copy link
Contributor

Summary

Use relative installation root instead of absolute to avoid embedding absolute paths in pods project
Also removes a leading space from each path.

Before:

155846827-94c474b7-8a79-45fc-a900-8860a94fb318

After:

Screenshot 2022-02-26 at 15 58 32

Changelog

[iOS] [Fixed] - Remove absolute paths from pods project

Test Plan

Pod install and view in Xcode FBReactNativeSpec -> Build Phases -> [CP-User] Generate Specs

… absolute paths in pods project

Also removes a leading space from each path
@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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 26, 2022
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Feb 26, 2022
@analysis-bot
Copy link

analysis-bot commented Feb 26, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: a0511a1
Branch: main

@analysis-bot
Copy link

analysis-bot commented Feb 26, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,204,201 -8,462
android hermes armeabi-v7a 7,811,068 +12,280
android hermes x86 8,575,744 -10,333
android hermes x86_64 8,526,542 -10,237
android jsc arm64-v8a 9,872,944 -8,486
android jsc armeabi-v7a 8,864,360 +12,259
android jsc x86 9,840,570 -10,351
android jsc x86_64 10,435,497 -10,259

Base commit: a0511a1
Branch: main

@danilobuerger
Copy link
Contributor Author

This seems to fail because of a cyclic dependency between rntester and codegen. I don't really know if this is related and/or how to fix this.

@cortinico
Copy link
Contributor

I think the best reviewer here is @dmitryrykun as he was looking into relative path support for iOS inside our cocoapods setup 👍

@facebook-github-bot
Copy link
Contributor

@dmitryrykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cortinico
Copy link
Contributor

@danilobuerger Just a heads up that @dmitryrykun is off this week so this will take a bit longer 👍

@dmytrorykun
Copy link
Contributor

@danilobuerger these changes break the build of RNTester (and potentially any project with turbo modules). I'll take look into what's going on there, but it may take a while. Any help is appreciated.

@danilobuerger
Copy link
Contributor Author

@dmitryrykun It looks like a real cyclic dependency in Xcode to me:

Target "ScreenshotManager" depends on Target "React" depends on Target "React-RCTAnimation" depends on Target "React-Codegen".

Task "React-Codegen" has a Build Phase "Compile Sources" with an Input file:

../build/generated/ios/./ScreenshotmanagerSpec/ScreenshotmanagerSpec-generated.mm

This Input file is listed as an Output file in "ScreenshotManager" Build Phase "[CP-User] Generate Specs".

This PR fixed the paths so that Xcode saw the dependency cycle.

@danilobuerger
Copy link
Contributor Author

@dmitryrykun Changing ScreenshotManagers dependency from React to React-Core seems to successfully build. However, I am unsure if this is correct?

@danilobuerger
Copy link
Contributor Author

I remember a while back there was a massive change from dependency React => React-Core in third party libraries. I found #29633 (comment) maybe the ScreenshotManager was just forgotten?

This resolves a cyclic dependency between generating the spec and codegen using it and something relying on codegen transitively via the React dependency

See facebook#29633 (comment) for an explanation
This is advised by apple:

Dependency Order

Build targets in parallel according to their dependencies. Choose this option, not Manual Order.

Manual Order

Build targets serially in the order listed in the scheme, underutilizing your Mac’s multiple processors. This option is deprecated. Don’t use it.

See https://developer.apple.com/documentation/xcode/customizing-the-build-schemes-for-a-project?changes=latest_minor
@danilobuerger
Copy link
Contributor Author

@dmitryrykun all checks are green now. Hopefully this is the right approach.

It would be nice to get this into 0.68 so that we don't have hardcoded absolute paths in a lot of projects that could potentially break and lead to hard to debug issues.

Copy link
Contributor

@dmytrorykun dmytrorykun left a comment

Choose a reason for hiding this comment

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

Great job, thank you for getting it done!

@facebook-github-bot
Copy link
Contributor

@dmitryrykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @danilobuerger in 42b01a3.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Mar 7, 2022
@danilobuerger danilobuerger deleted the patch-4 branch March 7, 2022 15:39
ShikaSD pushed a commit that referenced this pull request Mar 16, 2022
… absolute paths in pods project (#33187)

Summary:
Use relative installation root instead of absolute to avoid embedding absolute paths in pods project
Also removes a leading space from each path.

Before:

<img width="799" alt="155846827-94c474b7-8a79-45fc-a900-8860a94fb318" src="https://user-images.githubusercontent.com/996231/155847731-de128759-bff5-4d1f-a59a-377298055d85.png">

After:

<img width="745" alt="Screenshot 2022-02-26 at 15 58 32" src="https://user-images.githubusercontent.com/996231/155847739-b783debc-a805-4ce7-a88a-33f764dc5985.png">

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - Remove absolute paths from pods project

Pull Request resolved: #33187

Test Plan: Pod install and view in Xcode FBReactNativeSpec -> Build Phases -> [CP-User] Generate Specs

Reviewed By: ShikaSD

Differential Revision: D34549541

Pulled By: dmitryrykun

fbshipit-source-id: 2926b093fb87f50ef9988e23fce593348f00077d
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
… absolute paths in pods project (facebook#33187)

Summary:
Use relative installation root instead of absolute to avoid embedding absolute paths in pods project
Also removes a leading space from each path.

Before:

<img width="799" alt="155846827-94c474b7-8a79-45fc-a900-8860a94fb318" src="https://user-images.githubusercontent.com/996231/155847731-de128759-bff5-4d1f-a59a-377298055d85.png">

After:

<img width="745" alt="Screenshot 2022-02-26 at 15 58 32" src="https://user-images.githubusercontent.com/996231/155847739-b783debc-a805-4ce7-a88a-33f764dc5985.png">

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - Remove absolute paths from pods project

Pull Request resolved: facebook#33187

Test Plan: Pod install and view in Xcode FBReactNativeSpec -> Build Phases -> [CP-User] Generate Specs

Reviewed By: ShikaSD

Differential Revision: D34549541

Pulled By: dmitryrykun

fbshipit-source-id: 2926b093fb87f50ef9988e23fce593348f00077d
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. Merged This PR has been merged. 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
Development

Successfully merging this pull request may close these issues.

6 participants