GH-37067: [C++] Install bundled GoogleTest #37483

kou · 2023-08-31T01:48:37Z

Rationale for this change

Because libarrow*_testing.so depend it. If we don't install bundled GoogleTest, users can't use libarrow*_testing.so.

What changes are included in this PR?

If we install bundled GoogleTest as-is, it may conflict existing GoogleTest. So this PR renames GoogleTest when we install GoogleTest:

lib{gtest,gtest_main,gmock,gmock_main}.so -> libarrow_{gtest,gtest_main,gmock,gmock_main}.so
${PREFIX}/include/{gtest,gmock}/ -> ${PREFIX}/include/arrow-gtest/${gtest,gmock}/

Other CMake packages and .pc files aren't installed. ArrowTesting CMake package and arrow-testing.pc configures bundled GoogleTest instead.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

Closes: [C++][CI] Flight tests fail running on some configurations #37067

kou · 2023-08-31T01:48:59Z

@github-actions crossbow submit test-debian-11-* test-ubuntu-22.04-*

kou · 2023-08-31T04:53:21Z

@github-actions crossbow submit test-debian-11-* test-ubuntu-22.04-*

github-actions · 2023-08-31T04:55:59Z

Revision: 5e13c25

Submitted crossbow builds: ursacomputing/crossbow @ actions-311479b333

Task	Status
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-go-1.17
test-debian-11-go-1.20
test-debian-11-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-docs
test-ubuntu-22.04-python-3

kevingurney · 2023-08-31T16:09:20Z

Thanks for working on this @kou!

It seems that the C++ tests are failing for the macOS MATLAB build.

Unfortunately the reason for the failure isn't obvious from the build logs.

@sgilmore10 and I working on trying to reproduce this failure locally.

kevingurney · 2023-08-31T16:28:02Z

@sgilmore10 and I did manage to reproduce the test failures locally on macOS.

Here is the log output (note I replaced some personal information from the file paths with "..." ):

dyld[26962]: Library not loaded: libarrow_gtest.1.11.0.dylib

  Referenced from: <02638200-BA25-3D9D-AF66-D4C71F49AEFC> /System/Volumes/Data/.../arrow/matlab/build/placeholder_test

  Reason: tried: 'libarrow_gtest.1.11.0.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OSlibarrow_gtest.1.11.0.dylib' (no such file), 'libarrow_gtest.1.11.0.dylib' (no such file), '/usr/local/lib/libarrow_gtest.1.11.0.dylib' (no such file), '/usr/lib/libarrow_gtest.1.11.0.dylib' (no such file, not in dyld cache), '/System/Volumes/Data/.../arrow/matlab/build/libarrow_gtest.1.11.0.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/System/Volumes/Data/.../arrow/matlab/build/libarrow_gtest.1.11.0.dylib' (no such file), '/System/Volumes/Data/.../arrow/matlab/build/libarrow_gtest.1.11.0.dylib' (no such file), '/usr/local/lib/libarrow_gtest.1.11.0.dylib' (no such file), '/usr/lib/libarrow_gtest.1.11.0.dylib' (no such file, not in dyld cache)

It appears that the linker is unable to find libarrow_gtest.1.11.0.dylib. However, we manually verified that ibarrow_gtest.1.11.0.dylib does appear under ${MATLAB_BUILD_DIR}/${ARROW_EP_PREFIX}/lib (e.g. .../matlab/build/arrow_ep-prefix/lib/) as expected.

It seems that the linker is not searching in .../matlab/build/arrow_ep-prefix/lib. We suspect this is the issue.

kevingurney · 2023-08-31T19:32:03Z

It seems like this is some kind of rpath-related issue on macOS. There is an rpath warning in the build logs:

CMake Warning (dev):
  Policy CMP0042 is not set: MACOSX_RPATH is enabled by default.  Run "cmake
  --help-policy CMP0042" for policy details.  Use the cmake_policy command to
  set the policy and suppress this warning.

  MACOSX_RPATH is not specified for the following targets:

   gmock
   gmock_main
   gtest
   gtest_main

sgilmore10 · 2023-08-31T22:04:04Z

Hi @kou

@kevingurney and I spent some more time investigating this failure. Here's what we found:

If you run the ldd command on the placeholder_test executable (with these changes), the names of the gtest and gtest_main libraries are specified as relative paths, while other library names are specified as absolute paths:

>> ldd placeholder_test
placeholder_test:
	libarrow_gtest.1.11.0.dylib (compatibility version 0.0.0, current version 1.11.0)
	libarrow_gtest_main.1.11.0.dylib (compatibility version 0.0.0, current version 1.11.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1300.36.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.0.0)

However, if you run ldd on the placeholder_test executable built prior to these changes, the names for gtest and gtest_main are specified as absolute paths:

>> ldd placeholder_test
placeholder_test:
	</path/to/repo>/arrow/matlab/build/arrow_ep-build/googletest_ep-prefix/lib/libgtest.1.11.0.dylib (compatibility version 0.0.0, current version 1.11.0)
	</path/to/repo>/arrow/matlab/build/arrow_ep-build/googletest_ep-prefix/lib/libgtest_main.1.11.0.dylib (compatibility version 0.0.0, current version 1.11.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1300.36.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.0.0)

Note: I replaced some personal information with </path/to/repo>.

We were able to run the placeholder_test executable successfully if we made the changes below:
- Specify the MACOSX_RPATH property to TRUE for both targets gtest and gtest_main in arrow/cpp/cmake_modules/ThirdartyToolchain.cmake.
- Specify the BUILD_RPATH property to "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix/lib" for target placeholder_test in arrow/matlab/CMakeLists.txt.

With those changes, the executable runs as expected:

Internal ctest changing into directory: <path/to/repo>/arrow/matlab/build
Test project <path/to/repo>/arrow/matlab/build
    Start 1: PlaceholderTestTarget
1/1 Test #1: PlaceholderTestTarget ............   Passed    0.01 sec

100% tests passed, 0 tests failed out of 1

Total Test time (real) =   0.03 sec

For reference, here's the output of ldd placeholder with these changes applied:

placeholder_test:
	@rpath/libarrow_gtest.1.11.0.dylib (compatibility version 0.0.0, current version 1.11.0)
	@rpath/libarrow_gtest_main.1.11.0.dylib (compatibility version 0.0.0, current version 1.11.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1300.36.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.0.0)

I'm not sure if this solution is the right one, but wanted to share with you what we found.

Best,
Sarah

Because `libarrow*_testing.so` depend it.

kou · 2023-09-01T03:23:21Z

@kevingurney @sgilmore10 Thanks for your help! It's very helpful. I'll use the MACOS_RPATH in cpp/ approach.

BTW, it seems that we can remove GoogleTest support from matlab/ because we can run all unit tests for MATLAB with the MATLAB's testing framework now.

kou · 2023-09-01T05:41:49Z

Ah, do we need to set both of MACOSX_RPATH and BUILD_RPATH?

sgilmore10 · 2023-09-01T12:54:23Z

Ah, do we need to set both of MACOSX_RPATH and BUILD_RPATH?

Yes, I had to set both to get it to work.

sgilmore10 · 2023-09-01T15:13:12Z

@kevingurney @sgilmore10 Thanks for your help! It's very helpful. I'll use the MACOS_RPATH in cpp/ approach.

BTW, it seems that we can remove GoogleTest support from matlab/ because we can run all unit tests for MATLAB with the MATLAB's testing framework now.

I think we want to keep the GoogleTest support for now because we do plan on adding C++ unit tests. There are a few error-conditions that cannot be triggered from MATLAB for which we want to add C++ unit tests.

kevingurney · 2023-09-01T23:45:45Z

I agree with @sgilmore10. I don't think it would be ideal to completely remove GoogleTest because C++ tests would be beneficial for the MATLAB interface.

kevingurney · 2023-09-01T23:46:29Z

@kou - sorry! I accidentally hit the "Close with comment" button rather than the "Comment" button. I just reopened the pull request.

kou · 2023-09-04T00:58:51Z

@sgilmore10 @kevingurney I see. I don't remove the GoogleTest support in matlab/ in this PR. But can we discuss it in #37532?

kou · 2023-09-04T00:59:24Z

I'll merge this in this week if nobody objects this.

kou · 2023-09-04T01:00:41Z

Ah, do we need to set both of MACOSX_RPATH and BUILD_RPATH?

Yes, I had to set both to get it to work.

Thanks! It worked by adding BUILD_RPATH too!

kevingurney · 2023-09-05T14:10:53Z

@kou - very sorry for the delayed response. I was out sick at the end of last week and am just getting caught up now.

These changes look good!

Thanks for bringing up the GoogleTest question and capturing it in #37532. After further consideration, we agree that it is worth thinking more about removing GoogleTest. As you suggested, we can discuss this in more detail on #37532.

Thank you!

kevingurney · 2023-09-05T14:10:57Z

+1

kou · 2023-09-06T01:43:24Z

No problem. I'm not in hurry. :-)

Thanks for your approvement. I merge this.

conbench-apache-arrow · 2023-09-07T10:34:06Z

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

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.

danepitkin · 2023-10-06T20:02:02Z

@github-actions crossbow submit java-jars

github-actions · 2023-10-06T20:04:44Z

Revision: 52138c1

Submitted crossbow builds: ursacomputing/crossbow @ actions-e36e4f51b2

Task	Status
java-jars

### Rationale for this change Because `libarrow*_testing.so` depend it. If we don't install bundled GoogleTest, users can't use `libarrow*_testing.so`. ### What changes are included in this PR? If we install bundled GoogleTest as-is, it may conflict existing GoogleTest. So this PR renames GoogleTest when we install GoogleTest: * `lib{gtest,gtest_main,gmock,gmock_main}.so` -> `libarrow_{gtest,gtest_main,gmock,gmock_main}.so` * `${PREFIX}/include/{gtest,gmock}/` -> `${PREFIX}/include/arrow-gtest/${gtest,gmock}/` Other CMake packages and `.pc` files aren't installed. `ArrowTesting` CMake package and `arrow-testing.pc` configures bundled GoogleTest instead. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * Closes: apache#37067 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

kou force-pushed the cpp-gtest-bundled-install branch from a00e50a to 763a7e9 Compare August 31, 2023 01:48

github-actions bot added Component: C++ awaiting committer review Awaiting committer review labels Aug 31, 2023

This comment was marked as outdated.

Sign in to view

kou force-pushed the cpp-gtest-bundled-install branch from c0d00c9 to 5e13c25 Compare August 31, 2023 04:53

kou requested a review from kevingurney as a code owner August 31, 2023 04:53

github-actions bot added the Component: MATLAB label Aug 31, 2023

kou added 8 commits September 1, 2023 12:20

apacheGH-37067: [C++] Install bundled GoogleTest

943f0b9

Because `libarrow*_testing.so` depend it.

Add missing gmock dependencies

583a54c

Can't use /WX:NO and use function for scope

5313e99

Use static link by default

a110cc9

Adjust MATLAB

64f2766

Fix path

b512457

Fix path

4decaf9

Enable MACOSX_RPATH by default

4fff568

kou force-pushed the cpp-gtest-bundled-install branch from 633b61e to 4fff568 Compare September 1, 2023 03:20

Set MACOSX_RPATH explicitly

6be7127

Use CMAKE_MACOSX_RPATH

d0aa123

kevingurney closed this Sep 1, 2023

kevingurney reopened this Sep 1, 2023

kou added 3 commits September 3, 2023 16:09

Fix style

3be85b2

Set BUILD_RPATH

893da3b

Use cache variable

52138c1

kou mentioned this pull request Sep 4, 2023

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

Closed

kevingurney approved these changes Sep 5, 2023

View reviewed changes

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

kou merged commit ad7f6ef into apache:main Sep 6, 2023
34 checks passed

kou deleted the cpp-gtest-bundled-install branch September 6, 2023 01:44

kou removed the awaiting merge Awaiting merge label Sep 6, 2023

bkietz mentioned this pull request Sep 7, 2023

[C++] Bundled GTest CI jobs seem to #includeing GTest headers from somewhere else #36379

Closed

pitrou mentioned this pull request Sep 8, 2023

GH-36379: [C++] Bundled dependency include paths should override system include dirs #37612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37067: [C++] Install bundled GoogleTest #37483

GH-37067: [C++] Install bundled GoogleTest #37483

kou commented Aug 31, 2023 •

edited by github-actions bot

Loading

kou commented Aug 31, 2023

This comment was marked as outdated.

kou commented Aug 31, 2023

github-actions bot commented Aug 31, 2023

kevingurney commented Aug 31, 2023

kevingurney commented Aug 31, 2023 •

edited

Loading

kevingurney commented Aug 31, 2023

sgilmore10 commented Aug 31, 2023

kou commented Sep 1, 2023 •

edited

Loading

kou commented Sep 1, 2023

sgilmore10 commented Sep 1, 2023

sgilmore10 commented Sep 1, 2023

kevingurney commented Sep 1, 2023 •

edited

Loading

kevingurney commented Sep 1, 2023

kou commented Sep 4, 2023

kou commented Sep 4, 2023

kou commented Sep 4, 2023

kevingurney commented Sep 5, 2023 •

edited

Loading

kevingurney commented Sep 5, 2023

kou commented Sep 6, 2023

conbench-apache-arrow bot commented Sep 7, 2023

danepitkin commented Oct 6, 2023

github-actions bot commented Oct 6, 2023

GH-37067: [C++] Install bundled GoogleTest #37483

GH-37067: [C++] Install bundled GoogleTest #37483

Conversation

kou commented Aug 31, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

kou commented Aug 31, 2023

This comment was marked as outdated.

kou commented Aug 31, 2023

github-actions bot commented Aug 31, 2023

kevingurney commented Aug 31, 2023

kevingurney commented Aug 31, 2023 • edited Loading

kevingurney commented Aug 31, 2023

sgilmore10 commented Aug 31, 2023

kou commented Sep 1, 2023 • edited Loading

kou commented Sep 1, 2023

sgilmore10 commented Sep 1, 2023

sgilmore10 commented Sep 1, 2023

kevingurney commented Sep 1, 2023 • edited Loading

kevingurney commented Sep 1, 2023

kou commented Sep 4, 2023

kou commented Sep 4, 2023

kou commented Sep 4, 2023

kevingurney commented Sep 5, 2023 • edited Loading

kevingurney commented Sep 5, 2023

kou commented Sep 6, 2023

conbench-apache-arrow bot commented Sep 7, 2023

danepitkin commented Oct 6, 2023

github-actions bot commented Oct 6, 2023

kou commented Aug 31, 2023 •

edited by github-actions bot

Loading

kevingurney commented Aug 31, 2023 •

edited

Loading

kou commented Sep 1, 2023 •

edited

Loading

kevingurney commented Sep 1, 2023 •

edited

Loading

kevingurney commented Sep 5, 2023 •

edited

Loading