Skip to content

Fix ios pod post_install logic for detecting if fabric is enabled - #41284

Closed
gabrieldonadel wants to merge 1 commit into
react:mainfrom
gabrieldonadel:@gabrieldonadel/fix-post-install-fabric-detection
Closed

Fix ios pod post_install logic for detecting if fabric is enabled#41284
gabrieldonadel wants to merge 1 commit into
react:mainfrom
gabrieldonadel:@gabrieldonadel/fix-post-install-fabric-detection

Conversation

@gabrieldonadel

Copy link
Copy Markdown
Collaborator

Summary:

There is a problem in the way that we check if Fabric is enabled inside react_native_post_install.

https://github.com/facebook/react-native/blob/899e7cdb55197fc17a96a93af4f8bcc7519553c2/packages/react-native/scripts/react_native_pods.rb#L239

We're determining if fabric is enabled by checking if the React-Fabric pod is present, but since we always call setup_fabric!(:react_native_path => prefix) (#39057) inside use_react_native the React-Fabric pod is always present causing the -DRN_FABRIC_ENABLED flag to always be added to project.pbxproj even if the new arch is disabled.

Changelog:

[IOS] [FIXED] - Fix ios pod post_install logic for detecting if fabric is enabled

Test Plan:

Run use_react_native!(fabric => false) should not add the -DRN_FABRIC_ENABLED flag to project.pbxproj

@facebook-github-botfacebook-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: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Nov 1, 2023

@cipolleschicipolleschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perfect! Thank you @gabrieldonadel!

@cipolleschi

Copy link
Copy Markdown
Contributor

For another change, we should probably do the same for Hermes, as line 100 sets ENV['USE_HERMES'], while line 240 decides whether we are using hermes based on the installed pods.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@gabrieldonadel

Copy link
Copy Markdown
CollaboratorAuthor

For another change, we should probably do the same for Hermes, as line 100 sets ENV['USE_HERMES'], while line 240 decides whether we are using hermes based on the installed pods.

Here is the followup pr #41286

@github-actions

Copy link
Copy Markdown

This pull request was successfully merged by @gabrieldonadel in 0f8a83e.

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

@github-actionsgithub-actionsBot added the Merged This PR has been merged. label Nov 1, 2023
@gabrieldonadel
gabrieldonadel deleted the @gabrieldonadel/fix-post-install-fabric-detection branch November 1, 2023 20:51
douglowder pushed a commit to react-native-tvos/react-native-tvos that referenced this pull request Nov 2, 2023
…1284)
Summary:
There is a problem in the way that we check if Fabric is enabled inside `react_native_post_install`.
https://github.com/facebook/react-native/blob/899e7cdb55197fc17a96a93af4f8bcc7519553c2/packages/react-native/scripts/react_native_pods.rb#L239
We're determining if fabric is enabled by checking if the `React-Fabric pod `is present, but since we always call `setup_fabric!(:react_native_path => prefix)` (react/react-native#39057) inside `use_react_native` the `React-Fabric` pod is always present causing the `-DRN_FABRIC_ENABLED` flag to always be added to `project.pbxproj` even if the new arch is disabled.
[IOS] [FIXED] - Fix ios pod post_install logic for detecting if fabric is enabled
Pull Request resolved: react/react-native#41284
Test Plan: Run `use_react_native!(fabric => false)` should not add the `-DRN_FABRIC_ENABLED` flag to `project.pbxproj`
Reviewed By: fkgozali
Differential Revision: D50896487
Pulled By: cipolleschi
fbshipit-source-id: 78154407ce52b09fd3a317b7dc64bd4bba56363e
facebook-github-bot pushed a commit that referenced this pull request Nov 2, 2023
…41286)
Summary:
Follow up of #41284 (comment)
We should not rely on checking if the `React-hermes` pod is present to determine if hermes is enabled
## Changelog:
[IOS] [CHANGED] - Update ios pod post_install logic for detecting if hermes is enabled
Pull Request resolved: #41286
Test Plan: Run `use_react_native!(hermes => false)` should not add `USE_HERMES = true;` to `project.pbxproj`
Reviewed By: blakef
Differential Revision: D50899654
Pulled By: cipolleschi
fbshipit-source-id: a5ab5b0117c61014e77b780c50bf349da92c6342
lunaleaps pushed a commit that referenced this pull request Nov 3, 2023
…1284)
Summary:
There is a problem in the way that we check if Fabric is enabled inside `react_native_post_install`.
https://github.com/facebook/react-native/blob/899e7cdb55197fc17a96a93af4f8bcc7519553c2/packages/react-native/scripts/react_native_pods.rb#L239
We're determining if fabric is enabled by checking if the `React-Fabric pod `is present, but since we always call `setup_fabric!(:react_native_path => prefix)` (#39057) inside `use_react_native` the `React-Fabric` pod is always present causing the `-DRN_FABRIC_ENABLED` flag to always be added to `project.pbxproj` even if the new arch is disabled.
[IOS] [FIXED] - Fix ios pod post_install logic for detecting if fabric is enabled
Pull Request resolved: #41284
Test Plan: Run `use_react_native!(fabric => false)` should not add the `-DRN_FABRIC_ENABLED` flag to `project.pbxproj`
Reviewed By: fkgozali
Differential Revision: D50896487
Pulled By: cipolleschi
fbshipit-source-id: 78154407ce52b09fd3a317b7dc64bd4bba56363e
huntie pushed a commit that referenced this pull request Jan 2, 2024
…41286)
Summary:
Follow up of #41284 (comment)
We should not rely on checking if the `React-hermes` pod is present to determine if hermes is enabled
## Changelog:
[IOS] [CHANGED] - Update ios pod post_install logic for detecting if hermes is enabled
Pull Request resolved: #41286
Test Plan: Run `use_react_native!(hermes => false)` should not add `USE_HERMES = true;` to `project.pbxproj`
Reviewed By: blakef
Differential Revision: D50899654
Pulled By: cipolleschi
fbshipit-source-id: a5ab5b0117c61014e77b780c50bf349da92c6342
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.p: ExpoPartner: ExpoPartnerShared with MetaApplied 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.

3 participants

@gabrieldonadel@cipolleschi@facebook-github-bot