Skip to content

Commit

Permalink
apacheGH-37532: [CI][Docs][MATLAB] Remove GoogleTest support from t…
Browse files Browse the repository at this point in the history 
…he CMake build system for the MATLAB interface (apache#37784)

### Rationale for this change

This pull request removes `GoogleTest` support from the CMake build system for the MATLAB interface.

1. `GoogleTest` support adds a lot of additional complexity to the CMake build system for the MATLAB interface, and we currently don't have any standalone C++ tests for the MATLAB interface code.
2. In order to use `GoogleTest` in the MATLAB CI workflows, we are currently relying on building the tests for the Arrow C++ libraries in order to "re-use" the `GoogleTest binaries. This adds additional overhead to the MATLAB CI workflows.
3. If we want to test some internal C++ code for the MATLAB interface in the future, we can instead use a MEX function to call the code from a MATLAB test as suggested by @ kou in apache#37532 (comment).
4. There is [precedent for testing internal C++ code without GoogleTest for the Python bindings](apache#14117).
5. On a somewhat related note - removing `GoogleTest` support will help unblock apache#37773 as discussed in apache#37773 (comment).

### What changes are included in this PR?

1. Removed the `MATLAB_BUILD_TESTS` flag from the CMake build system for the MATLAB interface since there are no longer any C++ tests for the MATLAB interface to build.
2. Updated the `matlab_build.sh` CI workflow script to avoid building the tests for the Arrow C++ libraries and to no longer call `ctest`.
3. Updated the `README.md` for the MATLAB interface to no longer mention building or running C++ tests.
4. Updated the design document for the MATLAB Interface to no longer mention `GoogleTest` since we may end up testing internal C++ code using MEX function calls from MATLAB instead.
5. Removed placeholder C++ test (i.e. `placeholder_test.cc`).

### Are these changes tested?

Yes.

The MATLAB CI workflow is passing on all platforms.

### Are there any user-facing changes?

Yes.

There are no longer any C++ tests for the MATLAB interface. The `MATLAB_BUILD_TESTS` flag has been removed from the CMake build system to reflect this change. If a user supplies a value for `MATLAB_BUILD_TESTS` when building the MATLAB interface, the flag will be ignored by CMake.

### Future Directions

1. Add more developer-focused documentation on how to test C++ code via MEX function calls from MATLAB.

### Notes

1. In the future, we can consider testing internal C++ code using MEX function calls from MATLAB tests as suggested by @ kou in apache#37532 (comment). Currently, we don't have any C++ tests that need to be adapted to use this approach.
2. Thank you @ sgilmore10 for your help with this pull request!
* Closes: apache#37532

Lead-authored-by: Kevin Gurney <[email protected]>
Co-authored-by: Sarah Gilmore <[email protected]>
Signed-off-by: Kevin Gurney <[email protected]>
  • Loading branch information
@kevingurney @sgilmore10
kevingurney and sgilmore10 authored Sep 19, 2023
1 parent fa43106 commit cda1e8f
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 277 deletions.
2 changes: 0 additions & 2 deletions ci/scripts/matlab_build.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ cmake \
-S ${source_dir} \
-B ${build_dir} \
-G Ninja \
-D MATLAB_BUILD_TESTS=ON \
-D CMAKE_INSTALL_PREFIX=${install_dir} \
-D MATLAB_ADD_INSTALL_DIR_TO_SEARCH_PATH=OFF
cmake --build ${build_dir} --config Release --target install
ctest --test-dir ${build_dir}
266 changes: 43 additions & 223 deletions matlab/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@

cmake_minimum_required(VERSION 3.20)

# Build the Arrow C++ libraries.
# Build the Arrow C++ libraries using ExternalProject_Add.
function(build_arrow)
set(options BUILD_GTEST)
set(options)
set(one_value_args)
set(multi_value_args)

Expand All @@ -37,70 +37,50 @@ function(build_arrow)
set(ARROW_CMAKE_ARGS "-DCMAKE_INSTALL_PREFIX=${ARROW_PREFIX}"
"-DCMAKE_INSTALL_LIBDIR=lib" "-DARROW_BUILD_STATIC=OFF")

if(Arrow_FOUND
AND NOT GTest_FOUND
AND ARG_BUILD_GTEST)
# If find_package has already found a valid Arrow installation, then
# we don't want to link against the Arrow libraries that will be built
# from source.
#
# However, we still need to create a library target to trigger building
# of the arrow_ep target, which will ultimately build the bundled
# GoogleTest binaries.
add_library(arrow_shared_for_gtest SHARED IMPORTED)
set(ARROW_LIBRARY_TARGET arrow_shared_for_gtest)
add_library(arrow_shared SHARED IMPORTED)
set(ARROW_LIBRARY_TARGET arrow_shared)

# Set the runtime shared library (.dll, .so, or .dylib)
if(WIN32)
# The shared library (i.e. .dll) is located in the "bin" directory.
set(ARROW_SHARED_LIBRARY_DIR "${ARROW_PREFIX}/bin")
else()
add_library(arrow_shared SHARED IMPORTED)
set(ARROW_LIBRARY_TARGET arrow_shared)

# Set the runtime shared library (.dll, .so, or .dylib)
if(WIN32)
# The shared library (i.e. .dll) is located in the "bin" directory.
set(ARROW_SHARED_LIBRARY_DIR "${ARROW_PREFIX}/bin")
else()
# The shared library (i.e. .so or .dylib) is located in the "lib" directory.
set(ARROW_SHARED_LIBRARY_DIR "${ARROW_PREFIX}/lib")
endif()

set(ARROW_SHARED_LIB_FILENAME
"${CMAKE_SHARED_LIBRARY_PREFIX}arrow${CMAKE_SHARED_LIBRARY_SUFFIX}")
set(ARROW_SHARED_LIB "${ARROW_SHARED_LIBRARY_DIR}/${ARROW_SHARED_LIB_FILENAME}")

set_target_properties(arrow_shared PROPERTIES IMPORTED_LOCATION ${ARROW_SHARED_LIB})

# Set the link-time import library (.lib)
if(WIN32)
# The import library (i.e. .lib) is located in the "lib" directory.
set(ARROW_IMPORT_LIB_FILENAME
"${CMAKE_IMPORT_LIBRARY_PREFIX}arrow${CMAKE_IMPORT_LIBRARY_SUFFIX}")
set(ARROW_IMPORT_LIB "${ARROW_PREFIX}/lib/${ARROW_IMPORT_LIB_FILENAME}")

set_target_properties(arrow_shared PROPERTIES IMPORTED_IMPLIB ${ARROW_IMPORT_LIB})
endif()

# Set the include directories
set(ARROW_INCLUDE_DIR "${ARROW_PREFIX}/include")
file(MAKE_DIRECTORY "${ARROW_INCLUDE_DIR}")
set_target_properties(arrow_shared PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
${ARROW_INCLUDE_DIR})

# Set the build byproducts for the ExternalProject build
# The appropriate libraries need to be guaranteed to be available when linking the test
# executables.
if(WIN32)
set(ARROW_BUILD_BYPRODUCTS "${ARROW_IMPORT_LIB}")
else()
set(ARROW_BUILD_BYPRODUCTS "${ARROW_SHARED_LIB}")
endif()
# The shared library (i.e. .so or .dylib) is located in the "lib" directory.
set(ARROW_SHARED_LIBRARY_DIR "${ARROW_PREFIX}/lib")
endif()

# Building the Arrow C++ libraries and bundled GoogleTest binaries requires ExternalProject.
include(ExternalProject)
set(ARROW_SHARED_LIB_FILENAME
"${CMAKE_SHARED_LIBRARY_PREFIX}arrow${CMAKE_SHARED_LIBRARY_SUFFIX}")
set(ARROW_SHARED_LIB "${ARROW_SHARED_LIBRARY_DIR}/${ARROW_SHARED_LIB_FILENAME}")

set_target_properties(arrow_shared PROPERTIES IMPORTED_LOCATION ${ARROW_SHARED_LIB})

# Set the link-time import library (.lib)
if(WIN32)
# The import library (i.e. .lib) is located in the "lib" directory.
set(ARROW_IMPORT_LIB_FILENAME
"${CMAKE_IMPORT_LIBRARY_PREFIX}arrow${CMAKE_IMPORT_LIBRARY_SUFFIX}")
set(ARROW_IMPORT_LIB "${ARROW_PREFIX}/lib/${ARROW_IMPORT_LIB_FILENAME}")

if(ARG_BUILD_GTEST)
enable_gtest()
set_target_properties(arrow_shared PROPERTIES IMPORTED_IMPLIB ${ARROW_IMPORT_LIB})
endif()

# Set the include directories
set(ARROW_INCLUDE_DIR "${ARROW_PREFIX}/include")
file(MAKE_DIRECTORY "${ARROW_INCLUDE_DIR}")
set_target_properties(arrow_shared PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
${ARROW_INCLUDE_DIR})

# Set the build byproducts for the ExternalProject build
if(WIN32)
set(ARROW_BUILD_BYPRODUCTS "${ARROW_IMPORT_LIB}")
else()
set(ARROW_BUILD_BYPRODUCTS "${ARROW_SHARED_LIB}")
endif()

# Building the Arrow C++ libraries requires ExternalProject.
include(ExternalProject)

externalproject_add(arrow_ep
SOURCE_DIR "${CMAKE_SOURCE_DIR}/../cpp"
BINARY_DIR "${ARROW_BINARY_DIR}"
Expand All @@ -109,69 +89,8 @@ function(build_arrow)

add_dependencies(${ARROW_LIBRARY_TARGET} arrow_ep)

if(ARG_BUILD_GTEST)
build_gtest()
endif()
endfunction()

macro(enable_gtest)
set(ARROW_GTEST_INCLUDE_DIR "${ARROW_PREFIX}/include/arrow-gtest")

set(ARROW_GTEST_IMPORT_LIB_DIR "${ARROW_PREFIX}/lib")
if(WIN32)
set(ARROW_GTEST_SHARED_LIB_DIR "${ARROW_PREFIX}/bin")
else()
set(ARROW_GTEST_SHARED_LIB_DIR "${ARROW_PREFIX}/lib")
endif()
set(ARROW_GTEST_IMPORT_LIB
"${ARROW_GTEST_IMPORT_LIB_DIR}/${CMAKE_IMPORT_LIBRARY_PREFIX}arrow_gtest${CMAKE_IMPORT_LIBRARY_SUFFIX}"
)
set(ARROW_GTEST_MAIN_IMPORT_LIB
"${ARROW_GTEST_IMPORT_LIB_DIR}/${CMAKE_IMPORT_LIBRARY_PREFIX}arrow_gtest_main${CMAKE_IMPORT_LIBRARY_SUFFIX}"
)
set(ARROW_GTEST_SHARED_LIB
"${ARROW_GTEST_SHARED_LIB_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}arrow_gtest${CMAKE_SHARED_LIBRARY_SUFFIX}"
)
set(ARROW_GTEST_MAIN_SHARED_LIB
"${ARROW_GTEST_SHARED_LIB_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}arrow_gtest_main${CMAKE_SHARED_LIBRARY_SUFFIX}"
)

list(APPEND ARROW_CMAKE_ARGS "-DARROW_BUILD_TESTS=ON")

# The appropriate libraries need to be guaranteed to be available when linking the test
# executables.
if(WIN32)
# On Windows, add the gtest link libraries as BUILD_BYPRODUCTS for arrow_ep.
list(APPEND ARROW_BUILD_BYPRODUCTS "${ARROW_GTEST_IMPORT_LIB}"
"${ARROW_GTEST_MAIN_IMPORT_LIB}")
else()
# On Linux and macOS, add the gtest shared libraries as BUILD_BYPRODUCTS for arrow_ep.
list(APPEND ARROW_BUILD_BYPRODUCTS "${ARROW_GTEST_SHARED_LIB}"
"${ARROW_GTEST_MAIN_SHARED_LIB}")
endif()
endmacro()

# Build the GoogleTest binaries that are bundled with the Arrow C++ libraries.
macro(build_gtest)
file(MAKE_DIRECTORY "${ARROW_GTEST_INCLUDE_DIR}")

# Create target GTest::gtest
add_library(GTest::gtest SHARED IMPORTED)
set_target_properties(GTest::gtest
PROPERTIES IMPORTED_IMPLIB ${ARROW_GTEST_IMPORT_LIB}
IMPORTED_LOCATION ${ARROW_GTEST_SHARED_LIB}
INTERFACE_INCLUDE_DIRECTORIES
${ARROW_GTEST_INCLUDE_DIR})
add_dependencies(GTest::gtest arrow_ep)

# Create target GTest::gtest_main
add_library(GTest::gtest_main SHARED IMPORTED)
set_target_properties(GTest::gtest_main
PROPERTIES IMPORTED_IMPLIB ${ARROW_GTEST_MAIN_IMPORT_LIB}
IMPORTED_LOCATION ${ARROW_GTEST_MAIN_SHARED_LIB})
add_dependencies(GTest::gtest_main arrow_ep)
endmacro()

set(CMAKE_CXX_STANDARD 17)

set(MLARROW_VERSION "14.0.0-SNAPSHOT")
Expand All @@ -185,8 +104,6 @@ if(WIN32)
set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreadedDLL")
endif()

option(MATLAB_BUILD_TESTS "Build the C++ tests for the MATLAB interface" OFF)

# Add tools/cmake directory to the CMAKE_MODULE_PATH.
list(PREPEND CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR}/tools/cmake)

Expand All @@ -208,56 +125,9 @@ else()
set(MATLAB_BUILD_OUTPUT_DIR "${CMAKE_BINARY_DIR}")
endif()

# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
if(MATLAB_BUILD_TESTS)
# find_package(GTest) supports custom GTEST_ROOT as well as package managers.
find_package(GTest)

if(NOT GTest_FOUND)
# find_package(Arrow) supports custom ARROW_HOME as well as package
# managers.
find_package(Arrow QUIET)
# Trigger an automatic build of the Arrow C++ libraries and bundled
# GoogleTest binaries. If a valid Arrow installation was not already
# found by find_package, then build_arrow will use the Arrow
# C++ libraries that are built from source.
build_arrow(BUILD_GTEST)
else()
# On Windows, IMPORTED_LOCATION needs to be set to indicate where the shared
# libraries live when GTest is found.
if(WIN32)
set(GTEST_SHARED_LIB_DIR "${GTEST_ROOT}/bin")
set(GTEST_SHARED_LIBRARY_FILENAME
"${CMAKE_SHARED_LIBRARY_PREFIX}gtest${CMAKE_SHARED_LIBRARY_SUFFIX}")
set(GTEST_SHARED_LIBRARY_LIB
"${GTEST_SHARED_LIB_DIR}/${GTEST_SHARED_LIBRARY_FILENAME}")

set(GTEST_MAIN_SHARED_LIB_DIR "${GTEST_ROOT}/bin")
set(GTEST_MAIN_SHARED_LIBRARY_FILENAME
"${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${CMAKE_SHARED_LIBRARY_SUFFIX}")
set(GTEST_MAIN_SHARED_LIBRARY_LIB
"${GTEST_MAIN_SHARED_LIB_DIR}/${GTEST_MAIN_SHARED_LIBRARY_FILENAME}")

set_target_properties(GTest::gtest PROPERTIES IMPORTED_LOCATION
"${GTEST_SHARED_LIBRARY_LIB}")

set_target_properties(GTest::gtest_main
PROPERTIES IMPORTED_LOCATION
"${GTEST_MAIN_SHARED_LIBRARY_LIB}")
endif()

find_package(Arrow QUIET)
if(NOT Arrow_FOUND)
# Trigger an automatic build of the Arrow C++ libraries.
build_arrow()
endif()
endif()

else()
find_package(Arrow QUIET)
if(NOT Arrow_FOUND)
build_arrow()
endif()
find_package(Arrow QUIET)
if(NOT Arrow_FOUND)
build_arrow()
endif()

# MATLAB is Required
Expand Down Expand Up @@ -311,56 +181,6 @@ else()
message(STATUS "ARROW_INCLUDE_DIR: ${ARROW_INCLUDE_DIR}")
endif()

# ##############################################################################
# C++ Tests
# ##############################################################################
# Only build the C++ tests if MATLAB_BUILD_TESTS=ON.
if(MATLAB_BUILD_TESTS)
enable_testing()

# Define a test executable target. TODO: Remove the placeholder test. This is
# just for testing GoogleTest integration.
add_executable(placeholder_test ${CMAKE_SOURCE_DIR}/src/placeholder_test.cc)

# Declare a dependency on the GTest::gtest and GTest::gtest_main IMPORTED
# targets.
target_link_libraries(placeholder_test GTest::gtest GTest::gtest_main)

# Ensure using GTest:gtest and GTest::gtest_main on macOS without
# specifying DYLD_LIBRARY_DIR.
set_target_properties(placeholder_test
PROPERTIES BUILD_RPATH
"$<TARGET_FILE_DIR:GTest::gtest>;$<TARGET_FILE_DIR:GTest::gtest_main>"
)

# Add test targets for C++ tests.
add_test(PlaceholderTestTarget placeholder_test)

# On Windows:
# Add the directory of gtest.dll and gtest_main.dll to the %PATH% for running
# all tests.
# Add the directory of libmx.dll, libmex.dll, and libarrow.dll to the %PATH% for running
# CheckNumArgsTestTarget.
# Note: When appending to the path using set_test_properties' ENVIRONMENT property,
# make sure that we escape ';' to prevent CMake from interpreting the input as
# a list of strings.
if(WIN32)
get_target_property(GTEST_SHARED_LIB GTest::gtest IMPORTED_LOCATION)
get_filename_component(GTEST_SHARED_LIB_DIR ${GTEST_SHARED_LIB} DIRECTORY)

get_target_property(GTEST_MAIN_SHARED_LIB GTest::gtest_main IMPORTED_LOCATION)
get_filename_component(GTEST_MAIN_SHARED_LIB_DIR ${GTEST_MAIN_SHARED_LIB} DIRECTORY)

set_tests_properties(PlaceholderTestTarget
PROPERTIES ENVIRONMENT
"PATH=${GTEST_SHARED_LIB_DIR}\;${GTEST_MAIN_SHARED_LIB_DIR}\;$ENV{PATH}"
)

get_target_property(ARROW_SHARED_LIB arrow_shared IMPORTED_LOCATION)
get_filename_component(ARROW_SHARED_LIB_DIR ${ARROW_SHARED_LIB} DIRECTORY)
endif()
endif()

# ##############################################################################
# Install
# ##############################################################################
Expand Down
19 changes: 0 additions & 19 deletions matlab/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,31 +100,12 @@ As part of the install step, the installation directory is added to the [MATLAB

## Test

There are two kinds of tests for the MATLAB Interface: MATLAB and C++.

### MATLAB

To run the MATLAB tests, start MATLAB in the `arrow/matlab` directory and call the [`runtests`](https://mathworks.com/help/matlab/ref/runtests.html) command on the `test` directory with `IncludeSubFolders=true`:

``` matlab
>> runtests("test", IncludeSubFolders=true);
```

### C++

To enable the C++ tests, set the `MATLAB_BUILD_TESTS` flag to `ON` at build time:

```console
$ cmake -S . -B build -D MATLAB_BUILD_TESTS=ON
$ cmake --build build --config Release
```

After building with the `MATLAB_BUILD_TESTS` flag enabled, the C++ tests can be run using [CTest](https://cmake.org/cmake/help/latest/manual/ctest.1.html):

```console
$ ctest --test-dir build
```

## Usage

Included below are some example code snippets that illustrate how to use the MATLAB interface.
Expand Down
12 changes: 6 additions & 6 deletions matlab/doc/matlab_interface_for_apache_arrow_design.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,14 +257,13 @@ For large tables used in a multi-process "data processing pipeline", a user coul

## Testing
To ensure code quality, we would like to include the following testing infrastructure, at a minimum:
1. C++ APIs
- GoogleTest C++ Unit Tests
- Integration with CI workflows
2. MATLAB APIs
- [MATLAB Class-Based Unit Tests]
- Integration with CI workflows

1. [MATLAB Class-Based Unit Tests]
2. [MATLAB CI Workflows]
3. [Integration Testing]

**Note**: To test internal C++ code, we can use a [MEX function] to call the C++ code from a MATLAB Class-Based Unit Test.

## Documentation
To ensure usability, discoverability, and accessibility, we would like to include high quality documentation for the MATLAB Interface for Apache Arrow.

Expand Down Expand Up @@ -318,3 +317,4 @@ The table below provides a high-level roadmap for the development of specific ca
[`apache-arrow` package via the `npm` package manager]: https://www.npmjs.com/package/apache-arrow
[Rust user]: https://github.com/apache/arrow-rs
[`arrow` crate via the `cargo` package manager]: https://crates.io/crates/arrow
[MATLAB CI Workflows]: https://github.com/apache/arrow/actions/workflows/matlab.yml
Loading

0 comments on commit cda1e8f

Please sign in to comment.