You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2023/06/23 02:30:24 UTC
[GitHub] [arrow] kou commented on a diff in pull request #35792: GH-20047: [MATLAB] Enable GitHub Actions CI for MATLAB Interface on Windows
kou commented on code in PR #35792:
URL: https://github.com/apache/arrow/pull/35792#discussion_r1239257315
##########
matlab/CMakeLists.txt:
##########
@@ -520,28 +503,48 @@ if(UNIX
# at runtime.
set_target_properties(mexcall PROPERTIES INSTALL_RPATH $ORIGIN)
- if(NOT Arrow_FOUND)
- # If Arrow_FOUND is false, Arrow is built by the arrow_shared target and needs
- # to be copied to CMAKE_PACKAGED_INSTALL_DIR. The DIRECTORY install command is used to
- # install libarrow.so (symlink) and the real files it points to.
- #
- # The subfolders cmake and pkgconfig are excluded as they will be empty.
- # Note: The following CMake Issue suggests enabling an option to exclude all
- # folders that would be empty after installation:
- # https://gitlab.kitware.com/cmake/cmake/-/issues/17122
- install(DIRECTORY "${ARROW_SHARED_LIB_DIR}/"
- DESTINATION ${CMAKE_PACKAGED_INSTALL_DIR}
- FILES_MATCHING
- REGEX "${ARROW_SHARED_LIB_FILENAME}\\.so.*"
- PATTERN "cmake" EXCLUDE
- PATTERN "pkgconfig" EXCLUDE)
-
- # Add $ORIGIN to the RUNPATH of arrow_matlab so that libarrow.so can be found
- # at runtime.
- set_target_properties(arrow_matlab PROPERTIES INSTALL_RPATH $ORIGIN)
+ # Add $ORIGIN to the RUNPATH of arrow_matlab so that libarrow.so can be found
+ # at runtime.
+ set_target_properties(arrow_matlab PROPERTIES INSTALL_RPATH $ORIGIN)
+endif()
+
+if(NOT Arrow_FOUND)
+ # If Arrow_FOUND is false, Arrow is built by the arrow_shared target and needs
+ # to be copied to CMAKE_PACKAGED_INSTALL_DIR.
+
+ if(APPLE)
+ # Install libarrow.dylib (symlink) and the real files it points to.
+ # on MacOS, we need to match these files: libarrow.dylib
Review Comment:
```suggestion
# On macOS, we need to match these files: libarrow.dylib
```
##########
matlab/CMakeLists.txt:
##########
@@ -520,28 +503,48 @@ if(UNIX
# at runtime.
set_target_properties(mexcall PROPERTIES INSTALL_RPATH $ORIGIN)
- if(NOT Arrow_FOUND)
- # If Arrow_FOUND is false, Arrow is built by the arrow_shared target and needs
- # to be copied to CMAKE_PACKAGED_INSTALL_DIR. The DIRECTORY install command is used to
- # install libarrow.so (symlink) and the real files it points to.
- #
- # The subfolders cmake and pkgconfig are excluded as they will be empty.
- # Note: The following CMake Issue suggests enabling an option to exclude all
- # folders that would be empty after installation:
- # https://gitlab.kitware.com/cmake/cmake/-/issues/17122
- install(DIRECTORY "${ARROW_SHARED_LIB_DIR}/"
- DESTINATION ${CMAKE_PACKAGED_INSTALL_DIR}
- FILES_MATCHING
- REGEX "${ARROW_SHARED_LIB_FILENAME}\\.so.*"
- PATTERN "cmake" EXCLUDE
- PATTERN "pkgconfig" EXCLUDE)
-
- # Add $ORIGIN to the RUNPATH of arrow_matlab so that libarrow.so can be found
- # at runtime.
- set_target_properties(arrow_matlab PROPERTIES INSTALL_RPATH $ORIGIN)
+ # Add $ORIGIN to the RUNPATH of arrow_matlab so that libarrow.so can be found
+ # at runtime.
+ set_target_properties(arrow_matlab PROPERTIES INSTALL_RPATH $ORIGIN)
+endif()
+
+if(NOT Arrow_FOUND)
+ # If Arrow_FOUND is false, Arrow is built by the arrow_shared target and needs
+ # to be copied to CMAKE_PACKAGED_INSTALL_DIR.
+
+ if(APPLE)
+ # Install libarrow.dylib (symlink) and the real files it points to.
+ # on MacOS, we need to match these files: libarrow.dylib
+ # libarrow.1300.dylib
+ # libarrow.1300.0.0.dylib
+ # where the version number might change.
+ set(SHARED_LIBRARY_VERSION_REGEX "${ARROW_SHARED_LIB_FILENAME}(([.][0-9]+)?([.][0-9]+)?([.][0-9]+)?)${CMAKE_SHARED_LIBRARY_SUFFIX}")
+ elseif(UNIX
+ AND NOT APPLE
+ AND NOT CYGWIN)
Review Comment:
Do we want to support Cygwin? If we don't, how about removing this condition?
##########
matlab/CMakeLists.txt:
##########
@@ -520,28 +503,48 @@ if(UNIX
# at runtime.
set_target_properties(mexcall PROPERTIES INSTALL_RPATH $ORIGIN)
- if(NOT Arrow_FOUND)
- # If Arrow_FOUND is false, Arrow is built by the arrow_shared target and needs
- # to be copied to CMAKE_PACKAGED_INSTALL_DIR. The DIRECTORY install command is used to
- # install libarrow.so (symlink) and the real files it points to.
- #
- # The subfolders cmake and pkgconfig are excluded as they will be empty.
- # Note: The following CMake Issue suggests enabling an option to exclude all
- # folders that would be empty after installation:
- # https://gitlab.kitware.com/cmake/cmake/-/issues/17122
- install(DIRECTORY "${ARROW_SHARED_LIB_DIR}/"
- DESTINATION ${CMAKE_PACKAGED_INSTALL_DIR}
- FILES_MATCHING
- REGEX "${ARROW_SHARED_LIB_FILENAME}\\.so.*"
- PATTERN "cmake" EXCLUDE
- PATTERN "pkgconfig" EXCLUDE)
-
- # Add $ORIGIN to the RUNPATH of arrow_matlab so that libarrow.so can be found
- # at runtime.
- set_target_properties(arrow_matlab PROPERTIES INSTALL_RPATH $ORIGIN)
+ # Add $ORIGIN to the RUNPATH of arrow_matlab so that libarrow.so can be found
+ # at runtime.
+ set_target_properties(arrow_matlab PROPERTIES INSTALL_RPATH $ORIGIN)
+endif()
+
+if(NOT Arrow_FOUND)
+ # If Arrow_FOUND is false, Arrow is built by the arrow_shared target and needs
+ # to be copied to CMAKE_PACKAGED_INSTALL_DIR.
+
+ if(APPLE)
+ # Install libarrow.dylib (symlink) and the real files it points to.
+ # on MacOS, we need to match these files: libarrow.dylib
+ # libarrow.1300.dylib
+ # libarrow.1300.0.0.dylib
+ # where the version number might change.
+ set(SHARED_LIBRARY_VERSION_REGEX "${ARROW_SHARED_LIB_FILENAME}(([.][0-9]+)?([.][0-9]+)?([.][0-9]+)?)${CMAKE_SHARED_LIBRARY_SUFFIX}")
Review Comment:
Can we simplify this?
```suggestion
set(SHARED_LIBRARY_VERSION_REGEX "${ARROW_SHARED_LIB_FILENAME}([.][0-9]+)*${CMAKE_SHARED_LIBRARY_SUFFIX}")
```
##########
matlab/CMakeLists.txt:
##########
@@ -520,28 +503,48 @@ if(UNIX
# at runtime.
set_target_properties(mexcall PROPERTIES INSTALL_RPATH $ORIGIN)
- if(NOT Arrow_FOUND)
- # If Arrow_FOUND is false, Arrow is built by the arrow_shared target and needs
- # to be copied to CMAKE_PACKAGED_INSTALL_DIR. The DIRECTORY install command is used to
- # install libarrow.so (symlink) and the real files it points to.
- #
- # The subfolders cmake and pkgconfig are excluded as they will be empty.
- # Note: The following CMake Issue suggests enabling an option to exclude all
- # folders that would be empty after installation:
- # https://gitlab.kitware.com/cmake/cmake/-/issues/17122
- install(DIRECTORY "${ARROW_SHARED_LIB_DIR}/"
- DESTINATION ${CMAKE_PACKAGED_INSTALL_DIR}
- FILES_MATCHING
- REGEX "${ARROW_SHARED_LIB_FILENAME}\\.so.*"
- PATTERN "cmake" EXCLUDE
- PATTERN "pkgconfig" EXCLUDE)
-
- # Add $ORIGIN to the RUNPATH of arrow_matlab so that libarrow.so can be found
- # at runtime.
- set_target_properties(arrow_matlab PROPERTIES INSTALL_RPATH $ORIGIN)
+ # Add $ORIGIN to the RUNPATH of arrow_matlab so that libarrow.so can be found
+ # at runtime.
+ set_target_properties(arrow_matlab PROPERTIES INSTALL_RPATH $ORIGIN)
+endif()
+
+if(NOT Arrow_FOUND)
+ # If Arrow_FOUND is false, Arrow is built by the arrow_shared target and needs
+ # to be copied to CMAKE_PACKAGED_INSTALL_DIR.
+
+ if(APPLE)
+ # Install libarrow.dylib (symlink) and the real files it points to.
+ # on MacOS, we need to match these files: libarrow.dylib
+ # libarrow.1300.dylib
+ # libarrow.1300.0.0.dylib
+ # where the version number might change.
+ set(SHARED_LIBRARY_VERSION_REGEX "${ARROW_SHARED_LIB_FILENAME}(([.][0-9]+)?([.][0-9]+)?([.][0-9]+)?)${CMAKE_SHARED_LIBRARY_SUFFIX}")
+ elseif(UNIX
+ AND NOT APPLE
Review Comment:
This is redundant.
```suggestion
```
##########
matlab/CMakeLists.txt:
##########
@@ -520,28 +503,48 @@ if(UNIX
# at runtime.
set_target_properties(mexcall PROPERTIES INSTALL_RPATH $ORIGIN)
- if(NOT Arrow_FOUND)
- # If Arrow_FOUND is false, Arrow is built by the arrow_shared target and needs
- # to be copied to CMAKE_PACKAGED_INSTALL_DIR. The DIRECTORY install command is used to
- # install libarrow.so (symlink) and the real files it points to.
- #
- # The subfolders cmake and pkgconfig are excluded as they will be empty.
- # Note: The following CMake Issue suggests enabling an option to exclude all
- # folders that would be empty after installation:
- # https://gitlab.kitware.com/cmake/cmake/-/issues/17122
- install(DIRECTORY "${ARROW_SHARED_LIB_DIR}/"
- DESTINATION ${CMAKE_PACKAGED_INSTALL_DIR}
- FILES_MATCHING
- REGEX "${ARROW_SHARED_LIB_FILENAME}\\.so.*"
- PATTERN "cmake" EXCLUDE
- PATTERN "pkgconfig" EXCLUDE)
-
- # Add $ORIGIN to the RUNPATH of arrow_matlab so that libarrow.so can be found
- # at runtime.
- set_target_properties(arrow_matlab PROPERTIES INSTALL_RPATH $ORIGIN)
+ # Add $ORIGIN to the RUNPATH of arrow_matlab so that libarrow.so can be found
+ # at runtime.
+ set_target_properties(arrow_matlab PROPERTIES INSTALL_RPATH $ORIGIN)
+endif()
+
+if(NOT Arrow_FOUND)
+ # If Arrow_FOUND is false, Arrow is built by the arrow_shared target and needs
+ # to be copied to CMAKE_PACKAGED_INSTALL_DIR.
+
+ if(APPLE)
+ # Install libarrow.dylib (symlink) and the real files it points to.
+ # on MacOS, we need to match these files: libarrow.dylib
+ # libarrow.1300.dylib
+ # libarrow.1300.0.0.dylib
+ # where the version number might change.
+ set(SHARED_LIBRARY_VERSION_REGEX "${ARROW_SHARED_LIB_FILENAME}(([.][0-9]+)?([.][0-9]+)?([.][0-9]+)?)${CMAKE_SHARED_LIBRARY_SUFFIX}")
+ elseif(UNIX
+ AND NOT APPLE
+ AND NOT CYGWIN)
+ # Install libarrow.so (symlink) and the real files it points to.
+ # On Linux, we need to match these files: libarrow.so
+ # libarrow.so.1200
+ # libarrow.so.1200.0.0
+ # where the version number might change.
+ set(SHARED_LIBRARY_VERSION_REGEX "${ARROW_SHARED_LIB_FILENAME}${CMAKE_SHARED_LIBRARY_SUFFIX}(([.][0-9]+)?([.][0-9]+)?([.][0-9]+)?)")
Review Comment:
Can we simplify this?
```suggestion
set(SHARED_LIBRARY_VERSION_REGEX "${ARROW_SHARED_LIB_FILENAME}${CMAKE_SHARED_LIBRARY_SUFFIX}([.][0-9]+)*")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org