GH-37532: [CI][Docs][MATLAB] Remove `GoogleTest` support from the CMake build system for the MATLAB interface by kevingurney · Pull Request #37784 · apache/arrow

kevingurney · 2023-09-18T20:38:10Z

Rationale for this change

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

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.
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.
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 [CI][Docs][MATLAB] Remove GoogleTest support from the CMake build system for the MATLAB interface #37532 (comment).
There is precedent for testing internal C++ code without GoogleTest for the Python bindings.
On a somewhat related note - removing GoogleTest support will help unblock GH-37770: [MATLAB] Add CSV TableReader and TableWriter MATLAB classes #37773 as discussed in GH-37770: [MATLAB] Add CSV TableReader and TableWriter MATLAB classes #37773 (comment).

What changes are included in this PR?

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.
Updated the matlab_build.sh CI workflow script to avoid building the tests for the Arrow C++ libraries and to no longer call ctest.
Updated the README.md for the MATLAB interface to no longer mention building or running C++ tests.
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.
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

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

Notes

In the future, we can consider testing internal C++ code using MEX function calls from MATLAB tests as suggested by @kou in [CI][Docs][MATLAB] Remove GoogleTest support from the CMake build system for the MATLAB interface #37532 (comment). Currently, we don't have any C++ tests that need to be adapted to use this approach.
Thank you @sgilmore10 for your help with this pull request!

Closes: [CI][Docs][MATLAB] Remove GoogleTest support from the CMake build system for the MATLAB interface #37532

README.md.

script. Co-authored-by: Sarah Gilmore <[email protected]>

kevingurney · 2023-09-18T21:37:09Z

We probably need to see how things change run-over-run, but it looks like removing GoogleTest support may have reduced the MATLAB CI workflow time a bit: https://github.com/apache/arrow/actions/runs/6228155477?pr=37784.

kou

+1

matlab/doc/matlab_interface_for_apache_arrow_design.md

kou · 2023-09-18T23:52:25Z

CI time on main: https://github.com/apache/arrow/actions/runs/6224657359

Ubuntu: 19m 41s
macOS: 24m 47s
Windows: 33m 0s

CI time on this PR: https://github.com/mathworks/arrow/actions/runs/6228155036

Ubuntu: 7m 53s
macOS: 11m 3s
Windows: 10m 43s

assignUser

Looks good, always a fan of reducing complexity :D

One thing for a follow up would be to use fetchContent instead of externalProject to build libarrow, that way you don't have to manually create the targets and add the imported location etc.

kevingurney · 2023-09-19T14:28:31Z

@assignUser - using FetchContent rather than ExternalProject sounds like a great idea!

However, I was under the impression that the Arrow C++ libraries weren't properly configured to be used with FetchContent (I'm not sure what the exact issues were).

I remember stumbling upon a few different references which seemed to indicate that there were issues with FetchContent:

Do you know if this changed recently? If so, this is definitely something we would love to do.

assignUser · 2023-09-19T14:32:39Z

Ah that's true... we should fix that ^^

Though @amoeba has an example but that needs a hack so I am inclined to agree the ep is the better choice for now.

kevingurney · 2023-09-19T17:19:12Z

we should fix that ^^

Agreed! Supporting FetchContent is definitely something that seems like it could help simplify adoption of the Arrow C++ libraries in other projects.

Though @amoeba has an example

Thanks for sharing this! I wasn't aware there was a working example.

As you said, I think that sticking with ExternalProject for the time being is a good idea, given the need for the hack. However, we would be happy to move over to FetchContent if this gets resolved in the future.

2. Add a note about testing internal C++ code via MEX function calls.

kevingurney · 2023-09-19T20:05:32Z

+1

conbench-apache-arrow · 2023-09-20T05:03:55Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit cda1e8f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

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

kevingurney and others added 8 commits September 18, 2023 15:14

Remove MATLAB_BUILD_TESTS flag from CMake build system.

f33cb98

Remove build_gtest and enable_gtest macros.

366316d

Remove the BUILD_GTEST option from the build_arrow function.

75d76b5

Modify comments.

e4ffc74

Delete C++ placeholder test.

469017c

Update design document to remove mention of GoogleTest.

c9100db

Remove mention of C++ tests and MATLAB_BUILD_TESTS flag from the

8fce8c1

README.md.

Remove MATLAB_BUILD_TESTS=ON and ctest call from matlab_build.sh CI

b14c663

script. Co-authored-by: Sarah Gilmore <[email protected]>

kevingurney requested review from assignUser, kou and raulcd as code owners September 18, 2023 20:38

github-actions bot added Component: MATLAB Component: Documentation awaiting committer review Awaiting committer review labels Sep 18, 2023

kevingurney added 2 commits September 18, 2023 16:40

Fix typo.

078f96d

Fix CMake linting error.

a7f41b2

kevingurney mentioned this pull request Sep 18, 2023

GH-37770: [MATLAB] Add CSV TableReader and TableWriter MATLAB classes #37773

Merged

kou approved these changes Sep 18, 2023

View reviewed changes

matlab/doc/matlab_interface_for_apache_arrow_design.md Outdated Show resolved Hide resolved

github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Sep 18, 2023

assignUser approved these changes Sep 19, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Sep 19, 2023

1. Modify testing section.

a9d7622

2. Add a note about testing internal C++ code via MEX function calls.

github-actions bot removed the awaiting changes Awaiting changes label Sep 19, 2023

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 19, 2023

kevingurney merged commit cda1e8f into apache:main Sep 19, 2023

kevingurney deleted the GH-37532 branch September 19, 2023 20:06

kevingurney removed the awaiting changes Awaiting changes label Sep 19, 2023

kevingurney mentioned this pull request Sep 20, 2023

[MATLAB] Reduce MATLAB CI Build Time by Building Arrow C++ Libraries separately and using Packaged GoogleTest #29285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37532: [CI][Docs][MATLAB] Remove `GoogleTest` support from the CMake build system for the MATLAB interface#37784

GH-37532: [CI][Docs][MATLAB] Remove `GoogleTest` support from the CMake build system for the MATLAB interface#37784
kevingurney merged 11 commits intoapache:mainfrom
mathworks:GH-37532

kevingurney commented Sep 18, 2023 •

edited

Loading

Uh oh!

kevingurney commented Sep 18, 2023 •

edited

Loading

Uh oh!

kou left a comment

Uh oh!

Uh oh!

kou commented Sep 18, 2023

Uh oh!

assignUser left a comment

Uh oh!

kevingurney commented Sep 19, 2023

Uh oh!

assignUser commented Sep 19, 2023

Uh oh!

kevingurney commented Sep 19, 2023

Uh oh!

kevingurney commented Sep 19, 2023

Uh oh!

conbench-apache-arrow bot commented Sep 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kevingurney commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Future Directions

Notes

Uh oh!

kevingurney commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kou commented Sep 18, 2023

Uh oh!

assignUser left a comment

Choose a reason for hiding this comment

Uh oh!

kevingurney commented Sep 19, 2023

Uh oh!

assignUser commented Sep 19, 2023

Uh oh!

kevingurney commented Sep 19, 2023

Uh oh!

kevingurney commented Sep 19, 2023

Uh oh!

conbench-apache-arrow bot commented Sep 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevingurney commented Sep 18, 2023 •

edited

Loading

kevingurney commented Sep 18, 2023 •

edited

Loading