[GitHub] [arrow] kevingurney commented on a diff in pull request #34563: GH-33854: [MATLAB] Add basic libmexclass integration code to MATLAB interface

["kevingurney (via GitHub)" <gi...@apache.org>]

kevingurney commented on code in PR #34563:
URL: https://github.com/apache/arrow/pull/34563#discussion_r1139036861


##########
matlab/tools/cmake/BuildMatlabArrowInterface.cmake:
##########
@@ -0,0 +1,80 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# -------
+# Config
+# -------
+
+# Build configuration for libmexclass.
+set(CUSTOM_PROXY_FACTORY_INCLUDE_DIR "${CMAKE_SOURCE_DIR}/src/cpp/arrow/matlab/proxy;${CMAKE_SOURCE_DIR}/src/cpp")
+set(CUSTOM_PROXY_FACTORY_SOURCES "${CMAKE_SOURCE_DIR}/src/cpp/arrow/matlab/proxy/factory.cc")
+set(CUSTOM_PROXY_SOURCES "${CMAKE_SOURCE_DIR}/src/cpp/arrow/matlab/array/proxy/float64_array.cc")
+set(CUSTOM_PROXY_INCLUDE_DIR "${CMAKE_SOURCE_DIR}/src/cpp;${ARROW_INCLUDE_DIR}")
+set(CUSTOM_PROXY_LINK_LIBRARIES ${ARROW_LINK_LIB})
+# On Windows, arrow.dll must be installed regardless of
+# whether Arrow_FOUND is true or false. Therefore, we explicitly
+# copy ARROW_SHARED_LIB to the installation folder +libmexclass/+proxy.
+set(CUSTOM_PROXY_RUNTIME_LIBRARIES ${ARROW_SHARED_LIB})
+set(CUSTOM_PROXY_FACTORY_HEADER_FILENAME "factory.h")
+set(CUSTOM_PROXY_FACTORY_CLASS_NAME "arrow::matlab::proxy::Factory")

Review Comment:
   Thank you @kou and @assignUser for all of your feedback! It's all very helpful.
   
   ---
   
   @assignUser - thanks for suggesting the use of `FetchContent`. I can see how using it would make `libmexclass` more "tightly integrated" with client CMake code by directly exposing targets that can be manipulated by the client.
   
   However, I want to make sure we are all on the same page before starting any refactoring efforts on the `libmexclass` build system. To start, I thought it might be helpful to clarify the current "architecture" of the `libmexclass` build system.
   
   The way `libmexclass` currently works is by building 3 separate binaries (one of which is named `libmexclass.so`).
   
   The 3 binaries and their link dependencies look like the following:
   
   1. `libmexclass.so` -- depends on --> MATLAB shared libraries
   2. `libmexclassclient.so` -- depends on --> `libmexclass.so` AND custom client link libraries (e.g. `libarrow.so`)
   3. `gateway.mexa64` -- depends on --> `libmexclass.so` AND `libmexclassclient.so`
   
   `libmexclassclient.so` is a shared library that is built directly from the specified client code (e.g. `float64_array.cc`). The "semantic" use of this library is to provide a concrete subclass of `libmexclass::proxy::Factory` to the MEX gateway, so that `Proxy` instances can be created appropriately. To be clear - `libmexclassclient.so` is the only binary that is actually built directly from code that the client CMake project provides. The rest of the binaries are built from code in the `libmexclass` project.
   
   One thing worth explicitly noting, is that `target_compile_definitions` is used to "dynamically" build a MEX binary from [predefined "template" code](https://github.com/mathworks/libmexclass/blob/484ade97176c611cbb7056b941df99f1f3a6a0dc/libmexclass/cpp/source/libmexclass/mex/mex_gateway_function.cpp#L62). Essentially, we use these compiler definitions to inform the compiler what the appropriate [header file](https://github.com/mathworks/libmexclass/blob/484ade97176c611cbb7056b941df99f1f3a6a0dc/libmexclass/cpp/source/libmexclass/mex/mex_gateway_function.cpp#L7) is for the client's subclass of `libmexclass::proxy::Factory` and what the [name of their subclass is](https://github.com/mathworks/libmexclass/blob/484ade97176c611cbb7056b941df99f1f3a6a0dc/libmexclass/cpp/source/libmexclass/mex/mex_gateway_function.cpp#L62).
   
   Hopefully, this helps somewhat to clear up what is going on under the hood when building a CMake project that is using `libmexclass`.
   
   ---
   
   In terms of moving forward, a few questions:
   
   1. Assuming we refactor the `libmexclass` build system so that its targets can be used via `FetchContent`, are we OK if the client code still has to call a few macros to set up the appropriate target and link dependencies between the client shared library (i.e. their `Factory` and `Proxy` code) and the MEX `gateway` target? The original design of the build system for `libmexclass` was designed to optimize for simplicity of use (which happens to be at expense of some control in this case). Our hope is that we could potentially preserve some of this simplicity by providing a few helper macros.
   2. Are we OK with continuing to build the MEX `gateway` from a [predefined "template" source file](https://github.com/mathworks/libmexclass/blob/484ade97176c611cbb7056b941df99f1f3a6a0dc/libmexclass/cpp/source/libmexclass/mex/mex_gateway_function.cpp#L62) and using "code injection" (i.e. `target_compile_definitions`) to inform the compiler what header file and class name to use for the client subclass of `libmexclass::proxy::Factory`? If we aren't OK with this, then we would need the client to author the MEX gateway source code entirely on their own, which would be more error prone and would probably require us to modify the `libmexclass` APIs.
   3. It's possible I am misunderstanding, but it seems like there are two different approaches being suggested here. One is to use `FetchContent` to essentially "embed" the build system of `libmexclass` into the client build system (i.e. make the targets and macros available). The other approach seems to be creating a CMake package and using it in conjunction with `find_package`. Are there any strong opinions on which approach is preferable? At first glance, it seems like using `FetchContent` might be a bit simpler to setup.
   
   ---
   
   If anything is unclear, please don't hesitate to let me know.
   
   Thank you very much for thinking through this with us!



-- 
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