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 pod post_install logic for detecting if fabric is enabled #41284

Conversation

gabrieldonadel
Copy link
Collaborator

Summary:

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

fabric_enabled = ReactNativePodsUtils.has_pod(installer, 'React-Fabric')

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-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: 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
Copy link
Contributor

@cipolleschi cipolleschi left a comment

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
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
Contributor

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

@gabrieldonadel
Copy link
Collaborator Author

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

Copy link

github-actions bot commented Nov 1, 2023

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

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

@github-actions github-actions bot 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)`  (facebook/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: facebook/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
Othinn pushed a commit to Othinn/react-native that referenced this pull request Jan 9, 2024
…cebook#41284)

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)`  (facebook#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

Pull Request resolved: facebook#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
Othinn pushed a commit to Othinn/react-native that referenced this pull request Jan 9, 2024
…acebook#41286)

Summary:
Follow up of facebook#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: facebook#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 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. Merged This PR has been merged. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants