[ios] Only add HERMES_ENABLE_DEBUGGER flag to debug builds by gabrieldonadel · Pull Request #5383 · software-mansion/react-native-reanimated

gabrieldonadel · 2023-11-16T20:10:34Z

Summary

Building on release mode using react-native 0.73.0-rc.4 results in a build error due to undefined symbols

Undefined symbols for architecture x86_64:
"facebook::hermes::inspector_modern::chrome::enableDebugging(std::__1::unique_ptr<facebook::hermes::inspector_modern::RuntimeAdapter, std::__1::default_deletefacebook::hermes::inspector_modern::RuntimeAdapter>, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator> const&)", referenced from:
reanimated::ReanimatedHermesRuntime::ReanimatedHermesRuntime(std::__1::unique_ptr<facebook::hermes::HermesRuntime, std::__1::default_deletefacebook::hermes::HermesRuntime>, std::__1::shared_ptrfacebook::react::MessageQueueThread const&, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator> const&) in libRNReanimated.a(ReanimatedHermesRuntime.o)
"facebook::hermes::inspector_modern::chrome::disableDebugging(int)", referenced from:
reanimated::ReanimatedHermesRuntime::~ReanimatedHermesRuntime() in libRNReanimated.a(ReanimatedHermesRuntime.o)

This happens because react native only sets HERMES_ENABLE_DEBUGGER in debug mode https://github.com/facebook/react-native/blob/23cf10428eb58cfcbf614b8b0728f2535f2d252f/packages/react-native/scripts/cocoapods/utils.rb#L44-L47

So in order to fix this I believe that instead of always adding the HERMES_ENABLE_DEBUGGER flag we should only it on debug mode as well, using GCC_PREPROCESSOR_DEFINITIONS[config=Debug]

Test plan

Build using react-native 0.73.0-rc.4 on release mode

tomekzaw

Thanks for this PR! Left one minor suggestion, let me know what you think.

cc @piaskowyk we'll need to include it in 3.6.0 for RN 0.73 support

Common/cpp/ReanimatedRuntime/ReanimatedHermesRuntime.h

Common/cpp/ReanimatedRuntime/ReanimatedHermesRuntime.cpp

tjzel · 2023-11-20T12:15:17Z

@gabrieldonadel Thanks for this PR, it helped us a great deal with some other problem we had.

I'm a bit worried about doing config=Debug. If the user has custom debug config (MyDebug) then this macro will not be defined. Could you elaborate if this is some standard way of specifying build flags? (hardcoding for Debug/Release)

Co-authored-by: Tomek Zawadzki <tomekzawadzki98@gmail.com>

gabrieldonadel · 2023-11-20T12:24:27Z

I'm a bit worried about doing config=Debug. If the user has custom debug config (MyDebug) then this macro will not be defined. Could you elaborate if this is some standard way of specifying build flags? (hardcoding for Debug/Release)

Hi @tjzel, if the user had a custom debug config that could indeed be a problem but React-hermes and hermes-engine would also not have these build flags as React native also uses this same logic to set HERMES_ENABLE_DEBUGGER only in debug mode

https://github.com/gabrieldonadel/react-native/blob/3c259240f0ea68a9105be817d71b06f3e046d6d3/packages/react-native/scripts/cocoapods/utils.rb#L44-L47

## 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).

tjzel · 2023-11-20T13:37:43Z

I see. In other ruby scripts RN tends to regex 'Debug' but here where you pointed they just go straight for the literal.

…elease

tjzel · 2023-11-20T14:45:23Z

@gabrieldonadel I took a look into backwards compatibility and on 0.72 and lower we still have to rely on PRODUCTION=1. If the user on 0.72 chooses a Release build in XCode but doesn't do PRODUCTION=1 we wouldn't define HERMES_ENABLE_DEBUGGER when it should be defined.

For consistency - could you add a conditional here that for 0.72 and below we set this flag only when PRODUCTION=1 is not set?

gabrieldonadel · 2023-11-20T19:04:44Z

For consistency - could you add a conditional here that for 0.72 and below we set this flag only when PRODUCTION=1 is not set?

Sure @tjzel, updated

piaskowyk

👏

## Summary This PR fixes error: ```bash [!] Invalid `Podfile` file: [!] Invalid `RNReanimated.podspec` file: undefined local variable or method `config' for Pod:Module Did you mean? concerning. # from /Users/runner/work/react-native-reanimated/react-native-reanimated/Example/node_modules/react-native-reanimated/RNReanimated.podspec:90 # ------------------------------------------- # gcc_debug_definitions = "$(inherited)" > if config[:react_native_minor_version] >= 73 || !is_release # gcc_debug_definitions << " HERMES_ENABLE_DEBUGGER=1" # ------------------------------------------- . # from /Users/runner/work/react-native-reanimated/react-native-reanimated/Example/ios/Podfile:31 # ------------------------------------------- # target 'ReanimatedExample' do > config = use_native_modules! # # ------------------------------------------- ``` Related to PRs: #5383 and #5334 It is because we renamed `config` to global variable `$config` ## Test plan Build Example app --------- Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com>

## 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).

## Summary Building on release mode using react-native 0.73.0-rc.4 results in a build error due to undefined symbols > Undefined symbols for architecture x86_64: "facebook::hermes::inspector_modern::chrome::enableDebugging(std::__1::unique_ptr<facebook::hermes::inspector_modern::RuntimeAdapter, std::__1::default_delete<facebook::hermes::inspector_modern::RuntimeAdapter>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)", referenced from: reanimated::ReanimatedHermesRuntime::ReanimatedHermesRuntime(std::__1::unique_ptr<facebook::hermes::HermesRuntime, std::__1::default_delete<facebook::hermes::HermesRuntime>>, std::__1::shared_ptr<facebook::react::MessageQueueThread> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) in libRNReanimated.a(ReanimatedHermesRuntime.o) "facebook::hermes::inspector_modern::chrome::disableDebugging(int)", referenced from: reanimated::ReanimatedHermesRuntime::~ReanimatedHermesRuntime() in libRNReanimated.a(ReanimatedHermesRuntime.o) This happens because react native only sets `HERMES_ENABLE_DEBUGGER` in debug mode https://github.com/facebook/react-native/blob/23cf10428eb58cfcbf614b8b0728f2535f2d252f/packages/react-native/scripts/cocoapods/utils.rb#L44-L47 So in order to fix this I believe that instead of always adding the `HERMES_ENABLE_DEBUGGER` flag we should only it on debug mode as well, using `GCC_PREPROCESSOR_DEFINITIONS[config=Debug]` ## Test plan Build using react-native 0.73.0-rc.4 on release mode --------- Co-authored-by: Tomek Zawadzki <tomekzawadzki98@gmail.com>

## Summary This PR fixes error: ```bash [!] Invalid `Podfile` file: [!] Invalid `RNReanimated.podspec` file: undefined local variable or method `config' for Pod:Module Did you mean? concerning. # from /Users/runner/work/react-native-reanimated/react-native-reanimated/Example/node_modules/react-native-reanimated/RNReanimated.podspec:90 # ------------------------------------------- # gcc_debug_definitions = "$(inherited)" > if config[:react_native_minor_version] >= 73 || !is_release # gcc_debug_definitions << " HERMES_ENABLE_DEBUGGER=1" # ------------------------------------------- . # from /Users/runner/work/react-native-reanimated/react-native-reanimated/Example/ios/Podfile:31 # ------------------------------------------- # target 'ReanimatedExample' do > config = use_native_modules! # # ------------------------------------------- ``` Related to PRs: #5383 and #5334 It is because we renamed `config` to global variable `$config` ## Test plan Build Example app --------- Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com>

## 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).

## Summary Building on release mode using react-native 0.73.0-rc.4 results in a build error due to undefined symbols > Undefined symbols for architecture x86_64: "facebook::hermes::inspector_modern::chrome::enableDebugging(std::__1::unique_ptr<facebook::hermes::inspector_modern::RuntimeAdapter, std::__1::default_delete<facebook::hermes::inspector_modern::RuntimeAdapter>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)", referenced from: reanimated::ReanimatedHermesRuntime::ReanimatedHermesRuntime(std::__1::unique_ptr<facebook::hermes::HermesRuntime, std::__1::default_delete<facebook::hermes::HermesRuntime>>, std::__1::shared_ptr<facebook::react::MessageQueueThread> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) in libRNReanimated.a(ReanimatedHermesRuntime.o) "facebook::hermes::inspector_modern::chrome::disableDebugging(int)", referenced from: reanimated::ReanimatedHermesRuntime::~ReanimatedHermesRuntime() in libRNReanimated.a(ReanimatedHermesRuntime.o) This happens because react native only sets `HERMES_ENABLE_DEBUGGER` in debug mode https://github.com/facebook/react-native/blob/23cf10428eb58cfcbf614b8b0728f2535f2d252f/packages/react-native/scripts/cocoapods/utils.rb#L44-L47 So in order to fix this I believe that instead of always adding the `HERMES_ENABLE_DEBUGGER` flag we should only it on debug mode as well, using `GCC_PREPROCESSOR_DEFINITIONS[config=Debug]` ## Test plan Build using react-native 0.73.0-rc.4 on release mode --------- Co-authored-by: Tomek Zawadzki <tomekzawadzki98@gmail.com>

## Summary This PR fixes error: ```bash [!] Invalid `Podfile` file: [!] Invalid `RNReanimated.podspec` file: undefined local variable or method `config' for Pod:Module Did you mean? concerning. # from /Users/runner/work/react-native-reanimated/react-native-reanimated/Example/node_modules/react-native-reanimated/RNReanimated.podspec:90 # ------------------------------------------- # gcc_debug_definitions = "$(inherited)" > if config[:react_native_minor_version] >= 73 || !is_release # gcc_debug_definitions << " HERMES_ENABLE_DEBUGGER=1" # ------------------------------------------- . # from /Users/runner/work/react-native-reanimated/react-native-reanimated/Example/ios/Podfile:31 # ------------------------------------------- # target 'ReanimatedExample' do > config = use_native_modules! # # ------------------------------------------- ``` Related to PRs: #5383 and #5334 It is because we renamed `config` to global variable `$config` ## Test plan Build Example app --------- Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com>

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).

Building on release mode using react-native 0.73.0-rc.4 results in a build error due to undefined symbols > Undefined symbols for architecture x86_64: "facebook::hermes::inspector_modern::chrome::enableDebugging(std::__1::unique_ptr<facebook::hermes::inspector_modern::RuntimeAdapter, std::__1::default_delete<facebook::hermes::inspector_modern::RuntimeAdapter>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)", referenced from: reanimated::ReanimatedHermesRuntime::ReanimatedHermesRuntime(std::__1::unique_ptr<facebook::hermes::HermesRuntime, std::__1::default_delete<facebook::hermes::HermesRuntime>>, std::__1::shared_ptr<facebook::react::MessageQueueThread> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) in libRNReanimated.a(ReanimatedHermesRuntime.o) "facebook::hermes::inspector_modern::chrome::disableDebugging(int)", referenced from: reanimated::ReanimatedHermesRuntime::~ReanimatedHermesRuntime() in libRNReanimated.a(ReanimatedHermesRuntime.o) This happens because react native only sets `HERMES_ENABLE_DEBUGGER` in debug mode https://github.com/facebook/react-native/blob/23cf10428eb58cfcbf614b8b0728f2535f2d252f/packages/react-native/scripts/cocoapods/utils.rb#L44-L47 So in order to fix this I believe that instead of always adding the `HERMES_ENABLE_DEBUGGER` flag we should only it on debug mode as well, using `GCC_PREPROCESSOR_DEFINITIONS[config=Debug]` Build using react-native 0.73.0-rc.4 on release mode --------- Co-authored-by: Tomek Zawadzki <tomekzawadzki98@gmail.com>

This PR fixes error: ```bash [!] Invalid `Podfile` file: [!] Invalid `RNReanimated.podspec` file: undefined local variable or method `config' for Pod:Module Did you mean? concerning. # from /Users/runner/work/react-native-reanimated/react-native-reanimated/Example/node_modules/react-native-reanimated/RNReanimated.podspec:90 # ------------------------------------------- # gcc_debug_definitions = "$(inherited)" > if config[:react_native_minor_version] >= 73 || !is_release # gcc_debug_definitions << " HERMES_ENABLE_DEBUGGER=1" # ------------------------------------------- . # from /Users/runner/work/react-native-reanimated/react-native-reanimated/Example/ios/Podfile:31 # ------------------------------------------- # target 'ReanimatedExample' do > config = use_native_modules! # # ------------------------------------------- ``` Related to PRs: #5383 and #5334 It is because we renamed `config` to global variable `$config` Build Example app --------- Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com>

gabrieldonadel force-pushed the @gabrieldonadel/disable-hermes-debugger-on-release branch from 7a59bfa to cda52ed Compare November 16, 2023 20:30

[ios] Only add HERMES_ENABLE_DEBUGGER flag to debug builds

a401321

gabrieldonadel force-pushed the @gabrieldonadel/disable-hermes-debugger-on-release branch from cda52ed to a401321 Compare November 16, 2023 20:36

tomekzaw reviewed Nov 17, 2023

View reviewed changes

Common/cpp/ReanimatedRuntime/ReanimatedHermesRuntime.h Outdated Show resolved Hide resolved

Common/cpp/ReanimatedRuntime/ReanimatedHermesRuntime.cpp Outdated Show resolved Hide resolved

tjzel mentioned this pull request Nov 20, 2023

Fix NDEBUG macro not present in Release builds #5366

Merged

Apply suggestions from code review

620177f

Co-authored-by: Tomek Zawadzki <tomekzawadzki98@gmail.com>

Merge branch 'main' into @gabrieldonadel/disable-hermes-debugger-on-r…

d85fab3

…elease

gabrieldonadel requested a review from tomekzaw November 20, 2023 14:01

Update gcc debug flag logic

0a47707

tjzel approved these changes Nov 21, 2023

View reviewed changes

piaskowyk approved these changes Nov 21, 2023

View reviewed changes

piaskowyk added this pull request to the merge queue Nov 21, 2023

Merged via the queue into software-mansion:main with commit bf7df79 Nov 21, 2023

gabrieldonadel deleted the @gabrieldonadel/disable-hermes-debugger-on-release branch November 21, 2023 13:00

piaskowyk mentioned this pull request Nov 21, 2023

Fix undefined config in podspec #5390

Merged

adrianha mentioned this pull request Dec 1, 2023

[3.6.0] [Reanimated] [iOS] Build Error - Use of undeclared identifier 'debugger' #5446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ios] Only add HERMES_ENABLE_DEBUGGER flag to debug builds#5383

[ios] Only add HERMES_ENABLE_DEBUGGER flag to debug builds#5383
piaskowyk merged 4 commits intosoftware-mansion:mainfrom
gabrieldonadel:@gabrieldonadel/disable-hermes-debugger-on-release

gabrieldonadel commented Nov 16, 2023

Uh oh!

tomekzaw left a comment

Uh oh!

Uh oh!

Uh oh!

tjzel commented Nov 20, 2023

Uh oh!

gabrieldonadel commented Nov 20, 2023

Uh oh!

tjzel commented Nov 20, 2023

Uh oh!

tjzel commented Nov 20, 2023

Uh oh!

gabrieldonadel commented Nov 20, 2023

Uh oh!

piaskowyk left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Uh oh!

Conversation

gabrieldonadel commented Nov 16, 2023

Summary

Test plan

Uh oh!

tomekzaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tjzel commented Nov 20, 2023

Uh oh!

gabrieldonadel commented Nov 20, 2023

Uh oh!

tjzel commented Nov 20, 2023

Uh oh!

tjzel commented Nov 20, 2023

Uh oh!

gabrieldonadel commented Nov 20, 2023

Uh oh!

piaskowyk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments