Merged
Conversation
michalmaka
force-pushed
the
replace-debug-with-ndebug
branch
from
5e610f7 to
6305042
Compare
tomekzaw
approved these changes
Oct 9, 2023
Member
|
Added CI check that disallows |
piaskowyk
approved these changes
Oct 16, 2023
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 20, 2023
## Summary This PR aims to fix the issue when Release builds would fail on Paper on iOS. The reason this regression happened is #5113. XCode ships by default to new projects `DEBUG` macro for Debug Configuration. Due to some issues (#4902) we decided to switch over to `NDEBUG` macro. However, the `NDEBUG` is not provided by XCode out of the box. Incidentally, it's provided by React Native which puts its install script into project's Podfile - however, [only in Fabric](facebook/react-native@421df9f). What is more, some time ago React Native made it so `PRODUCTION=1 pod install` is [no longer a necessary step](facebook/react-native@93fdcba) to make a release build, therefore we cannot rely on it. Until React Native defines `NDEBUG` for Paper too we have to detect, based on available options, whether we potentially have a Release build and this pull request does this. Thanks to #5383 we finally figures out how to do it the most agreeable way. We hardcode that for a build config named `Release` NDEBUG=1 will be set. If the user uses some custom release build config e.g. "MyRelease", NDEBUG=1 will not be set - in this situation we'll require the user to use `PRODUCTION=1`. ## Test plan Run all example apps (except TVOS cause I couldn't make it cooperate with me, and Web) on both iOS and Android in both Release and Debug and see if they work properly (e.g. add a throw in #ifndef fragment).
Latropos
pushed a commit
that referenced
this pull request
Nov 24, 2023
## Summary This PR aims to fix the issue when Release builds would fail on Paper on iOS. The reason this regression happened is #5113. XCode ships by default to new projects `DEBUG` macro for Debug Configuration. Due to some issues (#4902) we decided to switch over to `NDEBUG` macro. However, the `NDEBUG` is not provided by XCode out of the box. Incidentally, it's provided by React Native which puts its install script into project's Podfile - however, [only in Fabric](facebook/react-native@421df9f). What is more, some time ago React Native made it so `PRODUCTION=1 pod install` is [no longer a necessary step](facebook/react-native@93fdcba) to make a release build, therefore we cannot rely on it. Until React Native defines `NDEBUG` for Paper too we have to detect, based on available options, whether we potentially have a Release build and this pull request does this. Thanks to #5383 we finally figures out how to do it the most agreeable way. We hardcode that for a build config named `Release` NDEBUG=1 will be set. If the user uses some custom release build config e.g. "MyRelease", NDEBUG=1 will not be set - in this situation we'll require the user to use `PRODUCTION=1`. ## Test plan Run all example apps (except TVOS cause I couldn't make it cooperate with me, and Web) on both iOS and Android in both Release and Debug and see if they work properly (e.g. add a throw in #ifndef fragment).
Latropos
pushed a commit
that referenced
this pull request
Dec 12, 2023
## Summary This PR aims to fix the issue when Release builds would fail on Paper on iOS. The reason this regression happened is #5113. XCode ships by default to new projects `DEBUG` macro for Debug Configuration. Due to some issues (#4902) we decided to switch over to `NDEBUG` macro. However, the `NDEBUG` is not provided by XCode out of the box. Incidentally, it's provided by React Native which puts its install script into project's Podfile - however, [only in Fabric](facebook/react-native@421df9f). What is more, some time ago React Native made it so `PRODUCTION=1 pod install` is [no longer a necessary step](facebook/react-native@93fdcba) to make a release build, therefore we cannot rely on it. Until React Native defines `NDEBUG` for Paper too we have to detect, based on available options, whether we potentially have a Release build and this pull request does this. Thanks to #5383 we finally figures out how to do it the most agreeable way. We hardcode that for a build config named `Release` NDEBUG=1 will be set. If the user uses some custom release build config e.g. "MyRelease", NDEBUG=1 will not be set - in this situation we'll require the user to use `PRODUCTION=1`. ## Test plan Run all example apps (except TVOS cause I couldn't make it cooperate with me, and Web) on both iOS and Android in both Release and Debug and see if they work properly (e.g. add a throw in #ifndef fragment).
Latropos
pushed a commit
that referenced
this pull request
Dec 18, 2023
This PR aims to fix the issue when Release builds would fail on Paper on iOS. The reason this regression happened is #5113. XCode ships by default to new projects `DEBUG` macro for Debug Configuration. Due to some issues (#4902) we decided to switch over to `NDEBUG` macro. However, the `NDEBUG` is not provided by XCode out of the box. Incidentally, it's provided by React Native which puts its install script into project's Podfile - however, [only in Fabric](facebook/react-native@421df9f). What is more, some time ago React Native made it so `PRODUCTION=1 pod install` is [no longer a necessary step](facebook/react-native@93fdcba) to make a release build, therefore we cannot rely on it. Until React Native defines `NDEBUG` for Paper too we have to detect, based on available options, whether we potentially have a Release build and this pull request does this. Thanks to #5383 we finally figures out how to do it the most agreeable way. We hardcode that for a build config named `Release` NDEBUG=1 will be set. If the user uses some custom release build config e.g. "MyRelease", NDEBUG=1 will not be set - in this situation we'll require the user to use `PRODUCTION=1`. Run all example apps (except TVOS cause I couldn't make it cooperate with me, and Web) on both iOS and Android in both Release and Debug and see if they work properly (e.g. add a throw in #ifndef fragment).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
During working on integration of some other library @t0ms0n00 and I found out that our definition od
DEBUGpreprocessor macro was causing compilation error when we compile 3rd pary library. Such definition should not be used and better way is to use standard way ofNDEBUGdefinition which is defined in release builds by default.The problem was due to 3rd party having an enum with one entry named
DEBUGwhich was replaced by preprocessor macro to1and caused compilation error.We should use
NDEBUGor name our preprocessor macro differently likeREANIMATED_DEBUG.NDEBUGis mentioned several times in C++ standard https://isocpp.org/files/papers/N4860.pdfTest plan
Check if compilation is successful