Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add visionOS support #1358

Closed
wants to merge 1 commit into from

Conversation

okwasniewski
Copy link
Contributor

@okwasniewski okwasniewski commented Mar 19, 2024

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:

  • Add CI

Test Plan

Build test app for visionOS

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Mar 19, 2024
@okwasniewski okwasniewski force-pushed the feat/visionos branch 3 times, most recently from fabb2be to 2d92e23 Compare March 21, 2024 12:11
@matthargett
Copy link

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

@tmikov
Copy link
Contributor

tmikov commented Mar 22, 2024

Out of curiosity, what functionality was needed in CMake?

@@ -24,13 +24,14 @@ Pod::Spec.new do |spec|
# The podspec would be serialized to JSON and people will download prebuilt binaries instead of the source.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think React Native uses this file. It is pretty outdated, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was needed to make the internal hermes tests work and properly build in Xcode

@matthargett
Copy link

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

@okwasniewski okwasniewski force-pushed the feat/visionos branch 9 times, most recently from 0e503ec to 484be5b Compare March 25, 2024 13:30
@neildhar
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@okwasniewski okwasniewski force-pushed the feat/visionos branch 3 times, most recently from f33c5bd to d2f90af Compare March 26, 2024 15:51
@tmikov
Copy link
Contributor

tmikov commented Mar 26, 2024

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

  • Can we specify explicit versions to brew install?
  • If we are building for VisionOS Simulator, can we add a cmake version check. Something like:
if(CMAKE_VERSION VERSION_LESS 3.28.4)
  message("VisionOS Simulator requires CMake version >= 3.28.4")
endif()

@okwasniewski okwasniewski force-pushed the feat/visionos branch 2 times, most recently from 6784b01 to 439d599 Compare March 28, 2024 14:13
@neildhar
Copy link
Contributor

@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 okwasniewski marked this pull request as ready for review April 2, 2024 10:33
@okwasniewski okwasniewski force-pushed the feat/visionos branch 5 times, most recently from 71607be to b9a8dd2 Compare April 24, 2024 10:39
@okwasniewski
Copy link
Contributor Author

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

@facebook-github-bot
Copy link
Contributor

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

.circleci/config.yml Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@okwasniewski
Copy link
Contributor Author

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

@neildhar
Copy link
Contributor

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
Copy link
Contributor

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

@okwasniewski
Copy link
Contributor Author

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

@@ -165,10 +165,6 @@ jobs:
- checkout
- restore_cache:
key: v4-repo-{{ .Environment.CIRCLE_SHA1 }}
- run:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologise, I thought testing didn't require CMake, but it looks like pod install is looking for it. We should add the same thing here with a comment.

@facebook-github-bot
Copy link
Contributor

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

ci: update to Xcode 15.2

feat: add visionos to podspec

add xr framework metadata

fix: set correct target for xros

feat: temp workaround for CMake bug
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@neildhar merged this pull request in dda343b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants