kevingurney commented on code in PR #34563:
URL: https://github.com/apache/arrow/pull/34563#discussion_r1173944277
##########
matlab/CMakeLists.txt:
##########
@@ -317,6 +324,51 @@ matlab_add_mex(R2018a
target_include_directories(mexcall PRIVATE ${CPP_SOURCE_DIR})
+# ARROW_SHARED_LIB
+# On Windows, this will be ARROW_HOME/bin/arrow.dll and on Linux and macOS, it
is the arrow.so/dylib in the newly built arrow_shared library.
+if(NOT Arrow_FOUND)
+ message(STATUS "ARROW_SHARED_LIB will be set using IMPORTED_LOCATION value
when building."
+ )
+ get_target_property(ARROW_SHARED_LIB arrow_shared IMPORTED_LOCATION)
+else()
+ # If not building Arrow, ARROW_SHARED_LIB derived from ARROW_PREFIX set to
the ARROW_HOME specified with cmake would be non-empty.
+ message(STATUS "ARROW_SHARED_LIB: ${ARROW_SHARED_LIB}")
+endif()
+
+# ARROW_LINK_LIB
+# On Windows, we use the arrow.lib for linking arrow_matlab against the Arrow
C++ library.
+# The location of arrow.lib is previously saved in IMPORTED_IMPLIB.
+if(WIN32)
+ # If not building Arrow, IMPORTED_IMPLIB will be empty.
+ # Then set ARROW_LINK_LIB to ARROW_IMPORT_LIB which would have been derived
from ARROW_PREFIX set to the ARROW_HOME specified with cmake. This will avoid
the ARROW_LINK_LIB set to NOTFOUND error.
+ # The ARROW_IMPORT_LIB should be ARROW_HOME/lib/arrow.lib on Windows.
+ if(NOT Arrow_FOUND)
+ message(STATUS "ARROW_LINK_LIB will be set using IMPORTED_IMPLIB value
when building."
+ )
+ get_target_property(ARROW_LINK_LIB arrow_shared IMPORTED_IMPLIB)
+ else()
+ set(ARROW_LINK_LIB "${ARROW_IMPORT_LIB}")
+ message(STATUS "Setting ARROW_LINK_LIB to ARROW_IMPORT_LIB:
${ARROW_IMPORT_LIB}, which is derived from the ARROW_HOME provided."
+ )
+ endif()
+else()
+ # On Linux and macOS, it is the arrow.so/dylib in the newly built
arrow_shared library used for linking.
+ # On Unix, this is the same as ARROW_SHARED_LIB.
+ message(STATUS "Setting ARROW_LINK_LIB to ARROW_SHARED_LIB as they are same
on Unix.")
+ set(ARROW_LINK_LIB "${ARROW_SHARED_LIB}")
+endif()
+
+# ARROW_INCLUDE_DIR should be set so that header files in the include
directory can be found correctly.
+# The value of ARROW_INCLUDE_DIR should be ARROW_HOME/include on all platforms.
+if(NOT Arrow_FOUND)
+ message(STATUS "ARROW_INCLUDE_DIR will be set using
INTERFACE_INCLUDE_DIRECTORIES value when building."
+ )
+ get_target_property(ARROW_INCLUDE_DIR arrow_shared
INTERFACE_INCLUDE_DIRECTORIES)
+else()
+ # If not building Arrow, ARROW_INCLUDE_DIR derived from ARROW_PREFIX set to
the ARROW_HOME specified with cmake would be non-empty.
+ message(STATUS "ARROW_INCLUDE_DIR: ${ARROW_INCLUDE_DIR}")
+endif()
+
Review Comment:
Thanks for the suggestion, @kou!
Just to clarify, are you suggesting that we should use `Arrow::arrow_shared`
in the `else` clauses where `Arrow_FOUND` is `true` for printing debug
messages? Or are you suggesting, more generally, that we don't set imported
target properties separately on `arrow_shared`?
For reference, when building Arrow from source using `ExternalProject_Add`
(i.e. when `find_package(Arrow)` fails to find a valid Arrow installation), the
imported target that is created from the `ExternalProject_Add` build artifacts
is called `arrow_shared`. If we start using `Arrow::arrow_shared`, there would
then be two possible different targets: (1) `Arrow::arrow_shared` and (2)
`arrow_shared`, which might complicate the logic of the build system (although,
that doesn't necessarily mean we shouldn't make this change - but, it seems
like a consideration worth keeping in mind).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
