Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Sep 19, 2023

Conversation

kevingurney
Copy link
Member

@kevingurney kevingurney commented Sep 18, 2023

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 [CI][Docs][MATLAB] Remove GoogleTest support from the CMake build system for the MATLAB interface #37532 (comment).
  4. There is precedent for testing internal C++ code without GoogleTest for the Python bindings.
  5. 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?

  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 [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.
  2. Thank you @sgilmore10 for your help with this pull request!

@kevingurney
Copy link
Member Author

kevingurney commented Sep 18, 2023

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.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

matlab/doc/matlab_interface_for_apache_arrow_design.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Sep 18, 2023
@kou
Copy link
Member

kou commented Sep 18, 2023

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

Copy link
Member

@assignUser assignUser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member Author

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.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Sep 19, 2023
2. Add a note about testing internal C++ code via MEX function calls.
@github-actions github-actions bot removed the awaiting changes Awaiting changes label Sep 19, 2023
@github-actions 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
Copy link
Member Author

+1

@kevingurney kevingurney merged commit cda1e8f into apache:main Sep 19, 2023
16 checks passed
@kevingurney kevingurney deleted the GH-37532 branch September 19, 2023 20:06
@kevingurney kevingurney removed the awaiting changes Awaiting changes label Sep 19, 2023
@conbench-apache-arrow
Copy link

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.

loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…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]>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI][Docs][MATLAB] Remove GoogleTest support from the CMake build system for the MATLAB interface
3 participants