facebook / hermes

A JavaScript engine optimized for running React Native.
https://hermesengine.dev/
MIT License
9.41k stars 596 forks source link

feat: add visionOS support #1358

Closed okwasniewski closed 1 week ago

okwasniewski commented 1 month ago

Summary

This PR adds support for visionOS.

Note: Unreleased version of CMake 3.28.4 is required to build for visionOS. We might need to wait until it gets released.

Things to sort out:

Test Plan

Build test app for visionOS

matthargett commented 1 month ago

both cmake 3.28.4 and 3.29 are released! https://cmake.org/download/

tmikov commented 1 month ago

Out of curiosity, what functionality was needed in CMake?

matthargett commented 1 month ago

Out of curiosity, what functionality was needed in CMake?

There were a few bugs/enhancements like this one when linking a simulator binary versus a real device binary: https://gitlab.kitware.com/cmake/cmake/-/issues/25188

neildhar commented 1 month ago

Thanks for working on this, most of the changes seem safe and in line with the existing implementation (apart from the tests failures). I know this isn't quite ready yet, but we took a look and wanted to share some thoughts.

  1. Why do we need to edit the CircleCI Android job?
  2. Can we avoid using brew update? We deliberately disable automatically running brew update in our CircleCI jobs because we have had problems with it in the past. Could we omit that step? (potentially by using a newer version of the image if necessary)
  3. What happens if an older version of CMake is used? If CMake doesn't already provided a descriptive error, perhaps we should add one.
okwasniewski commented 1 month ago

Hey @neildhar,

  1. For some reason when I updated the file Android job started failing with this error, I followed suggestions from the CircleCI blog post and set it to default: CleanShot 2024-03-25 at 11 40 19@2x
  2. I've updated to Xcode 15.3 image let's see if this will pull newer version of Cmake
  3. The older version of CMake contained a bug that lead to always building a version for the real device which wouldn't allow us to run the Xcode test. If you prefer I can remove Xcode test
okwasniewski commented 1 month ago

So without a brew upgrade, we are installing CMake 3.28.3 (and 3.28.4) is required for Apple Vision 😕

tmikov commented 1 month ago

The problem with running brew update is that we end up potentially performing every test run with different versions of tools.

neildhar commented 1 month ago

@okwasniewski I think the Android image issue should be addressed by #1364, so you should be able to remove those changes from this PR

okwasniewski commented 1 month ago

@neildhar I've rebased the PR - for some reason everything works locally but pod install fails on the CI.

CleanShot 2024-04-02 at 12 34 07@2x

Also regarding brew update we can wait for next circleci image to be released which will include a newer version of CMake as its not possible to install a specific version newer than what homebrew has set in its current version

okwasniewski commented 4 weeks ago

Hey @neildhar, @tmikov

I finally figured out why the build is failing and it's now all green. The last thing left to sort out is the homebrew upgrade. I've removed HOMEBREW_NO_AUTO_UPDATE env variable as it was linking to an issue from 2019 (https://discuss.circleci.com/t/brew-install-fails-while-updating/32992) do you think it's okay to remove it?

neildhar commented 3 weeks ago

Hey @okwasniewski, thanks for getting everything up and passing. Regarding the Homebrew updates, I propose that we leave HOMEBREW_NO_AUTO_UPDATE in the file. Instead, we should split up installing the homebrew packages into two steps, where the second step only installs CMake, pinned to whatever version we need. Then we should run brew update only before the CMake installation, with a comment explaining why.

That way, all packages will continue to be installed at a deterministic version.

okwasniewski commented 3 weeks ago

@neildhar I've applied your comment. CMake is now installed separately.

facebook-github-bot commented 3 weeks ago

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

facebook-github-bot commented 3 weeks ago

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

facebook-github-bot commented 3 weeks ago

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

facebook-github-bot commented 3 weeks ago

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

okwasniewski commented 2 weeks ago

Hey! Is there anything I can do to help you resolve those internal build issues?

neildhar commented 2 weeks ago

Hey @okwasniewski, I tried doing some cleanup internally to minimise changes but can't export them back out to your PR.

Could you apply this patch to the CI config? It addresses some lints and removes changes in jobs that do not build for visionOS.

@@ -5,16 +5,6 @@
   android: circleci/android@2.0.3
   node: circleci/node@4.7.0

-commands:
-  # Install latest CMake version, needed for building Hermes for visionOS which requires at least CMake 3.29.
-  brew-install-cmake:
-    steps:
-      - run:
-          name: Install CMake
-          command: |
-            brew update
-            brew install cmake
-
 workflows:
   version: 2
   build:
@@ -176,11 +166,6 @@
       - restore_cache:
           key: v4-repo-{{ .Environment.CIRCLE_SHA1 }}
       - run:
-          name: Install dependencies
-          command: |
-            brew install ninja
-      - brew-install-cmake
-      - run:
           name: Build the test application
           command: |
             pod install
@@ -227,9 +212,12 @@
       - run:
           name: Install dependencies
           command: |
-            brew install ninja
             sudo gem install cocoapods
-      - brew-install-cmake
+            # TODO: Building for Apple Vision Pro requires a newer version of
+            # CMake than is available by default. Remove "brew update" once the
+            # CircleCI image contains CMake > 3.29.2.
+            brew update
+            brew install cmake
       - run:
           name: Build the iOS frameworks
           command: ./utils/build-ios-framework.sh
@@ -259,9 +247,10 @@
       - run:
           name: Install dependencies
           command: |
-            brew install cmake
             sudo gem install cocoapods
-      - brew-install-cmake
+            # TODO: See comment in build-apple-runtime.
+            brew update
+            brew install cmake
       - run:
           name: Package the framework
           command: |
@@ -289,7 +278,7 @@

   macos:
     macos:
-      xcode: 15.3
+      xcode: 13.4.1
     steps:
       - checkout:
           path: hermes
@@ -327,12 +316,15 @@

   test-macos:
     macos:
-      xcode: 15.3
+      xcode: 13.4.1
     steps:
       - checkout:
           path: hermes
-      - brew-install-cmake
       - run:
+          name: Install dependencies
+          command: |
+            brew install cmake
+      - run:
           name: Run MacOS regression tests in debug mode
           command: |
             cmake -S hermes -B build -GXcode
@@ -701,7 +693,7 @@

   test-macos-test262:
     macos:
-      xcode: 15.3
+      xcode: 13.4.1
     steps:
       - checkout:
           path: hermes
facebook-github-bot commented 1 week ago

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

okwasniewski commented 1 week ago

Hey @neildhar, sorry for the delay I was on time off. I've updated the PR, hopefully the tests will be green now 🤞

facebook-github-bot commented 1 week ago

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

facebook-github-bot commented 1 week ago

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

facebook-github-bot commented 1 week ago

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

facebook-github-bot commented 1 week ago

@neildhar merged this pull request in facebook/hermes@dda343b80784fb75c791979c32ce943f8997dd37.