core: deployment compatibility for old mac after Accelerate New LAPACK fix #25625

fengyuentau · 2024-05-23T03:38:18Z

We may need to explicitly add build option -DCMAKE_OSX_DEPLOYMENT_TARGET=12.0 or environment variable (export MACOSX_DEPLOYMENT_TARGET=12.0) for mac builds (python package most probably) on builders with new macOS (>= 13.3).

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
The PR is proposed to the proper branch
There is a reference to the original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

fengyuentau · 2024-05-23T03:39:13Z

@seanm Feel free to review.

fengyuentau · 2024-05-23T03:40:07Z

@asmorkalov We may need to explicitly add build option -DCMAKE_OSX_DEPLOYMENT_TARGET=12.0 or environment variable (export MACOSX_DEPLOYMENT_TARGET=12.0) for mac builds (python package most probably) on builders with new macOS (>= 13.3).

cmake/OpenCVFindLAPACK.cmake

seanm · 2024-05-24T12:54:22Z

cmake/OpenCVFindLAPACK.cmake

    if(LAPACK_IMPL STREQUAL "LAPACK/Apple" AND NOT IOS) # https://github.com/opencv/opencv/issues/24660
-      # Get macOS version
-      execute_process(COMMAND sw_vers -productVersion
-                      OUTPUT_VARIABLE MACOS_VERSION
-                      OUTPUT_STRIP_TRAILING_WHITESPACE)
-      # Enable Accelerate New LAPACK if macOS >= 13.3
-      if (MACOS_VERSION VERSION_GREATER "13.3" OR MACOS_VERSION VERSION_EQUAL "13.3")
+      # Enable Accelerate New LAPACK if target macOS >= 13.3
+      if(${CMAKE_OSX_DEPLOYMENT_TARGET} VERSION_GREATER "13.3" OR ${CMAKE_OSX_DEPLOYMENT_TARGET} VERSION_EQUAL "13.3")
+        message(STATUS "LAPACK(${LAPACK_IMPL}): Accelerate New LAPACK is enabled.")


Something else has just occurred to me: this won't work on other Darwin variants (iOS, tvOS, visionOS, etc) because they have different version numbers.

For this reason, the NOT IOS check would be better as IS MACOS.

Another alternative is not to do any of this guessing and just let the user/programmer define ACCELERATE_NEW_LAPACK and ACCELERATE_LAPACK_ILP64 if he wants them.

The most important part is that OpenCV's code contains various #ifdef ACCELERATE_NEW_LAPACK conditions to support the newer mode (which I believe you did already).

@asmorkalov What do you think?

Friendly ping. (Our build is broken because of this...)

@fengyuentau maybe it would be good to squash your two commits?

VadimLevin

I think the most straightforward and flexible way is to define CMake dependent option for ACCELERATE_LAPACK_NEW usage and give it reasonable default.

Minimal example:

cmake_minimum_required(VERSION 3.16)

project(LapackOptionCheck LANGUAGES CXX)

include(CMakeDependentOption)

message(STATUS "Current system name: ${CMAKE_SYSTEM_NAME}")
message(STATUS "Current system version: ${CMAKE_OSX_DEPLOYMENT_TARGET}")

# Minimal platform versions to use with new interfaces (taken from
# vecLib/blas_new.h availability check)
# macos(13.3), ios(16.4), watchos(9.4), tvos(16.4)
if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
  set(_min_target_os_version "13.3")
elseif(CMAKE_SYSTEM_NAME STREQUAL "iOS")
  set(_min_target_os_version "16.4")
elseif(CMAKE_SYSTEM_NAME STREQUAL "watchOS")
  set(_min_target_os_version "9.4")
elseif(CMAKE_SYSTEM_NAME STREQUAL "tvOS")
  set(_min_target_os_version "16.4")
else()
  # visioOS has at least version 1.0
  set(_min_target_os_version "1.0")
endif()

message(STATUS "Min target system version: ${_min_target_os_version}")

if(${CMAKE_OSX_DEPLOYMENT_TARGET} VERSION_GREATER_EQUAL ${_min_target_os_version})
  set(_has_required_min_os_version TRUE)
else()
  set(_has_required_min_os_version FALSE)
endif()

cmake_dependent_option(OCV_OSX_USE_ACCELERATE_NEW_LAPACK
  "Use new BLAS/LAPACK interfaces from Accelerate framework on Apple platform"
  ON "APPLE;_has_required_min_os_version" OFF
)

message(STATUS "OCV_OSX_USE_ACCELERATE_NEW_LAPACK: ${OCV_OSX_USE_ACCELERATE_NEW_LAPACK}")

if(OCV_OSX_USE_ACCELERATE_NEW_LAPACK)
  message(STATUS "OpenCV will be built with enabled ACCELERATE_NEW_LAPACK")
endif()

Different cases

Everything is default and minimal system version requirement is fulfilled

cmake . -Bbuild

-- Current system name: Darwin
-- Current system version: 13.5
-- Min target system version: 13.3
-- OCV_OSX_USE_ACCELERATE_NEW_LAPACK: ON
-- OpenCV will be built with enabled ACCELERATE_NEW_LAPACK

Minimal system version requirement is fulfilled, but new interface is manually disabled

cmake -DOCV_OSX_USE_ACCELERATE_NEW_LAPACK=OFF . -Bbuild

-- Current system name: Darwin
-- Current system version: 13.5
-- Min target system version: 13.3
-- OCV_OSX_USE_ACCELERATE_NEW_LAPACK: OFF

Everything is default and minimal system version requirement is not fulfilled

cmake -DCMAKE_SYSTEM_NAME=iOS -DCMAKE_OSX_DEPLOYMENT_TARGET=15.0 . -Bbuild

-- Current system name: iOS
-- Current system version: 15.0
-- Min target system version: 16.4
-- OCV_OSX_USE_ACCELERATE_NEW_LAPACK: OFF

Minimal system version requirement is not fulfilled, so new new interface is manually enabled.

cmake -DCMAKE_SYSTEM_NAME=iOS -DCMAKE_OSX_DEPLOYMENT_TARGET=15.0 -DOCV_OSX_USE_ACCELERATE_NEW_LAPACK=ON . -Bbuild

-- Current system name: iOS
-- Current system version: 15.0
-- Min target system version: 16.4
-- OCV_OSX_USE_ACCELERATE_NEW_LAPACK: OFF

fengyuentau · 2024-05-28T08:30:10Z

I guess @seanm wants the old default which builds without Accelerate New LAPACK.

I think the problem is how we do with default building:

Default to build with new LAPACK. In this case, downstream needs to update if their build host is on new macOS and products are for old and new macOS.
Default to build wtihout new LAPACK. In this case, we need to update our build option to avoid build warnings once our build host is upgraded to new macOS.

VadimLevin · 2024-05-28T08:39:38Z

From the evolution point of view, defaults should give better experience out of the box, backward capability can be kept with the manually specified option.

If we take a look to Apple realm, new option is set to true for the majority of devices (I would say >90%).

seanm · 2024-05-28T17:37:08Z

I guess @seanm wants the old default which builds without Accelerate New LAPACK.

No, I just want to be able to deploy to older versions of macOS.

@VadimLevin's example code above looks good to me.

I think the problem is how we do with default building:

Ultimately, OpenCV should, by default, use CMAKE_OSX_DEPLOYMENT_TARGET to decide if the new LAPACK interfaces should be used or not. Having that OCV_OSX_USE_ACCELERATE_NEW_LAPACK override is not so necessary in my opinion, but I don't oppose it.

@fengyuentau will you update this PR with @VadimLevin's example logic?

fengyuentau · 2024-05-29T07:55:22Z

CMAKE_SYSTEM_NAME

I cannot recall the equivalent in OpenCV CMake. We have only APPLE, IOS, XROS. Not sure if it is suitable to use CMAKE_SYSTEM_NAME directly for this.

VadimLevin · 2024-05-29T08:02:18Z

CMAKE_SYSTEM_NAME

I cannot recall the equivalent in OpenCV CMake. We have only APPLE, IOS, XROS. Not sure if it is suitable to use CMAKE_SYSTEM_NAME directly for this.

It is built-in CMake variable describing target OS, why it should be avoided?

fengyuentau · 2024-05-29T08:14:19Z

It is built-in CMake variable describing target OS, why it should be avoided?

I thought it was for build host. No problem then. Verified with python platforms/ios/build_framework.py ios and I got iOS from CMAKE_SYSTEM_NAME.

seanm · 2024-05-29T12:51:26Z

@fengyuentau can you squash into 1 commit?, I think it would be easier to review the final result integrated.

fengyuentau · 2024-05-29T13:59:31Z

@fengyuentau can you squash into 1 commit?, I think it would be easier to review the final result integrated.

Not a big fan of squashing as it loses commit history and admin can always merge with squashing. Didn't the tab Files changed work for you?

seanm · 2024-05-29T23:19:20Z

cmake/OpenCVFindLAPACK.cmake

-      endif()
+    if(LAPACK_IMPL STREQUAL "LAPACK/Apple" AND OPENCV_OSX_USE_ACCELERATE_NEW_LAPACK)
+      message(STATUS "LAPACK(${LAPACK_IMPL}): Accelerate New LAPACK is enabled.")
+      set(LAPACK_TRY_COMPILE_DEF "-DACCELERATE_NEW_LAPACK")


I don't think it's necessary to try-compile this, but I guess it's not harmful.

Just to keep aligned with what we are doing.

seanm · 2024-05-29T23:21:01Z

Not a big fan of squashing as it loses commit history

Sure, but in this case the history is not valuable, as it was all WIP changes.

Didn't the tab Files changed work for you?

Never used that before, but it was useful, thanks.

This change lgtm and I think it can be merged. (The try-compile I think is unnecessary, but I don't feel strongly about it).

cmake/OpenCVFindLAPACK.cmake

check CMAKE_OSX_DEPLOYMENT_TARGET

07480cb

fengyuentau added category: core category: build/install labels May 23, 2024

fengyuentau added this to the 4.10.0 milestone May 23, 2024

fengyuentau requested review from asmorkalov and opencv-alalek May 23, 2024 03:38

asmorkalov reviewed May 23, 2024

View reviewed changes

cmake/OpenCVFindLAPACK.cmake Outdated Show resolved Hide resolved

simply checks

00af938

seanm reviewed May 24, 2024

View reviewed changes

VadimLevin reviewed May 28, 2024

View reviewed changes

fengyuentau added 2 commits May 29, 2024 17:08

adapt to new control logic

60bff0f

fix CMAKE_OSX_DEPLOYMENT_TARGET with empty value

e965960

fix empty variable _apple_device_min_target_os_version

e50bc99

seanm reviewed May 29, 2024

View reviewed changes

VadimLevin reviewed May 30, 2024

View reviewed changes

cmake/OpenCVFindLAPACK.cmake Show resolved Hide resolved

asmorkalov assigned VadimLevin May 30, 2024

asmorkalov approved these changes May 30, 2024

View reviewed changes

seanm approved these changes May 30, 2024

View reviewed changes

asmorkalov mentioned this pull request May 30, 2024

macOs error: no matching function for call to 'dgeev_' #25675

Closed

4 tasks

VadimLevin approved these changes May 30, 2024

View reviewed changes

asmorkalov merged commit 7e9ef4d into opencv:4.x May 30, 2024
29 of 30 checks passed

fengyuentau deleted the core/deploy_fix_lapack_ilp64 branch May 30, 2024 14:09

fengyuentau mentioned this pull request May 31, 2024

calib3d: fix data type for ilp64 lapack #25682

Merged

6 tasks

mshabunin mentioned this pull request Jun 14, 2024

Merge 4.x -> 5.x #25745

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: deployment compatibility for old mac after Accelerate New LAPACK fix #25625

core: deployment compatibility for old mac after Accelerate New LAPACK fix #25625

fengyuentau commented May 23, 2024

fengyuentau commented May 23, 2024 •

edited

fengyuentau commented May 23, 2024

seanm May 24, 2024 •

edited

fengyuentau May 24, 2024

seanm May 27, 2024

VadimLevin left a comment

fengyuentau commented May 28, 2024

VadimLevin commented May 28, 2024

seanm commented May 28, 2024

fengyuentau commented May 29, 2024

VadimLevin commented May 29, 2024 •

edited

fengyuentau commented May 29, 2024 •

edited

seanm commented May 29, 2024

fengyuentau commented May 29, 2024

seanm May 29, 2024

fengyuentau May 30, 2024

seanm commented May 29, 2024

core: deployment compatibility for old mac after Accelerate New LAPACK fix #25625

core: deployment compatibility for old mac after Accelerate New LAPACK fix #25625

Conversation

fengyuentau commented May 23, 2024

Pull Request Readiness Checklist

fengyuentau commented May 23, 2024 • edited

fengyuentau commented May 23, 2024

seanm May 24, 2024 • edited

Choose a reason for hiding this comment

fengyuentau May 24, 2024

Choose a reason for hiding this comment

seanm May 27, 2024

Choose a reason for hiding this comment

VadimLevin left a comment

Choose a reason for hiding this comment

fengyuentau commented May 28, 2024

VadimLevin commented May 28, 2024

seanm commented May 28, 2024

fengyuentau commented May 29, 2024

VadimLevin commented May 29, 2024 • edited

fengyuentau commented May 29, 2024 • edited

seanm commented May 29, 2024

fengyuentau commented May 29, 2024

seanm May 29, 2024

Choose a reason for hiding this comment

fengyuentau May 30, 2024

Choose a reason for hiding this comment

seanm commented May 29, 2024

fengyuentau commented May 23, 2024 •

edited

seanm May 24, 2024 •

edited

VadimLevin commented May 29, 2024 •

edited

fengyuentau commented May 29, 2024 •

edited