sgilmore10 commented on a change in pull request #10305:
URL: https://github.com/apache/arrow/pull/10305#discussion_r658299069
##########
File path: matlab/CMakeLists.txt
##########
@@ -29,22 +30,51 @@ if(EXISTS "${CPP_CMAKE_MODULES}")
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CPP_CMAKE_MODULES})
endif()
-## Arrow is Required
+set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
+
+# Arrow is Required
find_package(Arrow REQUIRED)
-## MATLAB is required to be installed to build MEX interfaces
-set(MATLAB_ADDITIONAL_VERSIONS "R2018a=9.4")
-find_package(Matlab REQUIRED MX_LIBRARY)
-
-# Build featherread mex file based on the arrow shared library
-matlab_add_mex(NAME featherreadmex
- SRC src/featherreadmex.cc src/feather_reader.cc
src/util/handle_status.cc
- src/util/unicode_conversion.cc
- LINK_TO ${ARROW_SHARED_LIB})
-target_include_directories(featherreadmex PRIVATE ${ARROW_INCLUDE_DIR})
-
-# Build featherwrite mex file based on the arrow shared library
-matlab_add_mex(NAME featherwritemex
- SRC src/featherwritemex.cc src/feather_writer.cc
src/util/handle_status.cc
- LINK_TO ${ARROW_SHARED_LIB})
-target_include_directories(featherwritemex PRIVATE ${ARROW_INCLUDE_DIR})
+# MATLAB is Required
+find_package(Matlab REQUIRED)
+
+# Construct the absolute path to featherread's source files
+set(featherread_sources featherreadmex.cc feather_reader.cc
util/handle_status.cc
+ util/unicode_conversion.cc)
+list(TRANSFORM featherread_sources PREPEND ${CMAKE_SOURCE_DIR}/src/)
+
+# Build featherreadmex MEX binary
+matlab_add_mex(R2018a
+ NAME
+ featherreadmex
+ SRC
+ ${featherread_sources}
+ LINK_TO
+ arrow_shared)
+
+# Construct the absolute path to featherwrite's source files
+set(featherwrite_sources featherwritemex.cc feather_writer.cc
util/handle_status.cc
+ util/unicode_conversion.cc)
+list(TRANSFORM featherwrite_sources PREPEND ${CMAKE_SOURCE_DIR}/src/)
+
+# Build featherwritemex MEX binary
+matlab_add_mex(R2018a
+ NAME
+ featherwritemex
+ SRC
+ ${featherwrite_sources}
+ LINK_TO
+ arrow_shared)
+
+# Ensure the MEX binaries are placed in the src directory on all platforms
Review comment:
We could add the build folder to the MATLAB search path. However, this
will make harder to run our unit tests automatically because we would require
the user to explicitly tell us where their build files are located every time.
Additionally, as the MATLAB interface grows, we will have many MEX files. In
order to keep things organized. We see two approaches:
1) We can encode the relationship of MEX files to MATLAB classes via the MEX
file name itself. For example, arrow_array_new.mexw64.
or
2) The other option is to encode the relationship between the two via the
MATLAB packaging mechanism. For example, the folder structure
+matlab/+array/+mex exposes a package called matlab.array.mex to MATLAB. If we
choose this option, we can reuse common names, such as "new", and keep the
names short.
Option 2 seems more scalable, but if you're experience tells us this will
lead to issues, we can revisit adding the build folder to the path.
Best,
Sarah
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
