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

Add macOS arm64 CI runner, fix macOS arm64 builds #6695

Merged
merged 15 commits into from
Apr 17, 2024
Merged

Conversation

sitic
Copy link
Contributor

@sitic sitic commented Mar 13, 2024

GitHub macOS arm64 runners are now in public beta. Add them for testing and to build wheels.

Type

Motivation and Context

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

Copy link

update-docs bot commented Mar 13, 2024

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

Copy link
Member

@ssheorey ssheorey left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @sitic Let me know if you need any help, or when it's ready for review.

util/ci_utils.sh Outdated Show resolved Hide resolved
@@ -17,6 +17,9 @@ ExternalProject_Add(
COMMAND ${GIT_EXECUTABLE} init
COMMAND ${GIT_EXECUTABLE} apply --ignore-space-change --ignore-whitespace
${CMAKE_CURRENT_LIST_DIR}/fix-cudacrt.patch
# Patch for macOS ARM64 support for versions < 2.50.0
Copy link
Member

Choose a reason for hiding this comment

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

TODO: Upgrade librealsense to latest so that the patch isn't needed. We can create an issue for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've opened #6714, however builds fail across all platforms currently due to linking and configuration errors. I'll check whats needed to fix them.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Will take a look.

python_version: '3.8'
- is_main: false
- os: macos-14
Copy link
Member

Choose a reason for hiding this comment

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

OK to just move all macOS CI (for feature branches) to Python 3.9 for simplicity (no need to have separate options for Intel vs ARM). For main branch, we would need to keep 3.8-3.11 for Intel and 3.9-3.11 for ARM.

Copy link
Contributor Author

@sitic sitic Mar 24, 2024

Choose a reason for hiding this comment

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

The setup-python action for macOS arm64 currently doesn't work for Python 3.9 unfortunately, see setup-python#808. Pull request python-versions#259 will fix this, once it's merged we can remove the if statements used to exclude python < 3.10 for the arm64 runner.

cwreynolds added a commit to cwreynolds/evoflock that referenced this pull request Mar 19, 2024
Could only get so far before running into isl-org/Open3D#6707 which is due to isl-org/Open3D#6695
@sitic sitic mentioned this pull request Mar 24, 2024
9 tasks
@sitic sitic force-pushed the m1 branch 4 times, most recently from df27b22 to 857ba22 Compare March 24, 2024 22:14
@sitic
Copy link
Contributor Author

sitic commented Mar 24, 2024

MacOS says the arm64 version of the app is damaged when I try to open it:
2024-03-22 19 09 34

xattr -c Open3D.app fixes the issue.

I don't understand why this warning instead of the unsigned developer one comes up. The only difference with the x64 version I see is that in the arm64 app libomp.dylib is dynamically linked and bundled, while the x64 app appears to statically link it.

Code-signing should probably resolve this issue.

@sitic sitic changed the title WIP: Add macOS arm64 CI runner, fix macOS arm64 builds Add macOS arm64 CI runner, fix macOS arm64 builds Mar 24, 2024
@sitic sitic marked this pull request as ready for review March 24, 2024 22:25
@sitic
Copy link
Contributor Author

sitic commented Mar 24, 2024

@ssheorey should be ready for review, I'm waiting for CI to complete

@ssheorey ssheorey requested a review from errissa March 25, 2024 04:28
@ssheorey
Copy link
Member

Hi @errissa can you take a look at this PR, and also test the built artifacts (wheel, viewer, ...) on both Intel and ARM64 macOS systems?

.github/workflows/macos.yml Show resolved Hide resolved
.github/workflows/macos.yml Outdated Show resolved Hide resolved
ln -s $(which gfortran-13) /usr/local/bin/gfortran

# default Xcode 15.0.1 linker causes build issues, embree recommends clang <= 14 for arm64
sudo xcode-select -switch /Applications/Xcode_14.3.1.app
Copy link
Member

Choose a reason for hiding this comment

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

Is this related to libomp? If not, move it before (or after) the libomp customization to clarify that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not. I've moved it up. The default Xcode (15.0.1) for the macos-14 runner uses a new linker (ld-prime) which causes build issues. Newer Xcode versions fix these problems, but I decided to use Xcode 14.3.1 because the embree release mentions possible "EXEC_BAD_INSTRUCTION" runtime exceptions for Xcode >= 15: https://github.com/embree/embree/releases/tag/v4.3.1

I haven't seen these runtime exceptions being triggered. Added all of that as a code comment.

.github/workflows/macos.yml Show resolved Hide resolved
.github/workflows/macos.yml Outdated Show resolved Hide resolved
@@ -17,6 +17,9 @@ ExternalProject_Add(
COMMAND ${GIT_EXECUTABLE} init
COMMAND ${GIT_EXECUTABLE} apply --ignore-space-change --ignore-whitespace
${CMAKE_CURRENT_LIST_DIR}/fix-cudacrt.patch
# Patch for macOS ARM64 support for versions < 2.50.0
Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Will take a look.

cpp/tests/geometry/TriangleMesh.cpp Show resolved Hide resolved
Copy link
Member

@ssheorey ssheorey left a comment

Choose a reason for hiding this comment

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

Tested universal wheels on x86_64. Need testing on arm64 @errissa
Filament arm64 binaries uploaded to open3d_downloads.

@sitic only request would be to create fused binaries for the viewer app and the devel package. Good to merge after that.

docs/getting_started.in.rst Outdated Show resolved Hide resolved
.github/workflows/macos.yml Show resolved Hide resolved
3rdparty/filament/filament_download.cmake Outdated Show resolved Hide resolved
@sitic
Copy link
Contributor Author

sitic commented Apr 1, 2024

I've added a job to merge the x64 and arm64 viewer .app bundles. I don't get the "damaged" warning from my last comment anymore, only the normal downloaded app warning with the universal app.

One remaining difference between the x64 and Apple Silicon builds is WebRTC support, it's currently disabled in the Apple Silicon builds. We should probably fix that. Without fixing that it would be challenging to create universal devel packages because the includes are different.

I've tried running the WebRTC workflow to build the arm64 binaries, for the currently pinned WebRTC commit the builds fail for all runners (CI job) because some upstream build binary seems to be deleted. For a recent WebRTC commit one of the patches to control CXX11 ABI fails. I'll take a look if the WebRTC build is easy to fix.

Another issue for creating an universal devel package is that the INTERFACE_LINK_OPTIONS options of the Open3D::Open3D target in Open3DTargets.cmake are different. The INTERFACE_LINK_OPTIONS contains a lot of \$<LINK_ONLY:\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libz.a>; type of options, but the exact list of libraries is different between x64 and Apple Silicon:

X64 build:

set_target_properties(Open3D::Open3D PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "_GLIBCXX_USE_CXX11_ABI=0;_GLIBCXX_USE_CXX11_ABI=0;FMT_HEADER_ONLY=0;FMT_USE_WINDOWS_H=0;FMT_STRING_ALIAS=1"
  INTERFACE_COMPILE_FEATURES "cxx_std_14"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include/open3d/3rdparty;${_IMPORT_PREFIX}/include/open3d/3rdparty;${_IMPORT_PREFIX}/include/open3d/3rdparty;${_IMPORT_PREFIX}/include/open3d/3rdparty"
  INTERFACE_LINK_LIBRARIES "Open3D::3rdparty_eigen3;Open3D::3rdparty_parallelstl"
  INTERFACE_LINK_OPTIONS "\$<\$<STREQUAL:\$<TARGET_PROPERTY:Open3D::Open3D,TYPE>,STATIC_LIBRARY>:\$<LINK_ONLY:\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libz.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libzmq.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkFiltersGeneral-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkFiltersSources-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkFiltersModeling-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkFiltersCore-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonExecutionModel-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonDataModel-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonTransforms-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonMath-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonMisc-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonSystem-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonCore-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkkissfft-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkpugixml-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtksys-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libUVAtlas.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libmkl_intel_ilp64.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libmkl_tbb_thread.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libmkl_core.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libtbb_static.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libippiw.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libippicv.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libembree4.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libembree_sse42.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libsimd.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,liblexers.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libsys.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libmath.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libtasking.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libwebrtc.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libwebrtc_extra.a>>>"
)

Apple Silicon build:

set_target_properties(Open3D::Open3D PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "_GLIBCXX_USE_CXX11_ABI=0;_GLIBCXX_USE_CXX11_ABI=0;FMT_HEADER_ONLY=0;FMT_USE_WINDOWS_H=0;FMT_STRING_ALIAS=1"
  INTERFACE_COMPILE_FEATURES "cxx_std_14"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include/open3d/3rdparty;${_IMPORT_PREFIX}/include/open3d/3rdparty;${_IMPORT_PREFIX}/include/open3d/3rdparty;${_IMPORT_PREFIX}/include/open3d/3rdparty"
  INTERFACE_LINK_LIBRARIES "Open3D::3rdparty_eigen3;Open3D::3rdparty_parallelstl"
  INTERFACE_LINK_OPTIONS "\$<\$<STREQUAL:\$<TARGET_PROPERTY:Open3D::Open3D,TYPE>,STATIC_LIBRARY>:\$<LINK_ONLY:\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libz.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libzmq.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkFiltersGeneral-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkFiltersSources-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkFiltersModeling-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkFiltersCore-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonExecutionModel-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonDataModel-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonTransforms-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonMath-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonMisc-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonSystem-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkCommonCore-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkkissfft-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtkpugixml-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libvtksys-9.1.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libUVAtlas.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libopenblas.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libembree4.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libembree_avx2.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libsimd.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,liblexers.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libsys.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libmath.a>;\$<\$<CXX_COMPILER_ID:GNU>:LINKER:--exclude-libs,libtasking.a>>>"
)

The WebRTC support and the INTERFACE_LINK_OPTIONS of Open3DTargets.cmake are the only differences otherwise, the .dylib libraries would be easy to merge with lipo -create ARM64/lib/open3d_tf_ops.dylib X64/lib/libOpen3D.dylib -output libOpen3D.dylib.

$ diff -qr ARM64 X64                             
Files ARM64/include/open3d/Open3D.h and X64/include/open3d/Open3D.h differ
Files ARM64/lib/cmake/Open3D/Open3DTargets.cmake and X64/lib/cmake/Open3D/Open3DTargets.cmake differ
Files ARM64/lib/libOpen3D.dylib and X64/lib/libOpen3D.dylib differ
Files ARM64/lib/open3d_tf_ops.dylib and X64/lib/open3d_tf_ops.dylib differ
Files ARM64/lib/open3d_torch_ops.dylib and X64/lib/open3d_torch_ops.dylib differ
Only in X64/share/resources: html

@ssheorey
Copy link
Member

Thanks @sitic I think this PR looks very good already. I'm going to go ahead and merge it. Let's work on adding support for WebRTC (remote rendering) for macOS ARM64 in a new PR. Looks like we may need to update the WebRTC version.

Copy link
Member

@ssheorey ssheorey left a comment

Choose a reason for hiding this comment

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

Tested fused macOS python wheels and fused macOS viewer on Intel macOS.

@ssheorey ssheorey merged commit a4a173e into isl-org:main Apr 17, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants