feat: Add support for tvOS (Apple TV) #1000

douglowder · 2023-05-18T00:55:32Z

Summary

Add support for tvOS, so that Hermes will work with React Native for Apple TV.

Test Plan

New test target ApplePlatformsIntegrationTVTests in the integration test app for Apple platforms, similar to the existing ones for iOS and MacOS.

tmikov · 2023-05-18T02:27:40Z

Thank you! We will review this PR in the next few days. Please keep in mind that even if we merge it, we won't be able to provide actual support (answer questions, provide fixes, etc) for Hermes on tvOS, we will have to rely on the community.

douglowder · 2023-05-18T02:41:51Z

Thanks for reviewing this, @tmikov and the rest of the team :)

I would very much appreciate you merging this so that I can use the Hermes engine as is in the React Native TV repo, and not have to create a fork.

Needless to say, if there are issues, they will probably be coming from people using the React Native TV repo, so I will definitely be available to help with such issues if they arise.

douglowder · 2023-05-18T15:47:56Z

Hmm it seems like there is something weird going on in Circle CI -- the same xcodebuild test invocation that works fine on my desktop can't find an Apple TV simulator in the Circle CI environment.

neildhar · 2023-05-19T00:36:58Z

Thank you for contributing this, looking at the PR, some high level comments and questions:

The iOS build for Hermes has actually been migrated to RN. So changes to Hermes source and our CMake files should be made here, but things like build-ios-framework and build-apple-framework are now maintained by RN (we will delete them from Hermes in the near future). Correspondingly, these should live wherever React Native TV is.
It looks like many of the changes here are to build tools like the hermes CLI, and node-hermes. In general, building these should not be necessary to use Hermes in RN on tvOS, you should just need to build the libhermes target. Is there a reason your PR addresses these other targets?

I also have some questions about specific lines, but these can wait since the above two points may significantly reduce the scope of necessary changes.

douglowder · 2023-05-19T02:44:17Z

The iOS build for Hermes has actually been migrated to RN. So changes to Hermes source and our CMake files should be made here, but things like build-ios-framework and build-apple-framework are now maintained by RN (we will delete them from Hermes in the near future). Correspondingly, these should live wherever React Native TV is.

@neildhar thanks very much for letting me know about the above, and I will make the corresponding changes to those files in the React Native TV repo. I am curious: if you are removing those from Hermes, how will you build and test for any Apple targets in this repo, and therefore prevent regressions that would otherwise only be detected downstream in the React Native repo? Is it possibly your plan to move Hermes into the React Native monorepo as just another package?

It looks like many of the changes here are to build tools like the hermes CLI, and node-hermes. In general, building these should not be necessary to use Hermes in RN on tvOS, you should just need to build the libhermes target. Is there a reason your PR addresses these other targets?

It appeared to me that all these changes were needed in order for build-ios-framework.sh to complete successfully for tvOS. If you could identify which changes are specifically for building the CLI and node-hermes, I'll try removing them from the PR and see if everything is still ok. The difficulty with tvOS is that it is almost iOS but not quite -- certain APIs (even fundamental ones like fork()) are removed and will not compile if they are left in the source by the preprocessor.

neildhar · 2023-05-19T05:34:08Z

prevent regressions that would otherwise only be detected downstream in the React Native repo?

This is a great question. Empirically, this setup has been much more reliable. In the past, we've had problems at the point of producing the Hermes CocoaPod, and consuming it on the RN side. Having that process subsumed into the RN build has eliminated that class of problems entirely.
But to make sure that we don't break the RN build, we need to add a simple end-to-end test in our CI that clones the RN repo and builds RNTester against Hermes source from our repo. We already have such a test for Android, and it's a matter of setting up something similar for iOS. Once we have that, we can eliminate the existing iOS testing, and the build scripts in the Hermes repo, in favour of the end-to-end approach.

which changes are specifically for building the CLI and node-hermes

At the very least, the changes in the node-hermes directory should not be needed. I tried tinkering with some of your changes locally and I do see it complaining about certain system calls. I'd like to give some more thought to how best to address these.

douglowder · 2023-05-19T06:19:45Z

@neildhar I think you absolutely need the changes I put in node-hermes directory. They are to exclude code from Apple TV builds (!TARGET_OS_TV), in the same way that the existing preprocessor directives are excluding code from iOS builds (!TARGET_OS_IPHONE)

neildhar

@douglowder nothing in the node-hermes directory gets built when preparing the Apple frameworks. The library you modified is a third-party library, and had those iOS ifdefs to begin with, but we don't actually build it for iOS in Hermes.

To verify, I checked out your changes and was able to complete a successful build without any of the changes in that directory.

neildhar · 2023-05-19T20:29:35Z

CMakeLists.txt

@@ -57,6 +57,14 @@ if(HERMES_APPLE_TARGET_PLATFORM MATCHES "catalyst")
  set(THREADS_PREFER_PTHREAD_FLAG ON)
 endif()

+if(HERMES_APPLE_TARGET_PLATFORM MATCHES "appletvos")
+  add_definitions(-DTARGET_OS_TV=1)


Why do we need to define this ourselves, that is, why doesn't it defined automatically as it is for iOS?

I don't know the answer to that question..... I just found that it was needed.

I'll back out the node-hermes changes and see if CI succeeds and has the tvOS artifacts.

I think it would be great to understand what's going on with this. Based on some cursory searches, it looks like the tvOS define should be in TargetConditionals.h just like the iOS one. This may suggest something is wrong with how the build is set up, or warrant a comment to avoid confusion in the future.

I will have a look at TargetConditionals.h

I am not able to determine why the build is not picking up the tvOS version of TargetConditionals.h. I will add a comment here and hopefully we can figure that out in the future.

I apologise for the back and forth on this. But we can't merge changes that we don't really understand. In this instance, it may suggest a subtle issue in the build setup that could break in the future, or add overhead when managing the integration of our Apple builds with upstream RN.

I totally understand. I'll close this for now, and once the React Native TV 0.71 release is out, will come back to this and see what is going on with TargetConditionals. My guess is that somewhere in the Hermes build, the Apple CMake configuration is picking up iPhone includes instead of Apple TV includes.

neildhar

Thank you for making the changes to the node-hermes directory. I've done a pass over the individual ifdefs to get your input.

external/icu_decls/unicode/platform.h

neildhar · 2023-05-23T04:27:16Z

external/llvh/lib/Support/Unix/Program.inc

@@ -200,7 +200,11 @@ static bool Execute(ProcessInfo &PI, StringRef Program,
  // If this OS has posix_spawn and there is no memory limit being implied, use
  // posix_spawn.  It is more efficient than fork/exec.
 #ifdef HAVE_POSIX_SPAWN
+#if defined(TARGET_OS_TV) && TARGET_OS_TV


What is the issue being worked around here?

Apple TV needs to use posix_spawn (does not have fork()) but MemoryLimit did not exist for Apple TV (at least at the time I was working on this patch).

What do you mean? MemoryLimit is a parameter to this function.

Sorry I didn't say that correctly. Apple TV needs to use this path even if MemoryLimit is not 0.

lib/Support/OSCompatPosix.cpp

external/llvh/lib/Support/Unix/Program.inc

neildhar · 2023-05-23T04:46:51Z

CMakeLists.txt

@@ -57,6 +57,14 @@ if(HERMES_APPLE_TARGET_PLATFORM MATCHES "catalyst")
  set(THREADS_PREFER_PTHREAD_FLAG ON)
 endif()

+if(HERMES_APPLE_TARGET_PLATFORM MATCHES "appletvos")
+  add_definitions(-DTARGET_OS_TV=1)


I think it would be great to understand what's going on with this. Based on some cursory searches, it looks like the tvOS define should be in TargetConditionals.h just like the iOS one. This may suggest something is wrong with how the build is set up, or warrant a comment to avoid confusion in the future.

external/llvh/lib/Support/Unix/Program.inc

neildhar · 2023-05-24T20:20:45Z

CMakeLists.txt

@@ -57,6 +57,14 @@ if(HERMES_APPLE_TARGET_PLATFORM MATCHES "catalyst")
  set(THREADS_PREFER_PTHREAD_FLAG ON)
 endif()

+if(HERMES_APPLE_TARGET_PLATFORM MATCHES "appletvos")
+  add_definitions(-DTARGET_OS_TV=1)


I apologise for the back and forth on this. But we can't merge changes that we don't really understand. In this instance, it may suggest a subtle issue in the build setup that could break in the future, or add overhead when managing the integration of our Apple builds with upstream RN.

neildhar · 2023-05-24T20:25:49Z

lib/Support/OSCompatPosix.cpp

@@ -778,10 +778,12 @@ bool unset_env(const char *name) {
 void *SigAltStackLeakSuppressor::stackRoot_{nullptr};

 SigAltStackLeakSuppressor::~SigAltStackLeakSuppressor() {
+#if defined(__APPLE__) && !(defined(TARGET_OS_TV) && TARGET_OS_TV)


My suggestion was to use just defined(__APPLE__) && !TARGET_OS_TV. As noted in my previous comment, doing this correctly also ensures that TargetConditionals.h is properly imported.

In addition to the suggested change above, we should also add a comment that this is not supported on tvOS.

neildhar · 2023-05-24T20:31:11Z

external/llvh/lib/Support/Unix/Program.inc

@@ -200,7 +200,11 @@ static bool Execute(ProcessInfo &PI, StringRef Program,
  // If this OS has posix_spawn and there is no memory limit being implied, use
  // posix_spawn.  It is more efficient than fork/exec.
 #ifdef HAVE_POSIX_SPAWN
+#if defined(__APPLE__) && defined(TARGET_OS_TV) && TARGET_OS_TV


Thank you for explaining what was going on with MemoryLimit here. I think the right thing to do here would be to restrict the ifdef to just the region below with the fork, and then use ErrMsg to report an error if we're on tvOS and a MemoryLimit is specified. I suspect something similar would happen on iOS anyway, since I don't think we're allowed to actually call fork.

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

tmikov mentioned this pull request May 18, 2023

AppleTV support #704

Closed

neildhar reviewed May 19, 2023

View reviewed changes

douglowder added 8 commits May 19, 2023 14:41

Apple TV support

3cd0031

Add TV to ApplePlatformsIntegrationTests

7a0397b

Add TV test to Circle CI

d16673e

Fix CircleCI tvOS test

07c6413

Add another tvOS/iOS Hermes integration test

c3957a9

Another attempt at fixing CircleCI

fe91875

Comment out TV test

4958470

Back out node-hermes changes

4a57d64

douglowder force-pushed the tvos branch from b9015f5 to 4a57d64 Compare May 19, 2023 21:42

Merge branch 'main' into tvos

ea5d380

douglowder requested a review from neildhar May 22, 2023 23:27

neildhar reviewed May 23, 2023

View reviewed changes

douglowder commented May 23, 2023

View reviewed changes

external/llvh/lib/Support/Unix/Program.inc Outdated Show resolved Hide resolved

Respond to review comments

da0be0e

douglowder requested a review from neildhar May 24, 2023 01:07

Merge branch 'main' into tvos

c311108

neildhar requested changes May 24, 2023

View reviewed changes

douglowder closed this May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for tvOS (Apple TV) #1000

feat: Add support for tvOS (Apple TV) #1000

douglowder commented May 18, 2023

tmikov commented May 18, 2023

douglowder commented May 18, 2023

douglowder commented May 18, 2023

neildhar commented May 19, 2023

douglowder commented May 19, 2023 •

edited

neildhar commented May 19, 2023

douglowder commented May 19, 2023

neildhar left a comment

neildhar May 19, 2023

douglowder May 19, 2023

douglowder May 19, 2023

neildhar May 23, 2023

douglowder May 23, 2023

douglowder May 23, 2023

neildhar May 24, 2023

douglowder May 24, 2023

neildhar left a comment

neildhar May 23, 2023

douglowder May 23, 2023

neildhar May 23, 2023

douglowder May 23, 2023

neildhar May 23, 2023

neildhar May 24, 2023

neildhar May 24, 2023

neildhar May 24, 2023

feat: Add support for tvOS (Apple TV) #1000

feat: Add support for tvOS (Apple TV) #1000

Conversation

douglowder commented May 18, 2023

Summary

Test Plan

tmikov commented May 18, 2023

douglowder commented May 18, 2023

douglowder commented May 18, 2023

neildhar commented May 19, 2023

douglowder commented May 19, 2023 • edited

neildhar commented May 19, 2023

douglowder commented May 19, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douglowder commented May 19, 2023 •

edited