Details

Reviewers

Group Reviewers

Commits

rB652fbc200500: macOS: Portable builds with dynamic libraries.

Summary

Depends on lib update after D11748

For Blender.app: dropping libomp.dylib next to Blender executable is
enough for it getting picked up since @executable_path is a rpath.

For non-distributed binaries datatoc, makesdna, tests etc, code for
copying libomp.dylib to build folder is removed and replaced by
CMake's rpath option for build tree.

For bpy.so, the post build rpath change has also been replaced by CMake
install rpath option.
If portable, the dylib folder is bpy.so/../../Resources/{version}/lib.
If not portable, site-packages/Resources/{version}/lib.

Since -id has been changed in D11748, remove the
install_name_tool -change ... command.

Any dylib can just be dropped at MAC_BLENDER_TARGET_DYLIBS_DIR
hereafter. Appending dylib path to CMAKE_BUILD_RPATH will be needed
for datatoc if linked against one (instead of copying the dylib around).

[1]

diff --git b/build_files/cmake/platform/platform_apple.cmake a/build_files/cmake/platform/platform_apple.cmake
index d844a8cc86e..e3ae633d072 100644
--- b/build_files/cmake/platform/platform_apple.cmake
+++ a/build_files/cmake/platform/platform_apple.cmake
@@ -413,6 +413,27 @@ if(WITH_OPENMP)
     set(OpenMP_CXX_FLAGS "-Xclang -fopenmp -I'${LIBDIR}/openmp/include'")
     set(OpenMP_LINKER_FLAGS "-L'${LIBDIR}/openmp/lib' -lomp")
     set(OpenMP_LIBRARY "${LIBDIR}/openmp/lib/libomp.dylib")
+
+    # Copy libomp.dylib to allow executables like datatoc and tests to work.
+    # `@rpath/libomp.dylib` as `LC_ID_DYLIB` is added by the deps builder.
+    if(GENERATOR_IS_MULTI_CONFIG)
+      set(_openmp_CONFIGS Debug Release MinSizeRel RelWithDebInfo)
+    else()
+      set(_openmp_CONFIGS " ")
+    endif()
+    foreach(_config ${_openmp_CONFIGS})
+      # This will make single-config generator's config empty and that's intended. Multiple `/`s are considered as one.
+      string(STRIP ${_config} _config)
+      execute_process(
+        # datatoc, makesdna etc.
+        COMMAND ${CMAKE_COMMAND} -E copy "${OpenMP_LIBRARY}" "${EXECUTABLE_OUTPUT_PATH}/${_config}/libomp.dylib"
+        # Tests.
+        COMMAND ${CMAKE_COMMAND} -E copy "${OpenMP_LIBRARY}" "${EXECUTABLE_OUTPUT_PATH}/tests/${_config}/libomp.dylib"
+        # For Blender.app, dylib copy is done as an install step.
+      )
+    endforeach()
+    unset(_config)
+    unset(_openmp_CONFIGS)
   endif()
 endif()

Diff Detail

Repository

rB Blender

Branch

openmp_build (branched from master)

Build Status

Buildable 16016
Build 16016: arc lint + arc unit

Event Timeline

Ankit Meel (ankitm) requested review of this revision.Jul 22 2021, 2:09 PM

Ankit Meel (ankitm) created this revision.

Ankit Meel (ankitm) added a reviewer: Platform: macOS.

What is the motivation for having portable asan builds?

I'd like to make ASAN builds of my branch for users to test.

https://blender.chat/channel/blender-coders?msg=h2Ad9dggTBdooZb74
CC @Joseph Eagar (joeedh)

Ok, that's a good reason.

source/creator/CMakeLists.txt

1042–1045

WITH_PYTHON_MODULE sets a different rpath later, it should be one or the other.

I think a better solution would be (untested):

 # Add rpath for libraries like OpenMP, Address Sanitizer.
if(WITH_PYTHON_MODULE)
   set(_macos_lib_path "Resources/${BLENDER_VERSION}/lib")
else()
   set(_macos_lib_path "Resources/lib")
endif()
add_custom_command(TARGET blender POST_BUILD
  COMMAND xcrun install_name_tool -add_rpath "@loader_path/../${_macos_lib_path}" "$<TARGET_FILE:blender>")

...

if(WITH_OPENMP AND OPENMP_CUSTOM)
  if(WITH_PYTHON_MODULE)
   ...
    add_custom_command(TARGET blender POST_BUILD
      # Change hardcoded path in precompiled libomp.dylib to point to rpath.
      COMMAND xcrun install_name_tool -change "@executable_path/../Resources/lib/libomp.dylib" "@rpath/libomp.dylib" "$<TARGET_FILE:blender>")

...

if(WITH_COMPILER_ASAN)
  install(
    FILES ${COMPILER_ASAN_LIBRARY}
    DESTINATION "$<TARGET_BUNDLE_CONTENT_DIR:blender>/${_macos_lib_path}"
  )
endif()

WITH_PYTHON_MODULE sets a different rpath later, it should be one or the other.

Binaries can have multiple LC_RPATH entries.
Sure for code simplicity setting it once is better.

Also, the " # Change hardcoded path in precompiled libomp.dylib to point to rpath." command can be removed if we change it in
https://developer.blender.org/diffusion/B/browse/master/build_files/build_environment/cmake/openmp.cmake$27
rpaths are more flexible than @executable_path/../Resources/lib IMO

need to test asan + bpy also.

In D11997#309525, @Ankit Meel (ankitm) wrote:

Also, the " # Change hardcoded path in precompiled libomp.dylib to point to rpath." command can be removed if we change it in
https://developer.blender.org/diffusion/B/browse/master/build_files/build_environment/cmake/openmp.cmake$27
rpaths are more flexible than @executable_path/../Resources/lib IMO

Agree that would be better, but it requires a library update.

Ankit Meel (ankitm) mentioned this in D11748: Deps builder: OIIO/OSL/ISPC/OIDN/LLVM/WinFlex updates + add Flex.Jul 23 2021, 8:12 PM

Ankit Meel (ankitm) removed a subscriber: Joseph Eagar (joeedh).Jul 23 2021, 8:13 PM

macOS: Portable builds with dynamic libraries.
Remove openmp copying-while-configuring code. It seems that it's not needed if binaries are run on builder machine itself.

Harbormaster completed remote builds in B16003: Diff 39891.Jul 23 2021, 8:13 PM

Ankit Meel (ankitm) retitled this revision from macOS: Portable Address Sanitizer builds to macOS: Portable builds with dynamic libraries..Jul 23 2021, 8:14 PM

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)Jul 23 2021, 8:22 PM

Nice cleanup!

This revision is now accepted and ready to land.Jul 23 2021, 9:00 PM

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)Jul 24 2021, 11:58 AM

Set rpath for bpy that works.
set rpath for Blender executable. Merge the two.
Remove post install rpath manipulation.

Harbormaster completed remote builds in B16016: Diff 39912.Jul 24 2021, 11:58 AM

Some corrections, hopefully for the better.
My tests in last update were flawed because blender/ datatoc etc were loading libomp.dylib from /usr/local/lib/libomp.dylib🤯 (which comes from brew).
So after several rebuilds and cmake rpath options fiddling, here's a way to avoid copying dylibs in build folder, and avoid post build rpath manipulation.
Tested by renaming installed dylibs and verifying that launch fails. but still would keep an eye on new builds by buildbot after patch lands.

thanks to

Ankit Meel (ankitm) requested review of this revision.Jul 24 2021, 8:19 PM

Anton Raves (Memento) added a subscriber: Anton Raves (Memento).Jul 24 2021, 9:44 PM

Brecht Van Lommel (brecht) requested changes to this revision.Jul 26 2021, 1:26 PM

Brecht Van Lommel (brecht) added inline comments.

build_files/cmake/platform/platform_apple.cmake
505–510	Will this also add the library dir rpath for the Blender.app executable? I see nothing preventing it.

This revision now requires changes to proceed.Jul 26 2021, 1:26 PM

otool output for datatoc: https://pasteall.org/9iF5 (affected by CMAKE_BUILD_RPATH)
same for blender: https://pasteall.org/rpm1 (affected by empty CMAKE_INSTALL_RPATH) It's the same when building blender and install.

Used https://stackoverflow.com/questions/32183975/how-to-print-all-the-properties-of-a-target-in-cmake to get properties on blender too: https://pasteall.org/nAwo
Since BUILD_WITH_INSTALL_RPATH is set to true (in source/creator/CMakeLists.txt itself), it takes precedence over BUILD_RPATH. Even if that wasn't true, INSTALL_RPATH would save the day as it is empty.
docs:
BUILD_RPATH
INSTALL_RPATH
CMAKE_SKIP_BUILD_RPATH
CMAKE_SKIP_INSTALL_RPATH
BUILD_WITH_INSTALL_RPATH

Ah ok, I understand it now.

build_files/cmake/platform/platform_apple.cmake

501–504

I suggest to change the comment, this would have been more clear to me.

# For binaries that are not distributed (datatoc, makesdna, tests), we add an
# absolute path to the OpenMP library dir in CMAKE_SKIP_BUILD_RPATH. This avoids
# having to make many copies of the dylib next to each binary.
#
# For the installed Blender executable CMAKE_INSTALL_RPATH will be used, but
# needs no changes since it already looks for dylibs next to the executable by
# default.
#
# For the installed Python module, CMAKE_INSTALL_RPATH is modified to find the
# dylib in an adjacent folder.

This revision is now accepted and ready to land.Jul 26 2021, 6:28 PM