You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/06/28 22:15:42 UTC
[GitHub] [arrow] kevingurney opened a new pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
kevingurney opened a new pull request #10614:
URL: https://github.com/apache/arrow/pull/10614
**Overview**
This Pull Request integrates GoogleTest with the CMake build system for the MATLAB interface.
1. A user can supply a custom [`GTEST_ROOT`](https://cmake.org/cmake/help/latest/module/FindGTest.html) value to CMake in order to make use of pre-built GoogleTest binaries.
2. A user can provide no `GTEST_ROOT` value, and CMake will automatically fetch and build GoogleTest from source using the [`build_dependency`](https://github.com/apache/arrow/blob/57ecc73e6153fea04e0ac0d13792ba0abb0dd779/cpp/cmake_modules/ThirdpartyToolchain.cmake#L137) macro declared in [`cpp/cmake_modules/ThirdpartyToolchain.cmake`](https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake).
**Implementation**
This implementation leverages the existing [`build_dependency`](https://github.com/apache/arrow/blob/57ecc73e6153fea04e0ac0d13792ba0abb0dd779/cpp/cmake_modules/ThirdpartyToolchain.cmake#L137) macro declared in [`cpp/cmake_modules/ThirdpartyToolchain.cmake`](https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake) to automatically fetch and build GoogleTest from source.
**Testing**
1. These changes were qualified against the following platform/compiler configurations:
1.1. Windows 10 with Visual Studio 2019
1.2. macOS Big Sur (11.2.3) with GNU Make 3.81
1.3. Debian 10 with GNU Make GNU 4.2.1
2. We ran the CMake build with a custom `GTEST_ROOT` value that points to GoogleTest binaries that were built via the CMake `googletest_ep` target from the Arrow C++ libraries. In other words, we built GoogleTest with `cmake -DARROW_BUILD_TESTS=ON ..; make googletest_ep`.
3. We ran the CMake build without specifying a `GTEST_ROOT` value and GoogleTest was automatically fetched and built.
4. We ran the compiled `matlab/src/placeholder_test.cc` GoogleTest test suite on all platforms using `ctest`.
5. We ran the compiled `matlab/src/placeholder_test.cc` GoogleTest test suite on:
5.1. Debian using `make test`
5.2. macOS using `make test`
5.3. Windows using `devenv.com mlarrow.sln /build Release /project RUN_TESTS`
**Future Directions**
1. As follow up work to this PR, a major priority is to integrate the MATLAB interface with Arrow CI ecosystem, so that both C++ and MATLAB tests will be automatically run with every pull request.
2. Pending acceptance of this pull request, the MATLAB interface will have C++ testing infrastructure in place. This will allow us to shift focus towards incremental delivery of C++ feature code, along with associated tests.
**Note**
1. Thank you to @lafiona for helping me with this pull request!
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r660656255
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,23 @@
cmake_minimum_required(VERSION 3.20)
+# This function uses the ThirdpartyToolchain CMake module
+# in cpp/cmake_modules to automatically download and build GoogleTest.
+function(download_and_build_google_test)
+ # The ExternalProject module is required for downloading and building external dependencies.
+ include(ExternalProject)
+ # The ThirdpartyToolchain module automatically builds xsimd by default,
+ # so we explicitly disable building of xsimd here.
+ set(ARROW_SIMD_LEVEL "NONE")
+ set(ARROW_RUNTIME_SIMD_LEVEL "NONE")
+ # The ThirdpartyToolchain module must know the location of the Arrow C++ libraries source code.
+ set(arrow_SOURCE_DIR ${CMAKE_SOURCE_DIR}/../cpp)
+ # The ThirdpartyToolchain module uses ExternalProject to download GoogleTest.
+ include(ThirdpartyToolchain)
Review comment:
Thanks for reviewing these changes, @kou!
Could you elaborate on why `ThirdpartyToolchain` isn't expected to be `include`d in other CMake files?
We intentionally wanted to reuse the robust infrastructure that the community took time to develop for finding 3rd party libraries in a flexible way, rather than reinventing the wheel here.
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r661619865
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
@kou - we tried invoking an automatic build of the Arrow C++ libraries (and the bundled GoogleTest binaries) from `matlab/CMakeLists.txt` using:
```cmake
ExternalProject_Add(ArrowCpp SOURCE_DIR "${CMAKE_SOURCE_DIR}/../cpp" CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=. -DARROW_BUILD_TESTS=ON)
```
This did allow us to run `make ArrowCpp` to build the C++ libraries, but, this doesn't seem to be exporting targets for `arrow_shared` or `GTest::GTest` that we can link MEX files against. Admittedly, I am not very familiar with `ExternalProject_Add`, so I am guessing things aren't that simple.
Is this the kind of approach you were thinking of? Or, were you thinking that users would be expected to **manually** build the Arrow C++ libraries first, in order to get access to the bundled GoogleTest binares (i.e. similar to https://github.com/apache/arrow/blob/master/docs/source/developers/python.rst#build-and-test)?
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r661619865
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
We tried invoking an automatic build of the Arrow C++ libraries from `matlab/CMakeLists.txt` using:
```cmake
ExternalProject_Add(ArrowCpp SOURCE_DIR "${CMAKE_SOURCE_DIR}/../cpp" CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=. -DARROW_BUILD_TESTS=ON)
```
But, unfortunately, this doesn't seem to be exporting targets for `arrow_shared` or `GTest::GTest`. Admittedly, I am not particularly familiar with `ExternalProject_Add`.
Is this the approach you were suggesting? Or, were you thinking that users would just manually build the Arrow C++ libraries first, in order to get access to the bundled GoogleTest binares (i.e. similar to https://github.com/apache/arrow/blob/master/docs/source/developers/python.rst#build-and-test)?
--
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
[GitHub] [arrow] kevingurney commented on pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#issuecomment-879904821
Yes - sure thing. I'll update the description now.
Thanks!
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r666492483
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
Review comment:
Thanks - this is a really good idea!
We factored out the GoogleTest-specific logic into a macro called `build_gtest` for code cleanliness and added a `BUILD_GTEST` argument to `build_arrow` in order to enable us to remove the `build_arrow_and_gtest` function. `build_arrow` now just calls `build_gtest` when `BUILD_GTEST` is supplied.
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r660672641
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
Installing GoogleTest via a package manager can be convenient on certain platforms (e.g. modern Linux distributions, macOS, etc.), but package managers tend to be hit or miss in terms of the specific version of third party libraries that are available. For example, if a user is developing on an older version of Debian, installing GoogleTest via `apt` might not give them the flexibility of installing a more recent version.
The `GTEST_ROOT` approach is particularly helpful for letting users decide exactly how they want to integrate with GoogleTest (e.g. if they need to lock down all of their 3rd party dependencies and only refer to local source trees, without ever calling out to the Internet). On the other hand, it can be tedious and error prone for new developers to properly configure/build GoogleTest from source, so it's nice that the `build_dependency` macro will automatically fetch and build GoogleTest for you, without the user having to worry if they just want to get started with local development quickly.
Your point about supporting package managers is a good one, though. Our current `CMakeLists.txt` won't automatically find GoogleTest binaries that have been installed via a package manager (although, a user could still explicitly specify `GTEST_ROOT` here). A nice improvement would be to support automatic discovery of GoogleTest binaries installed via a package manager. Then, only if GoogleTest isn't found in the standard system directories (`/usr/include`, `/usr/lib`, etc.), we would fall back to downloading and building GoogleTest from source.
--
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
[GitHub] [arrow] kevingurney commented on pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#issuecomment-877243885
Yes - we are actively working on adding support for Windows and macOS, now. We'll add the changes to this branch.
Thanks!
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r666204614
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
+ # Should support custom GTEST_ROOT as well as package managers.
+ find_package(GTest)
+ if(NOT GTest_FOUND)
+ # Trigger an automatic build of the Arrow C++ libraries and bundled GoogleTest binaries.
+ build_arrow_and_gtest()
+ else()
+ # Should support custom ARROW_HOME as well as package managers.
+ find_package(Arrow)
+ if(NOT Arrow_FOUND)
+ # Trigger an automatic build of the Arrow C++ libraries.
+ build_arrow()
+ endif()
+ endif()
+else()
+ # Should support custom ARROW_HOME as well as package managers.
Review comment:
See my comment above. The comment wording here is misleading. I'll reword this to be less ambigious.
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
+ # Should support custom GTEST_ROOT as well as package managers.
+ find_package(GTest)
+ if(NOT GTest_FOUND)
+ # Trigger an automatic build of the Arrow C++ libraries and bundled GoogleTest binaries.
+ build_arrow_and_gtest()
+ else()
+ # Should support custom ARROW_HOME as well as package managers.
+ find_package(Arrow)
+ if(NOT Arrow_FOUND)
+ # Trigger an automatic build of the Arrow C++ libraries.
+ build_arrow()
+ endif()
+ endif()
+else()
+ # Should support custom ARROW_HOME as well as package managers.
Review comment:
See my comment above. The comment wording here is misleading. I'll reword this to be less ambiguous.
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r666203962
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
+ # Should support custom GTEST_ROOT as well as package managers.
Review comment:
My apologies - I think the wording of my comment on line 115 may have lead to some confusion. What I meant by this comment was "`find_package(GTest) should already automatically support GTEST_ROOT and package managers`, not that we "should add more code to enable this".
I'll reword this comment to be less ambiguous.
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r666208411
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
Review comment:
Good point! Thanks for the suggestion. I'll make this chance.
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
Review comment:
Good point! Thanks for the suggestion. I'll make this change.
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r660973945
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
Setting `GTEST_ROOT` to `${ARROW_BUILD_DIR}/googletest_ep-prefix` inside the `CMakeLists.txt` for MATLAB seems like a reasonable approach.
However, it seems like it would be error prone to expect users to manually build the Arrow C++ libraries first due to the variation in folder names they might use for their `${ARROW_BUILD_DIR}`, as well as the different flags they might pass to the build (e.g. they might forget to include `-DARROW_BUILD_TESTS=on`).
If we could automatically trigger a build of the Arrow C++ libraries with `ARROW_BUILD_TESTS=ON` (in order to get GoogleTest to be built in `${ARROW_BUILD_DIR}/googletest_ep-prefix`) from the MATLAB `CMakeLists.txt` file, that would be great. But, I'm not sure what the best approach is to accomplish this.
If you could share any additional thoughts on how you envision this approach might work, that would be great.
--
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
[GitHub] [arrow] kevingurney commented on pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#issuecomment-879449900
@kou we just pushed updates to `matlab/CMakeLists.txt` which enable building of the Arrow C++ libraries and bundled GoogleTest binaries automatically from source on macOS and Windows.
We are now able to build and run the MATLAB C++ tests consistently, using the same set of commands, on Windows, Mac, and Linux:
```bash
$ cmake -S . -B build -D MATLAB_BUILD_TESTS=ON
$ cmake --build build --config Release
$ ctest --test-dir build
```
I think we are good to go now.
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r660976175
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,23 @@
cmake_minimum_required(VERSION 3.20)
+# This function uses the ThirdpartyToolchain CMake module
+# in cpp/cmake_modules to automatically download and build GoogleTest.
+function(download_and_build_google_test)
+ # The ExternalProject module is required for downloading and building external dependencies.
+ include(ExternalProject)
+ # The ThirdpartyToolchain module automatically builds xsimd by default,
+ # so we explicitly disable building of xsimd here.
+ set(ARROW_SIMD_LEVEL "NONE")
+ set(ARROW_RUNTIME_SIMD_LEVEL "NONE")
+ # The ThirdpartyToolchain module must know the location of the Arrow C++ libraries source code.
+ set(arrow_SOURCE_DIR ${CMAKE_SOURCE_DIR}/../cpp)
+ # The ThirdpartyToolchain module uses ExternalProject to download GoogleTest.
+ include(ThirdpartyToolchain)
Review comment:
See my comment below.
It sounds like depending on `ThirdpartyToolchain` may not be desirable without first refactoring it to make it more reusable. Given this fact, I'm fine with instead pursuing the approach of trying to automatically build the Arrow C++ libraries first and then depending on the bundled GoogleTest binaries that are built when `ARROW_BUILD_TESTS` is set to `on`.
--
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
[GitHub] [arrow] kou commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r660207256
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,23 @@
cmake_minimum_required(VERSION 3.20)
+# This function uses the ThirdpartyToolchain CMake module
+# in cpp/cmake_modules to automatically download and build GoogleTest.
+function(download_and_build_google_test)
+ # The ExternalProject module is required for downloading and building external dependencies.
+ include(ExternalProject)
+ # The ThirdpartyToolchain module automatically builds xsimd by default,
+ # so we explicitly disable building of xsimd here.
+ set(ARROW_SIMD_LEVEL "NONE")
+ set(ARROW_RUNTIME_SIMD_LEVEL "NONE")
+ # The ThirdpartyToolchain module must know the location of the Arrow C++ libraries source code.
+ set(arrow_SOURCE_DIR ${CMAKE_SOURCE_DIR}/../cpp)
+ # The ThirdpartyToolchain module uses ExternalProject to download GoogleTest.
+ include(ThirdpartyToolchain)
Review comment:
`ThirdpartyToolchain isn't expected to be used by `include`.
Could you use `find_package(GTest REQUIRED CONFIG)` instead?
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
Do we need this functionality for MATLAB module?
How about installing GoogleTest via package manager on system such as `apt`, `dnf`, `brew` and so on?
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#issuecomment-870082046
https://issues.apache.org/jira/browse/ARROW-13100
--
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
[GitHub] [arrow] kevingurney commented on pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#issuecomment-870095510
The most recent CI failures seem to be related to `cmake-format` checks failing on `arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake`:
```bash
ERROR __main__.py:618: Check failed: /arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake
```
It didn't seem like this file was supposed to be formatted using `cmake-format` since a large portion of the lines that were unmodified by this PR did not match the expected style when we originally tried running the CMake linting checks using `archery`.
Given this fact, I'm assuming its safe to ignore this CI failure, but let me know if I should also format `ThirdpartyToolchain.cmake` using `cmake-format`.
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r660976175
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,23 @@
cmake_minimum_required(VERSION 3.20)
+# This function uses the ThirdpartyToolchain CMake module
+# in cpp/cmake_modules to automatically download and build GoogleTest.
+function(download_and_build_google_test)
+ # The ExternalProject module is required for downloading and building external dependencies.
+ include(ExternalProject)
+ # The ThirdpartyToolchain module automatically builds xsimd by default,
+ # so we explicitly disable building of xsimd here.
+ set(ARROW_SIMD_LEVEL "NONE")
+ set(ARROW_RUNTIME_SIMD_LEVEL "NONE")
+ # The ThirdpartyToolchain module must know the location of the Arrow C++ libraries source code.
+ set(arrow_SOURCE_DIR ${CMAKE_SOURCE_DIR}/../cpp)
+ # The ThirdpartyToolchain module uses ExternalProject to download GoogleTest.
+ include(ThirdpartyToolchain)
Review comment:
See my comment below.
It sounds like depending on `ThirdpartyToolchain` may not be desirable without first refactoring it to make it more reusable. Given this fact, I'm fine with instead pursuing the approach of trying to automatically build the Arrow C++ libraries first, and then depending on the bundled GoogleTest binaries that are built when `ARROW_BUILD_TESTS` is set to `on`.
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r666260445
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
+ # Should support custom GTEST_ROOT as well as package managers.
+ find_package(GTest)
+ if(NOT GTest_FOUND)
+ # Trigger an automatic build of the Arrow C++ libraries and bundled GoogleTest binaries.
+ build_arrow_and_gtest()
+ else()
+ # Should support custom ARROW_HOME as well as package managers.
+ find_package(Arrow)
+ if(NOT Arrow_FOUND)
+ # Trigger an automatic build of the Arrow C++ libraries.
+ build_arrow()
+ endif()
+ endif()
+else()
+ # Should support custom ARROW_HOME as well as package managers.
Review comment:
Fixed.
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
Review comment:
Fixed.
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r666228358
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
+ include(ExternalProject)
+
+ set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+ set(ARROW_INCLUDE_DIR "${ARROW_PREFIX}/include")
+ set(ARROW_LIBRARY_DIR "${ARROW_PREFIX}/lib")
+ set(ARROW_SHARED_LIB "${ARROW_LIBRARY_DIR}/libarrow.so")
Review comment:
Thanks for this suggestion. This will help make the code more generic across platforms.
--
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
[GitHub] [arrow] kou commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r663594218
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
Could you merge the commit to this branch?
Then I can comment on the implementation in this pull request.
--
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
[GitHub] [arrow] kou commented on pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#issuecomment-880309249
Thanks!
I've merged this.
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r661619865
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
@kou - we tried invoking an automatic build of the Arrow C++ libraries (and the bundled GoogleTest binaries) from `matlab/CMakeLists.txt` using:
```cmake
ExternalProject_Add(ArrowCpp SOURCE_DIR "${CMAKE_SOURCE_DIR}/../cpp" CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=. -DARROW_BUILD_TESTS=ON)
```
This did allow us to run `make ArrowCpp` to build the C++ libraries, but, this doesn't seem to be exporting targets for `arrow_shared` or `GTest::GTest` that we can link MEX files against. Admittedly, I am not very familiar with `ExternalProject_Add`, so I am guessing `ExternalProject_Add` doesn't export targets in this manner.
Is this the kind of approach you were thinking of? Or, were you thinking that users would be expected to **manually** build the Arrow C++ libraries first, in order to get access to the bundled GoogleTest binares (i.e. similar to https://github.com/apache/arrow/blob/master/docs/source/developers/python.rst#build-and-test)?
--
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
[GitHub] [arrow] kevingurney commented on pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#issuecomment-870089243
--
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
[GitHub] [arrow] kou commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r660207256
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,23 @@
cmake_minimum_required(VERSION 3.20)
+# This function uses the ThirdpartyToolchain CMake module
+# in cpp/cmake_modules to automatically download and build GoogleTest.
+function(download_and_build_google_test)
+ # The ExternalProject module is required for downloading and building external dependencies.
+ include(ExternalProject)
+ # The ThirdpartyToolchain module automatically builds xsimd by default,
+ # so we explicitly disable building of xsimd here.
+ set(ARROW_SIMD_LEVEL "NONE")
+ set(ARROW_RUNTIME_SIMD_LEVEL "NONE")
+ # The ThirdpartyToolchain module must know the location of the Arrow C++ libraries source code.
+ set(arrow_SOURCE_DIR ${CMAKE_SOURCE_DIR}/../cpp)
+ # The ThirdpartyToolchain module uses ExternalProject to download GoogleTest.
+ include(ThirdpartyToolchain)
Review comment:
`ThirdpartyToolchain isn't expected to be used by `include`.
Could you use `find_package(GTest REQUIRED CONFIG)` instead?
--
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
[GitHub] [arrow] kou commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r666514068
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
+ # Should support custom GTEST_ROOT as well as package managers.
Review comment:
Ah, sorry for my misunderstanding.
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,13 +17,95 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries.
+function(build_arrow)
+ set(options BUILD_GTEST)
+ set(one_value_args)
+ set(multi_value_args)
+ cmake_parse_arguments(ARG
+ "${options}"
+ "${one_value_args}"
+ "${multi_value_args}"
+ ${ARGN})
+ if(ARG_UNPARSED_ARGUMENTS)
+ message(SEND_ERROR "Error: unrecognized arguments: ${ARG_UNPARSED_ARGUMENTS}")
+ endif()
+
+ set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+ set(ARROW_INCLUDE_DIR "${ARROW_PREFIX}/include")
+ set(ARROW_LIBRARY_DIR "${ARROW_PREFIX}/lib")
+ set(ARROW_SHARED_LIB
+ "${ARROW_LIBRARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}arrow${CMAKE_SHARED_LIBRARY_SUFFIX}"
+ )
+ set(ARROW_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-build")
+ set(ARROW_CMAKE_ARGS "-DCMAKE_INSTALL_PREFIX=${ARROW_PREFIX}"
+ "-DCMAKE_INSTALL_LIBDIR=lib")
+ set(ARROW_BUILD_BYPRODUCTS "${ARROW_SHARED_LIB}")
+
+ if(ARG_BUILD_GTEST)
+ build_gtest()
+ endif()
+
+ include(ExternalProject)
+ externalproject_add(arrow_ep
+ SOURCE_DIR "${CMAKE_SOURCE_DIR}/../cpp"
+ BINARY_DIR "${ARROW_BINARY_DIR}"
+ CMAKE_ARGS ${ARROW_CMAKE_ARGS}
+ BUILD_BYPRODUCTS ${ARROW_BUILD_BYPRODUCTS})
+
+ file(MAKE_DIRECTORY "${ARROW_INCLUDE_DIR}")
+
+ add_library(arrow_shared SHARED IMPORTED)
+ set_target_properties(arrow_shared
+ PROPERTIES IMPORTED_LOCATION ${ARROW_SHARED_LIB}
+ INTERFACE_INCLUDE_DIRECTORIES ${ARROW_INCLUDE_DIR})
+
+ add_dependencies(arrow_shared arrow_ep)
+endfunction()
+
+# Build the GoogleTest binaries that are bundled with the Arrow C++ libraries.
+macro(build_gtest)
+ set(ARROW_GTEST_PREFIX "${ARROW_BINARY_DIR}/googletest_ep-prefix")
+ set(ARROW_GTEST_INCLUDE_DIR "${ARROW_GTEST_PREFIX}/include")
+ set(ARROW_GTEST_LIBRARY_DIR "${ARROW_GTEST_PREFIX}/lib")
+ set(ARROW_GTEST_SHARED_LIB "${ARROW_GTEST_LIBRARY_DIR}/libgtest.so")
+
+ set(ARROW_GTEST_MAIN_PREFIX "${ARROW_BINARY_DIR}/googletest_ep-prefix")
+ set(ARROW_GTEST_MAIN_INCLUDE_DIR "${ARROW_GTEST_MAIN_PREFIX}/include")
+ set(ARROW_GTEST_MAIN_LIBRARY_DIR "${ARROW_GTEST_MAIN_PREFIX}/lib")
+ set(ARROW_GTEST_MAIN_SHARED_LIB "${ARROW_GTEST_MAIN_LIBRARY_DIR}/libgtest_main.so")
Review comment:
```suggestion
set(ARROW_GTEST_MAIN_SHARED_LIB "${ARROW_GTEST_MAIN_LIBRARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${CMAKE_SHARED_LIBRARY_SUFFIX}")
```
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,13 +17,95 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries.
+function(build_arrow)
+ set(options BUILD_GTEST)
+ set(one_value_args)
+ set(multi_value_args)
+ cmake_parse_arguments(ARG
+ "${options}"
+ "${one_value_args}"
+ "${multi_value_args}"
+ ${ARGN})
+ if(ARG_UNPARSED_ARGUMENTS)
+ message(SEND_ERROR "Error: unrecognized arguments: ${ARG_UNPARSED_ARGUMENTS}")
+ endif()
+
+ set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+ set(ARROW_INCLUDE_DIR "${ARROW_PREFIX}/include")
+ set(ARROW_LIBRARY_DIR "${ARROW_PREFIX}/lib")
+ set(ARROW_SHARED_LIB
+ "${ARROW_LIBRARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}arrow${CMAKE_SHARED_LIBRARY_SUFFIX}"
+ )
+ set(ARROW_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-build")
+ set(ARROW_CMAKE_ARGS "-DCMAKE_INSTALL_PREFIX=${ARROW_PREFIX}"
+ "-DCMAKE_INSTALL_LIBDIR=lib")
+ set(ARROW_BUILD_BYPRODUCTS "${ARROW_SHARED_LIB}")
+
+ if(ARG_BUILD_GTEST)
+ build_gtest()
+ endif()
+
+ include(ExternalProject)
+ externalproject_add(arrow_ep
+ SOURCE_DIR "${CMAKE_SOURCE_DIR}/../cpp"
+ BINARY_DIR "${ARROW_BINARY_DIR}"
+ CMAKE_ARGS ${ARROW_CMAKE_ARGS}
+ BUILD_BYPRODUCTS ${ARROW_BUILD_BYPRODUCTS})
+
+ file(MAKE_DIRECTORY "${ARROW_INCLUDE_DIR}")
+
+ add_library(arrow_shared SHARED IMPORTED)
+ set_target_properties(arrow_shared
+ PROPERTIES IMPORTED_LOCATION ${ARROW_SHARED_LIB}
+ INTERFACE_INCLUDE_DIRECTORIES ${ARROW_INCLUDE_DIR})
+
+ add_dependencies(arrow_shared arrow_ep)
+endfunction()
+
+# Build the GoogleTest binaries that are bundled with the Arrow C++ libraries.
+macro(build_gtest)
+ set(ARROW_GTEST_PREFIX "${ARROW_BINARY_DIR}/googletest_ep-prefix")
+ set(ARROW_GTEST_INCLUDE_DIR "${ARROW_GTEST_PREFIX}/include")
+ set(ARROW_GTEST_LIBRARY_DIR "${ARROW_GTEST_PREFIX}/lib")
+ set(ARROW_GTEST_SHARED_LIB "${ARROW_GTEST_LIBRARY_DIR}/libgtest.so")
Review comment:
```suggestion
set(ARROW_GTEST_SHARED_LIB "${ARROW_GTEST_LIBRARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}gtest${CMAKE_SHARED_LIBRARY_SUFFIX}")
```
--
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
[GitHub] [arrow] kevingurney commented on pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#issuecomment-880204002
@kou - we updated the description of the pull request to reflect the latest status.
We also discovered a small issue during qualification. If you specified `MATLAB_BUILD_TESTS=ON`, then CMake would always use the Arrow C++ libraries built from source and ignore any custom `ARROW_HOME` or system-installed distributions of Arrow.
We just pushed a small fix for this issue in b12b6fe1398dc4ee2ebb8fb00ce54ab4bfe4012a. Let us know if you have any questions.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r665671192
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
Review comment:
Lowercase is better because we use lowercase for function call:
```suggestion
function(build_arrow)
```
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
Review comment:
Could you add `option(MATLAB_BUILD_TESTS ...)` somewhere?
https://cmake.org/cmake/help/latest/command/option.html
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
+ include(ExternalProject)
+
+ set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+ set(ARROW_INCLUDE_DIR "${ARROW_PREFIX}/include")
+ set(ARROW_LIBRARY_DIR "${ARROW_PREFIX}/lib")
+ set(ARROW_SHARED_LIB "${ARROW_LIBRARY_DIR}/libarrow.so")
+ set(ARROW_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep_build")
+
+ externalproject_add(arrow_ep
+ SOURCE_DIR "${CMAKE_SOURCE_DIR}/../cpp"
+ BINARY_DIR "${ARROW_BINARY_DIR}"
+ CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${ARROW_PREFIX}
Review comment:
We need to specify `CMAKE_INSTALL_LIBDIR=lib` explicitly because some platforms such as CentOS uses `lib64` for the default `CMAKE_INSTALL_LIBDIR`:
```suggestion
CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${ARROW_PREFIX} -DCMAKE_INSTALL_LIBDIR=lib
```
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
+ # Should support custom GTEST_ROOT as well as package managers.
Review comment:
`-DGTest_ROOT` should work with only `find_package(GTest)`. We don't need additional code for it: https://cmake.org/cmake/help/latest/command/find_package.html#search-procedure
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
+ include(ExternalProject)
+
+ set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+ set(ARROW_INCLUDE_DIR "${ARROW_PREFIX}/include")
+ set(ARROW_LIBRARY_DIR "${ARROW_PREFIX}/lib")
+ set(ARROW_SHARED_LIB "${ARROW_LIBRARY_DIR}/libarrow.so")
Review comment:
```suggestion
set(ARROW_SHARED_LIB "${ARROW_LIBRARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}arrow${CMAKE_SHARED_LIBRARY_SUFFIX}")
```
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
+ include(ExternalProject)
+
+ set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+ set(ARROW_INCLUDE_DIR "${ARROW_PREFIX}/include")
+ set(ARROW_LIBRARY_DIR "${ARROW_PREFIX}/lib")
+ set(ARROW_SHARED_LIB "${ARROW_LIBRARY_DIR}/libarrow.so")
+ set(ARROW_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep_build")
Review comment:
```suggestion
set(ARROW_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-build")
```
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
Review comment:
Can we unify `build_arrow` and `build_arrow_and_gtest` by using function arguments?
We can use `cmake_parse_arguments()` for processing arguments: https://cmake.org/cmake/help/latest/command/cmake_parse_arguments.html
See also `resolve_dependency` macro that uses `cmake_parse_arguments()`: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L200-L204
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
+ # Should support custom GTEST_ROOT as well as package managers.
+ find_package(GTest)
+ if(NOT GTest_FOUND)
+ # Trigger an automatic build of the Arrow C++ libraries and bundled GoogleTest binaries.
+ build_arrow_and_gtest()
+ else()
+ # Should support custom ARROW_HOME as well as package managers.
+ find_package(Arrow)
+ if(NOT Arrow_FOUND)
+ # Trigger an automatic build of the Arrow C++ libraries.
+ build_arrow()
+ endif()
+ endif()
+else()
+ # Should support custom ARROW_HOME as well as package managers.
Review comment:
`ARROW_HOME` is processed by `FindArrow.cmake`. So we don't need additional code here.
--
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
[GitHub] [arrow] kou commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r661808349
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
Can we clarify target people of the MATLAB library's test?
1. Users: People who use the MATLAB library.
2. Developers: People who develop the MATLAB library.
3. Both of 1. and 2.
My thought is 2.
For 2., custom Apache Arrow C++ build is useful. For example, developers may want to build with `-DCMAKE_BUILD_TYPE=debug` for debugging and with `-DCMAKE_BUILD_TYPE=release` for benchmarking.
So, it may be reasonable that developers build Apache Arrow C++ manually.
For 1., users may not want to enable tests by default. Because they just want to use the MATLAB library.
If you still want to implement automatic Apache Arrow C++ build in CMake, I'll take a look.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#issuecomment-870082046
https://issues.apache.org/jira/browse/ARROW-13100
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r664631389
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
@kou - my apologies for the delay! I was away for a few days.
I've pushed the changes from mathworks@42609d9 to this branch, so that you can comment on the implementation.
Thank you!
--
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
[GitHub] [arrow] kevingurney commented on pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#issuecomment-876721697
Thanks, @kou for all of your really helpful feedback! We've addressed all of your comments.
We will now focus on adding the additional logic to make this work on Windows and macOS. This hopefully shouldn't take too much effort.
--
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
[GitHub] [arrow] kevingurney commented on pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#issuecomment-870089243
My apologies! It looks like I forgot to add the Apache license header to `matlab/src/placeholder_test.cc`. I will make that change now.
--
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
[GitHub] [arrow] kou commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r660207969
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
Do we need this functionality for MATLAB module?
How about installing GoogleTest via package manager on system such as `apt`, `dnf`, `brew` and so on?
--
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
[GitHub] [arrow] kou commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r660207256
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,23 @@
cmake_minimum_required(VERSION 3.20)
+# This function uses the ThirdpartyToolchain CMake module
+# in cpp/cmake_modules to automatically download and build GoogleTest.
+function(download_and_build_google_test)
+ # The ExternalProject module is required for downloading and building external dependencies.
+ include(ExternalProject)
+ # The ThirdpartyToolchain module automatically builds xsimd by default,
+ # so we explicitly disable building of xsimd here.
+ set(ARROW_SIMD_LEVEL "NONE")
+ set(ARROW_RUNTIME_SIMD_LEVEL "NONE")
+ # The ThirdpartyToolchain module must know the location of the Arrow C++ libraries source code.
+ set(arrow_SOURCE_DIR ${CMAKE_SOURCE_DIR}/../cpp)
+ # The ThirdpartyToolchain module uses ExternalProject to download GoogleTest.
+ include(ThirdpartyToolchain)
Review comment:
`ThirdpartyToolchain` isn't expected to be used by `include`.
Could you use `find_package(GTest REQUIRED CONFIG)` instead?
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r660973945
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
Setting `GTEST_ROOT` to `${ARROW_BUILD_DIR}/googletest_ep-prefix` inside the `CMakeLists.txt` for MATLAB seems like a reasonable approach.
If we could automatically trigger a build of the Arrow C++ libraries with `ARROW_BUILD_TESTS=ON` (in order to get GoogleTest to be built in `${ARROW_BUILD_DIR}/googletest_ep-prefix`) from the MATLAB `CMakeLists.txt` file, that would be great. But, I'm not sure what the best approach is to accomplish this.
It seems like it would be error prone to expect users to manually build the Arrow C++ libraries first due to the variation in names they might use for their CMake build folder, as well as the different flags they might pass to the build (e.g. they might forget to include `-DARROW_BUILD_TESTS=on`).
--
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
[GitHub] [arrow] kou closed pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kou closed pull request #10614:
URL: https://github.com/apache/arrow/pull/10614
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r661920751
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
Thanks for sharing your thoughts, @kou!
-------------
> Can we clarify target people of the MATLAB library's test?
I agree with you that _Developers_ are the primary target audience of the C++ tests for the MATLAB interface.
> For 2., custom Apache Arrow C++ build is useful. For example, developers may want to build with -DCMAKE_BUILD_TYPE=debug for debugging and with -DCMAKE_BUILD_TYPE=release for benchmarking
You make an excellent point about providing developers with the flexibility to build their own custom `libarrow` for debugging and benchmarking purposes! This is definitely something that we would like to support. If we use `find_package`, as you previously suggested, this should support custom `libarrow` builds via `ARROW_HOME`, as well as package managers.
> For 1., users may not want to enable tests by default. Because they just want to use the MATLAB library.
Good point! I think it would make sense to introduce a flag like `MATLAB_BUILD_TESTS`, with a default value of `OFF`, so that users can toggle building of the C++ tests for the MATLAB interface.
-------------
Based on your input, we went back to the drawing board and learned more about `ExternalProject_Add` and `IMPORTED` targets. By studying the source code for `build_dependency` in `ThirdpartyToolchain` more closely, I now understand how `ExternalProject_Add` can be used to trigger an automatic build of the Apache Arrow C++ libraries (and bundled GoogleTest).
I put together a prototype which automatically builds Arrow C++ and bundled GoogleTest on Linux (only if the user has specified `-DMATLAB_BUILD_TESTS=ON`).
**You can find the prototype here:**
https://github.com/mathworks/arrow/commit/42609d9671501c8fe1e8c613337eedb207d32e91
I tested this manually under the following conditions:
1. GoogleTest installed via the `apt` package manager to `/usr/lib/...` on Debian 10
2. A custom `GTEST_ROOT` and a custom `ARROW_HOME` value are specified
3. `find_package` fails to find a valid Arrow or GTest installation via 1. or 2., and an automatic C++ source build is triggered
The code could probably benefit from some additional refactoring, and I'm not sure whether "Modern CMake Best Practices" would deem it acceptable to depend on the bundled GoogleTest binaries in this way, but it does seem to be working as expected.
Let me know what you think.
Thanks!
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r667005197
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
+ # Should support custom GTEST_ROOT as well as package managers.
Review comment:
No need to apologize! The original wording was confusing.
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r666223346
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
+ include(ExternalProject)
+
+ set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+ set(ARROW_INCLUDE_DIR "${ARROW_PREFIX}/include")
+ set(ARROW_LIBRARY_DIR "${ARROW_PREFIX}/lib")
+ set(ARROW_SHARED_LIB "${ARROW_LIBRARY_DIR}/libarrow.so")
+ set(ARROW_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep_build")
+
+ externalproject_add(arrow_ep
+ SOURCE_DIR "${CMAKE_SOURCE_DIR}/../cpp"
+ BINARY_DIR "${ARROW_BINARY_DIR}"
+ CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${ARROW_PREFIX}
Review comment:
Thanks for pointing this out! I didn't realize that some platforms will install shared libraries to directories other than `lib` by default.
--
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
[GitHub] [arrow] kou commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r660945071
##########
File path: matlab/CMakeLists.txt
##########
@@ -72,3 +89,28 @@ else()
set_target_properties(featherwritemex PROPERTIES LIBRARY_OUTPUT_DIRECTORY
$<1:${CMAKE_SOURCE_DIR}/src>)
endif()
+
+# ######################
+# GoogleTest Integration
+# ######################
+# If the user has specified a GTEST_ROOT value, use their pre-built GoogleTest binaries.
+# Otherwise, download and build GoogleTest automatically.
+if(GTEST_ROOT)
+ find_package(GTest REQUIRED)
+else()
+ download_and_build_google_test()
Review comment:
OK.
How about referring GoogleTest bundled in Apache Arrow C++ instead of adding this functionality to the MATLAB library?
If we build bundled GoogleTest in Apache Arrow C++, it's installed into `${ARROW_BUILD_DIR}/googletest_ep-prefix/{include,lib}/`. We can find it with `find_package(GTest REQUIRED CONFIG)` and `-DGTest_ROOT=${ARROW_BUILD_DIR}/googletest_ep-prefix`.
And `find_package(GTest REQUIED COFNIG)` without `-DGTest_ROOT` will find GoogleTest installed into system by package manager.
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,23 @@
cmake_minimum_required(VERSION 3.20)
+# This function uses the ThirdpartyToolchain CMake module
+# in cpp/cmake_modules to automatically download and build GoogleTest.
+function(download_and_build_google_test)
+ # The ExternalProject module is required for downloading and building external dependencies.
+ include(ExternalProject)
+ # The ThirdpartyToolchain module automatically builds xsimd by default,
+ # so we explicitly disable building of xsimd here.
+ set(ARROW_SIMD_LEVEL "NONE")
+ set(ARROW_RUNTIME_SIMD_LEVEL "NONE")
+ # The ThirdpartyToolchain module must know the location of the Arrow C++ libraries source code.
+ set(arrow_SOURCE_DIR ${CMAKE_SOURCE_DIR}/../cpp)
+ # The ThirdpartyToolchain module uses ExternalProject to download GoogleTest.
+ include(ThirdpartyToolchain)
Review comment:
`ThirdpartyToolchain` has many Apache Arrow C++ specific configurations like `configure_file()` you changed.
So you need to set some `ARROW_*` variables to `include` `ThirdpartyToolchain`.
If we want to reuse `ThirdpartyToolchain` in the MATLAB library, we should make `ThirdpartyToolchain` reusable and reuse rather than `set` some variables and `include`.
--
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
[GitHub] [arrow] kevingurney commented on a change in pull request #10614: ARROW-13100: [MATLAB] Integrate GoogleTest with MATLAB Interface C++ Code
Posted by GitBox <gi...@apache.org>.
kevingurney commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r666260099
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
+ # Should support custom GTEST_ROOT as well as package managers.
Review comment:
Fixed.
--
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