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

Add NDEBUG flag for Release builds for both architectures #41715

Closed
wants to merge 6 commits into from
Closed

Add NDEBUG flag for Release builds for both architectures #41715

wants to merge 6 commits into from

Conversation

tjzel
Copy link
Contributor

@tjzel tjzel commented Nov 30, 2023

Summary:

Currently React Native defines NDEBUG flag for all pods in Fabric only. This is useful for other libraries, like Reanimated, because they have no easy way of defining their compilation flags (at least none that I know of). Therefore defining NDEBUG for both architectures would be beneficial.

Changelog:

Pick one each for the category and type tags:

[IOS] [CHANGED] - Add NDEBUG flag for Release builds for both architectures

Test Plan:

Run ruby test suite.

Notes

For the time being I just copied
prepare_pod_target_installation_results_mock
and
def prepare_installer_for_cpp_flags
to utils-test.rb since I wasn't sure how to handle the installer mock.

@facebook-github-bot
Copy link
Contributor

Hi @tjzel!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 30, 2023
@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.

@cipolleschi
Copy link
Contributor

To fix the tests, open the new_architecture-test.rb file and modify:

#line 124
-  assert_equal(react_core_release_config.build_settings["OTHER_CFLAGS"], "$(inherited)")
+  assert_nil(react_core_release_config.build_settings["OTHER_CFLAGS"])

#line 128
- assert_equal(yoga_release_config.build_settings["OTHER_CFLAGS"], "$(inherited)")
+ assert_nil(yoga_release_config.build_settings["OTHER_CFLAGS"])

@cipolleschi
Copy link
Contributor

/rebase - this is an automatic command which will rebase automatically the PR on top of main

@tjzel tjzel marked this pull request as ready for review December 1, 2023 12:05
@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.

1 similar comment
@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.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Dec 1, 2023
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in 1a0e174.

Othinn pushed a commit to Othinn/react-native that referenced this pull request Jan 9, 2024
…#41715)

Summary:
Currently React Native defines `NDEBUG` flag for all pods in Fabric only. This is useful for other libraries, like Reanimated, because they have no easy way of defining their compilation flags (at least none that I know of). Therefore defining `NDEBUG` for both architectures would be beneficial.

## Changelog:

Pick one each for the category and type tags:

[IOS] [CHANGED] - Add `NDEBUG` flag for Release builds for both architectures

Pull Request resolved: facebook#41715

Test Plan:
Run ruby test suite.

## Notes

For the time being I just copied
`prepare_pod_target_installation_results_mock`
and
`def prepare_installer_for_cpp_flags`
to `utils-test.rb` since I wasn't sure how to handle the installer mock.

Reviewed By: cortinico

Differential Revision: D51708382

Pulled By: cipolleschi

fbshipit-source-id: ff206f8fc151934dbae89aacd1bc69c57b4f28ee
jasminmif pushed a commit to TeamGuilded/react-native that referenced this pull request Jan 24, 2024
…#41715)

Summary:
Currently React Native defines `NDEBUG` flag for all pods in Fabric only. This is useful for other libraries, like Reanimated, because they have no easy way of defining their compilation flags (at least none that I know of). Therefore defining `NDEBUG` for both architectures would be beneficial.

Pick one each for the category and type tags:

[IOS] [CHANGED] - Add `NDEBUG` flag for Release builds for both architectures

Pull Request resolved: facebook#41715

Test Plan:
Run ruby test suite.

For the time being I just copied
`prepare_pod_target_installation_results_mock`
and
`def prepare_installer_for_cpp_flags`
to `utils-test.rb` since I wasn't sure how to handle the installer mock.

Reviewed By: cortinico

Differential Revision: D51708382

Pulled By: cipolleschi

fbshipit-source-id: ff206f8fc151934dbae89aacd1bc69c57b4f28ee
jasminmif pushed a commit to TeamGuilded/react-native that referenced this pull request Jan 24, 2024
…#41715)

Summary:
Currently React Native defines `NDEBUG` flag for all pods in Fabric only. This is useful for other libraries, like Reanimated, because they have no easy way of defining their compilation flags (at least none that I know of). Therefore defining `NDEBUG` for both architectures would be beneficial.

Pick one each for the category and type tags:

[IOS] [CHANGED] - Add `NDEBUG` flag for Release builds for both architectures

Pull Request resolved: facebook#41715

Test Plan:
Run ruby test suite.

For the time being I just copied
`prepare_pod_target_installation_results_mock`
and
`def prepare_installer_for_cpp_flags`
to `utils-test.rb` since I wasn't sure how to handle the installer mock.

Reviewed By: cortinico

Differential Revision: D51708382

Pulled By: cipolleschi

fbshipit-source-id: ff206f8fc151934dbae89aacd1bc69c57b4f28ee
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: Software Mansion Partner: Software Mansion Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants