kevingurney commented on code in PR #34563: URL: https://github.com/apache/arrow/pull/34563#discussion_r1140561514
########## 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: > libmexclass.so can be shared from multiple MATALB modules. (Module A and module B can use the same libmexclass.so.) Correct! > libmexclassclient.so can't be shared from multiple MATAB modules. (Each module must build its libmexclassclient.so.) Correct! > It seems that we can just use C++ template feature for it instead of macros when we build libmexclient.so in our CMake. In the current architecture using C++ templates to instantiate the correct client subclass of `libmexclass:proxy::Factory` wouldn't be possible. This is because the client is *not* the one who builds / defines the MEX gateway function. Instead, the [`mex_gateway_function.cpp` "template code"](https://github.com/mathworks/libmexclass/blob/main/libmexclass/cpp/source/libmexclass/mex/mex_gateway_function.cpp) is "injected" with the name of the client's `Factory` subclass and header file name. In short, the client does *not* write the MEX gateway function code - they just "customize it" using `target_compile_definitions`. Don't hesitate to let me know if this is still unclear in any way. > BTW, who does instantiate MexFunction defined in mex_gateway_function.cpp? Same as my previous comment above. The `libmexclass` build system (i.e. *not* the client) is responsible for taking the `mex_gateway_function.cpp` "template code" and "injecting" the client's `Factory` subclass and header file name during the build process. This results in a single binary file `gateway.mexa64` that can be called from MATLAB as `gateway` like any other MATLAB function (note - this is [how MEX functions work, in general](https://www.mathworks.com/help/matlab/matlab_external/c-mex-functions.html)). > "The client code" is [mex_gateway_function.cpp](https://github.com/mathworks/libmexclass/blob/484ade97176c611cbb7056b941df99f1f3a6a0dc/libmexclass/cpp/source/libmexclass/mex/mex_gateway_function.cpp), right? Or our CMake code? Sorry for the confusion. I think I used "client code" to mean two different things in my previous comment. To clarify - there is the "client C++ `Proxy` and `Factory` code" - which is built into `libmexclassclient.so`. Then, there is also the "client CMake code" - which I intended to mean the Arrow-MATLAB CMakeLists.txt. I hope this clears things up. > "a few macros" are C/C++'s (C preprocessor's) macros, right? Or CMake's macro? CMake macros. > "the appropriate target and link dependencies" is for CMake, right? Correct. This is for CMake targets. > Calling add_library(), target_link_libraries() and so on for libmexclient.so and gateway.mexa64 are OK for me. Because it's one of natural CMake usages. Excellent! This sounds good. > "a few helper macros" are CMake's macros, right? Yep, these would be CMake helper macros which would hopefully make it easy to connect `libmexclass` targets with client-defined targets/options. > Can we use C++ template feature instead of the approach? I may provide a code example once I understand who instantiates MexFunction. See my previous answer to this. With the current approach, the client does *not* write the source code for the MEX gateway function. Instead, they simply use the predefined "template code". If we wanted to use C++ templates for this, then the client would have to be responsible for writing the MEX gateway code from scratch, as well as setting up the appropriate CMake options on the MEX target. This seems like it may be a more error prone approach and would lead to each client rewriting the exact same MEX gateway code (aside from the one line where they use their subclass of `Factory`). > I don't have a strong opinion. I think that the latter approach (installing libmexclass and use it as a CMake package) is better because multiple MATLAB modules can share one libmexclass installation. But I'm OK with the former approach (using FetchContent). Thanks for providing your input on this. Your reasoning for the package approach being more preferable makes a lot of sense. On the other hand, it seems like the `FetchContent` approach might be a bit simpler to set up. I think, if it is OK, we could start with the `FetchContent` approach to hopefully unblock development more quickly and then consider adding package support as a future improvement to `libmexclass`. It seems like the advantages of the package approach might also be more pertinent in the future when we may have multiple clients of `libmexclass` loaded into MATLAB at the same time. --- I hope I answered all of your questions clearly. If anything else is still not making sense, please let me know! If the approach of using a combination of `FetchContent` and the "template MEX function code" (i.e. [`mex_gateway_function.cpp`](https://github.com/mathworks/libmexclass/blob/main/libmexclass/cpp/source/libmexclass/mex/mex_gateway_function.cpp)) with "code injection" via `target_compile_definitions` seems like a reasonable approach, then we would be happy to start working on implementing that in `libmexclass`. Thanks again! -- 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]
