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

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

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

Static Hermes for React Native #1566

Are you sure you want to change the base?

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

neildhar Nov 18, 2024

Choose a reason for hiding this comment

piaskowyk Nov 27, 2024

Choose a reason for hiding this comment

neildhar Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

piaskowyk Dec 11, 2024

Choose a reason for hiding this comment

cipolleschi Nov 18, 2024

Choose a reason for hiding this comment

piaskowyk Nov 27, 2024

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

piaskowyk 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

neildhar Dec 17, 2024

Choose a reason for hiding this comment

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