Static Hermes for React Native #1566

piaskowyk · 2024-11-18T11:09:21Z

Summary

This PR introduces necessary changes to let React Native uses latest version of static Hermes.

Explanation

Part 1

append("/d2UndefIntOverflow-" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)

It seems like this flag doesn’t exist anymore in the MSVC 16 compiler.
CI logs - https://github.com/piaskowyk/react-native/actions/runs/11815096269/job/32915591004

fatal error C1007: unrecognized flag '-UndefIntOverflow-' in 'p2'

Part 2

if("${CMAKE_CXX_COMPILER_ID}" MATCHES "MSVC")
  # MSVC needs C++20
  set(CMAKE_CXX_STANDARD 20)
else()
  set(CMAKE_CXX_STANDARD 17)
endif()

Some of the new syntax in static Hermes requires the newer C++ standard on MSVC.

Part 3

# Changes in lib/CMakeLists.txt

These updates are necessary to successfully build the Hermes Framework for iOS.

Part 4

namespace hermes {
namespace hbc {
namespace {

class BytecodeSerializer {
-  friend void visitBytecodeSegmentsInOrder<BytecodeSerializer>(
+  friend void hermes::hbc::visitBytecodeSegmentsInOrder<BytecodeSerializer>(

Due to additional additional anonymous namespace, the MSVC wasn't able to recognise proper symbol without explicite definition.

Test Plan

Build RNTester app from here - https://github.com/piaskowyk/react-native/tree/%40piaskowyk/build-static-hermes

leecuong666 · 2024-11-18T12:59:44Z

sounds cool. how should I try it?

neildhar

This is awesome, thank you for working on this! I want to make sure I understand how this works, so I have some questions and suggestion.

Is there some simple e2e CI we can set up that builds an RN app to make sure this doesn't break?

CMakeLists.txt

lib/CMakeLists.txt

neildhar · 2024-11-18T16:39:19Z

lib/CMakeLists.txt

+    endif()
+    set(DSYM_PATH "${DSYM_PATH}.dSYM")
+
+    if(NOT CMAKE_DSYMUTIL)


Is CMAKE_DSYMUTIL something that is provided by cmake? If not, we should either remove it (if it can't be configured) or name it something else to avoid confusion.

I'm not entirely sure, but I've reused the implementation from the main branch here. It was added in this PR. If you want, I can look into it further and find out more details.

I think we can just rename it to something else. Like HERMES_DSYMUTIL, so we know that it is hermes specific, but may be configured.

A cleaner solution, if you're changing the RN build scripts anyway, may be to remove the HERMES_BUILD_APPLE_DSYM flag altogether, and instead just pass in -DCMAKE_CXX_FLAGS=-gdwarf from the RN build script, and then manually run the dsymutil from the script. (since this stuff can easily be done in the RN build script instead of in CMake)

Makes sense, so I'll try to move that part to the React Native build script. I'll let you know here when it's ready 🫡

lib/CMakeLists.txt

CMakeLists.txt

cipolleschi · 2024-11-18T17:40:29Z

lib/CMakeLists.txt

+    set_target_properties(hermesvm PROPERTIES
+      FRAMEWORK TRUE
+      VERSION ${PROJECT_VERSION}
+      SOVERSION ${PROJECT_VERSION}
+      FRAMEWORK_VERSION ${PROJECT_VERSION_MAJOR}
+      MACOSX_FRAMEWORK_SHORT_VERSION_STRING ${PROJECT_VERSION}
+      MACOSX_FRAMEWORK_BUNDLE_VERSION ${PROJECT_VERSION}
+      MACOSX_FRAMEWORK_IDENTIFIER dev.hermesengine.${HERMES_APPLE_TARGET_PLATFORM}
+    )


This builds only for MacOS. In React Native we support also iOS, Catalyst, XR and TvOS... 😅

Actually, these changes come from the main branch of Hermes, as shown here. They were introduced in this PR, and you can see the exact changes in this file. Since I'm not as familiar with the iOS build system as you are, I'm open to suggestions if you think changes need to be made 🫡

piaskowyk · 2024-11-27T15:59:36Z

Thanks for the review! 🎉

Is there some simple e2e CI we can set up that builds an RN app to make sure this doesn't break?

I think there isn't anything like that currently. Would you prefer to include it in this PR, or would it be better to handle it in a separate PR to avoid expanding the scope of the current one?

lib/CMakeLists.txt

CMakeLists.txt

neildhar · 2024-12-04T19:13:13Z

Thanks for updating the changes! It mostly looks good, just some minor things.

Does this work with the existing RN build scripts as-is? I'm wondering if we should eliminate HERMES_APPLE_TARGET_PLATFORM in favour of using the more standard CMAKE_SYSTEM_NAME, but only if that wouldn't require disturbing the Hermes build.

Would you prefer to include it in this PR, or would it be better to handle it in a separate PR to avoid expanding the scope of the current one?

That can be in a separate PR. For reference, we have one for Android in the main branch. (link)

piaskowyk · 2024-12-11T14:55:10Z

Does this work with the existing RN build scripts as-is? I'm wondering if we should eliminate HERMES_APPLE_TARGET_PLATFORM in favour of using the more standard CMAKE_SYSTEM_NAME, but only if that wouldn't require disturbing the Hermes build.

If you want, I can replace HERMES_APPLE_TARGET_PLATFORM with CMAKE_SYSTEM_NAME. This isn't a problem because compatibility with React Native requires changes in React Native itself - for example, I needed to update the name of a target, etc. You can see a draft of the changes here: https://github.com/piaskowyk/react-native/compare/main...piaskowyk:react-native:%40piaskowyk/build-static-hermes?expand=1

Update here: #1566 (comment)

That can be in a separate PR. For reference, we have one for Android in the main branch. link

Thanks for the reference! I'll add it 🫡

piaskowyk · 2024-12-11T15:57:32Z

TODO tasks in progress:

Remove HERMES_BUILD_APPLE_DSYM - Static Hermes for React Native #1566 (comment)
~~Eliminate HERMES_APPLE_TARGET_PLATFORM in favour of using the more standard CMAKE_SYSTEM_NAME - Static Hermes for React Native #1566 (comment)~~

piaskowyk · 2024-12-16T12:58:18Z

I've tried to replace HERMES_APPLE_TARGET_PLATFORM with CMAKE_SYSTEM_NAME, but unfortunately, it's more complicated than I initially thought. It seems we need more information about the target than just the build system. For example, you can see the context that necessitates this here: https://github.com/facebook/hermes/pull/1566/files#diff-4803677bd280f4dded52c428e3c8ac6c618508dcabcbe880b68be85897c2ad41R181

piaskowyk · 2024-12-16T13:01:25Z

CMakeLists.txt

@@ -48,7 +48,7 @@ set(CMAKE_OSX_SYSROOT ${HERMES_APPLE_TARGET_PLATFORM})

 if(HERMES_APPLE_TARGET_PLATFORM MATCHES "catalyst")
  set(CMAKE_OSX_SYSROOT "macosx")
-  set(CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS} "-target x86_64-arm64-apple-ios14.0-macabi -isystem ${CMAKE_OSX_SYSROOT}/System/iOSSupport/usr/include")
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -target x86_64-arm64-apple-ios14.0-macabi -isystem ${CMAKE_OSX_SYSROOT}/System/iOSSupport/usr/include")


These changes are intended to avoid concatenation with a semicolon; for example, to prevent strings from merging like -gdwarf;-target x86_64....

facebook-github-bot · 2024-12-17T03:23:00Z

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

neildhar

LGTM, just noting that we need to set CMAKE_C_FLAGS as well

seems we need more information about the target than just the build system

Could you say more about this? I'm not suggesting any changes on this PR, just trying to understand the situation slightly better. CMAKE_SYSTEM_NAME is set to the platform you are targeting. According to the documentation, it has support for iOS, Darwin, tvOS, and visionOS. Is Catalyst the issue?

neildhar · 2024-12-17T03:51:38Z

utils/build-apple-framework.sh

    -DHERMES_BUILD_APPLE_FRAMEWORK:BOOLEAN=true \
-    -DHERMES_BUILD_APPLE_DSYM:BOOLEAN=true \
+    -DCMAKE_CXX_FLAGS="-gdwarf" \


Hermes also contains some C sources, we should set CMAKE_C_FLAGS as well

Updated here: f287d3c

facebook-github-bot · 2024-12-18T11:17:00Z

@piaskowyk has updated the pull request. You must reimport the pull request before landing.

piaskowyk · 2024-12-18T12:13:44Z

Could you say more about this? I'm not suggesting any changes on this PR, just trying to understand the situation slightly better. CMAKE_SYSTEM_NAME is set to the platform you are targeting. According to the documentation, it has support for iOS, Darwin, tvOS, and visionOS. Is Catalyst the issue?

Yes, with the current implementation, the catalyst type is the only problematic area. There are 3 places where HERMES_APPLE_TARGET_PLATFORM is used:

hermes/CMakeLists.txt

Lines 49 to 53 in f287d3c

    
           if(HERMES_APPLE_TARGET_PLATFORM MATCHES "catalyst") 
        
             set(CMAKE_OSX_SYSROOT "macosx") 
        
             set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -target x86_64-arm64-apple-ios14.0-macabi -isystem ${CMAKE_OSX_SYSROOT}/System/iOSSupport/usr/include") 
        
             set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -target x86_64-arm64-apple-ios14.0-macabi -isystem ${CMAKE_OSX_SYSROOT}/System/iOSSupport/usr/include") 
        
             set(CMAKE_THREAD_LIBS_INIT "-lpthread")

hermes/lib/CMakeLists.txt

Lines 189 to 192 in f287d3c

    
             elseif(HERMES_APPLE_TARGET_PLATFORM MATCHES "catalyst") 
        
               add_custom_command(TARGET hermesvm POST_BUILD 
        
                 COMMAND /usr/libexec/PlistBuddy -c "Add :LSMinimumSystemVersion string ${CMAKE_OSX_DEPLOYMENT_TARGET}" $<TARGET_FILE_DIR:hermesvm>/Resources/Info.plist 
        
               )

hermes/tools/node-hermes/nodelib/CMakeLists.txt

Lines 14 to 16 in f287d3c

    
           if(HERMES_APPLE_TARGET_PLATFORM) 
        
             set(CMAKE_CROSSCOMPILING TRUE) 
        
           endif()

The first place could be problematic. The second and third places should be relatively easy to replace with CMAKE_SYSTEM_NAME, I think 🤔

neildhar · 2024-12-18T17:10:31Z

@piaskowyk I suspect this is possible, by looking at a combination of CMAKE_OSX_SYSROOT and CMAKE_SYSTEM_NAME. I believe the last time I looked, CMAKE_SYSTEM_NAME was necessary to support building with the XCode build system (as opposed to make/ninja as we do now).

That said, I certainly don't think we should block integrating SH on it. I'll merge this PR right away, thank you for all of the work you've put into this integration!

facebook-github-bot · 2024-12-18T17:10:46Z

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

Summary: This PR introduces necessary changes to let React Native uses latest version of static Hermes. ## Explanation ### Part 1 ```cmake append("/d2UndefIntOverflow-" CMAKE_C_FLAGS CMAKE_CXX_FLAGS) ``` It seems like this flag doesn’t exist anymore in the MSVC 16 compiler. CI logs - https://github.com/piaskowyk/react-native/actions/runs/11815096269/job/32915591004 ``` fatal error C1007: unrecognized flag '-UndefIntOverflow-' in 'p2' ``` ### Part 2 ```cmake if("${CMAKE_CXX_COMPILER_ID}" MATCHES "MSVC") # MSVC needs C++20 set(CMAKE_CXX_STANDARD 20) else() set(CMAKE_CXX_STANDARD 17) endif() ``` Some of the new syntax in static Hermes requires the newer C++ standard on MSVC. ### Part 3 ```cmake # Changes in lib/CMakeLists.txt ``` These updates are necessary to successfully build the Hermes Framework for iOS. ### Part 4 ```diff namespace hermes { namespace hbc { namespace { class BytecodeSerializer { - friend void visitBytecodeSegmentsInOrder<BytecodeSerializer>( + friend void hermes::hbc::visitBytecodeSegmentsInOrder<BytecodeSerializer>( ``` Due to additional additional anonymous namespace, the MSVC wasn't able to recognise proper symbol without explicite definition. X-link: facebook/hermes#1566 Test Plan: Build RNTester app from here - https://github.com/piaskowyk/react-native/tree/%40piaskowyk/build-static-hermes Reviewed By: tmikov, cipolleschi Differential Revision: D67316013 Pulled By: neildhar

piaskowyk · 2024-12-19T21:36:39Z

Thank you for your guidance! 🤗

If you think it would be beneficial to remove HERMES_APPLE_TARGET_PLATFORM from the codebase, maybe I can explore this topic in the near future 🤔

Summary: X-link: facebook/react-native#48327 This PR introduces necessary changes to let React Native uses latest version of static Hermes. ## Explanation ### Part 1 ```cmake append("/d2UndefIntOverflow-" CMAKE_C_FLAGS CMAKE_CXX_FLAGS) ``` It seems like this flag doesn’t exist anymore in the MSVC 16 compiler. CI logs - https://github.com/piaskowyk/react-native/actions/runs/11815096269/job/32915591004 ``` fatal error C1007: unrecognized flag '-UndefIntOverflow-' in 'p2' ``` ### Part 2 ```cmake if("${CMAKE_CXX_COMPILER_ID}" MATCHES "MSVC") # MSVC needs C++20 set(CMAKE_CXX_STANDARD 20) else() set(CMAKE_CXX_STANDARD 17) endif() ``` Some of the new syntax in static Hermes requires the newer C++ standard on MSVC. ### Part 3 ```cmake # Changes in lib/CMakeLists.txt ``` These updates are necessary to successfully build the Hermes Framework for iOS. ### Part 4 ```diff namespace hermes { namespace hbc { namespace { class BytecodeSerializer { - friend void visitBytecodeSegmentsInOrder<BytecodeSerializer>( + friend void hermes::hbc::visitBytecodeSegmentsInOrder<BytecodeSerializer>( ``` Due to additional additional anonymous namespace, the MSVC wasn't able to recognise proper symbol without explicite definition. Pull Request resolved: #1566 Test Plan: Build RNTester app from here - https://github.com/piaskowyk/react-native/tree/%40piaskowyk/build-static-hermes Reviewed By: tmikov, cipolleschi Differential Revision: D67316013 Pulled By: neildhar fbshipit-source-id: cf03850f94a75acd827b68794700a8f143a90e09

facebook-github-bot · 2024-12-20T00:25:31Z

@neildhar merged this pull request in 74ab1de.

Summary: Pull Request resolved: #48327 This PR introduces necessary changes to let React Native uses latest version of static Hermes. ## Explanation ### Part 1 ```cmake append("/d2UndefIntOverflow-" CMAKE_C_FLAGS CMAKE_CXX_FLAGS) ``` It seems like this flag doesn’t exist anymore in the MSVC 16 compiler. CI logs - https://github.com/piaskowyk/react-native/actions/runs/11815096269/job/32915591004 ``` fatal error C1007: unrecognized flag '-UndefIntOverflow-' in 'p2' ``` ### Part 2 ```cmake if("${CMAKE_CXX_COMPILER_ID}" MATCHES "MSVC") # MSVC needs C++20 set(CMAKE_CXX_STANDARD 20) else() set(CMAKE_CXX_STANDARD 17) endif() ``` Some of the new syntax in static Hermes requires the newer C++ standard on MSVC. ### Part 3 ```cmake # Changes in lib/CMakeLists.txt ``` These updates are necessary to successfully build the Hermes Framework for iOS. ### Part 4 ```diff namespace hermes { namespace hbc { namespace { class BytecodeSerializer { - friend void visitBytecodeSegmentsInOrder<BytecodeSerializer>( + friend void hermes::hbc::visitBytecodeSegmentsInOrder<BytecodeSerializer>( ``` Due to additional additional anonymous namespace, the MSVC wasn't able to recognise proper symbol without explicite definition. X-link: facebook/hermes#1566 Test Plan: Build RNTester app from here - https://github.com/piaskowyk/react-native/tree/%40piaskowyk/build-static-hermes Reviewed By: tmikov, cipolleschi Differential Revision: D67316013 Pulled By: neildhar fbshipit-source-id: cf03850f94a75acd827b68794700a8f143a90e09

tmikov · 2024-12-20T21:43:17Z

Merged in ecfdb2f

Static Hermes for React Native

dc3da45

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Nov 18, 2024

Update syntax

5c393c5

neildhar reviewed Nov 18, 2024

View reviewed changes

cipolleschi reviewed Nov 18, 2024

View reviewed changes

piaskowyk added 3 commits November 27, 2024 13:08

Improvements after review

555c672

Sync Apple cmake with main

f6585e2

Update comment

1755de5

Restore changes

53bb4f4

piaskowyk requested review from cipolleschi and neildhar November 27, 2024 16:04

Change the default value

c384c05

neildhar reviewed Dec 4, 2024

View reviewed changes

lib/CMakeLists.txt Outdated Show resolved Hide resolved

lib/CMakeLists.txt Outdated Show resolved Hide resolved

lib/CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

Review v2

5e41c57

piaskowyk added 2 commits December 16, 2024 13:02

Remove dSYM generation from CMake

30262f4

Update flag

04293c5

piaskowyk requested a review from neildhar December 16, 2024 12:59

piaskowyk commented Dec 16, 2024

View reviewed changes

neildhar reviewed Dec 17, 2024

View reviewed changes

Update c flags

f287d3c

neildhar mentioned this pull request Dec 18, 2024

Static Hermes for React Native facebook/react-native#48327

Closed

facebook-github-bot closed this in 74ab1de Dec 20, 2024

facebook-github-bot added the Merged label Dec 20, 2024

piaskowyk mentioned this pull request Jan 8, 2025

Static Hermes for React Native facebook/react-native#48531

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static Hermes for React Native #1566

Static Hermes for React Native #1566

piaskowyk commented Nov 18, 2024 •

edited

Loading

leecuong666 commented Nov 18, 2024

neildhar left a comment

neildhar Nov 18, 2024

piaskowyk Nov 27, 2024

neildhar Dec 4, 2024 •

edited

Loading

piaskowyk Dec 11, 2024

cipolleschi Nov 18, 2024

piaskowyk Nov 27, 2024

piaskowyk commented Nov 27, 2024

neildhar commented Dec 4, 2024

piaskowyk commented Dec 11, 2024 •

edited

Loading

piaskowyk commented Dec 11, 2024 •

edited

Loading

piaskowyk commented Dec 16, 2024

piaskowyk Dec 16, 2024

facebook-github-bot commented Dec 17, 2024

neildhar left a comment

neildhar Dec 17, 2024

piaskowyk Dec 18, 2024

facebook-github-bot commented Dec 18, 2024

piaskowyk commented Dec 18, 2024 •

edited

Loading

neildhar commented Dec 18, 2024

facebook-github-bot commented Dec 18, 2024

piaskowyk commented Dec 19, 2024

facebook-github-bot commented Dec 20, 2024

tmikov commented Dec 20, 2024

Static Hermes for React Native #1566

Static Hermes for React Native #1566

Conversation

piaskowyk commented Nov 18, 2024 • edited Loading

Summary

Explanation

Part 1

Part 2

Part 3

Part 4

Test Plan

leecuong666 commented Nov 18, 2024

neildhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neildhar Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piaskowyk commented Nov 27, 2024

neildhar commented Dec 4, 2024

piaskowyk commented Dec 11, 2024 • edited Loading

piaskowyk commented Dec 11, 2024 • edited Loading

piaskowyk commented Dec 16, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Dec 17, 2024

neildhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 18, 2024

piaskowyk commented Dec 18, 2024 • edited Loading

neildhar commented Dec 18, 2024

facebook-github-bot commented Dec 18, 2024

piaskowyk commented Dec 19, 2024

facebook-github-bot commented Dec 20, 2024

tmikov commented Dec 20, 2024

piaskowyk commented Nov 18, 2024 •

edited

Loading

neildhar Dec 4, 2024 •

edited

Loading

piaskowyk commented Dec 11, 2024 •

edited

Loading

piaskowyk commented Dec 11, 2024 •

edited

Loading

piaskowyk commented Dec 18, 2024 •

edited

Loading