GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest by kou · Pull Request #43465 · apache/arrow

kou · 2024-07-29T06:59:13Z

Rationale for this change

If we use bundled GoogleTest and system other dependencies such as Boost, our include path options may be:

-isystem /opt/homebrew/include (for Boost)
-isystem build_dir/_deps/googletest-src/googletest (for bundled GoogleTest)
-isystem build_dir/_deps/googletest-src/googlemock (for bundled GoogleTest)

With this order, GoogleTest headers in /opt/homebrew/include/ are used with bundled GoogleTest. It may cause link errors.

What changes are included in this PR?

This change introduces a new CMake target
arrow::GTest::gtest_headers that has include paths for bundled GoogleTest. And it's always used as the first link library of all test program. With this change, our include path options are:

-isystem build_dir/_deps/googletest-src/googletest (for bundled GoogleTest)
-isystem build_dir/_deps/googletest-src/googlemock (for bundled GoogleTest)
-isystem /opt/homebrew/include (for Boost)

With this order, we can always use our bundled GoogleTest.

arrow::GTest::gtest_headers is defined only when we use bundled GoogleTest. So this doesn't change the system GoogleTest case.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

GitHub Issue: [C++] Test linkage error when googletest 1.15.0 is installed system wide despite bundling #43400

…dled GoogleTest If we use bundled GoogleTest and system other dependencies such as Boost, our include path options may be: * `-isystem /opt/homebrew/include` (for Boost) * `-isystem build_dir/_deps/googletest-src/googletest` (for bundled GoogleTest) * `-isystem build_dir/_deps/googletest-src/googlemock` (for bundled GoogleTest) With this order, GoogleTest headers in `/opt/homebrew/include/` are used with bundled GoogleTest. It may cause link errors. This change introduces a new CMake target `arrow::GTest::gtest_headers` that has include paths for bundled GoogleTest. And it's always used as the first link library of all test program. With this change, our include path options are: * `-isystem build_dir/_deps/googletest-src/googletest` (for bundled GoogleTest) * `-isystem build_dir/_deps/googletest-src/googlemock` (for bundled GoogleTest) * `-isystem /opt/homebrew/include` (for Boost) With this order, we can always use our bundled GoogleTest. `arrow::GTest::gtest_headers` is defined only when we use bundled GoogleTest. So this doesn't change the system GoogleTest case.

github-actions · 2024-07-29T06:59:38Z

⚠️ GitHub issue #43400 has been automatically assigned in GitHub to PR creator.

kou · 2024-07-29T07:00:06Z

@github-actions crossbow submit java-jars

github-actions · 2024-07-29T07:02:24Z

Revision: 8aef170

Submitted crossbow builds: ursacomputing/crossbow @ actions-6a37a18c2b

Task	Status
java-jars

assignUser · 2024-07-30T01:46:12Z

This is an issue I have seen before with other dependencies when bundled deps are used (e.g. due to version mismatch) but they are also installed on the system, this solution works for gtest but I usually recommend to either upgrade or remove the system dependency...

kou · 2024-07-30T01:53:59Z

this solution works for gtest but I usually recommend to either upgrade or remove the system dependency...

My recommendation is "use system dependencies as much as possible" or "remove the system dependency".

BTW, can we merge this?

assignUser

I assume the special header target is necessary instead of just linking to gtest first?

kou · 2024-07-30T01:59:48Z

Ah, are you suggesting the existing brew uninstall googletest approach?

assignUser · 2024-07-30T02:08:09Z

@kou ah no, my question was why you need to create gtest_headers, could you not also just link against the normal gtest target first? Or would that mess with the link order for gtest deps?

kou · 2024-07-30T02:09:44Z

I assume the special header target is necessary instead of just linking to gtest first?

Right.
If a test executable needs arrow_testing_shared, the test executable needs to link to arrow_testing_shared and GTest::gtest* but GoogleTest header files must exist before arrow_testing_shared and other system dependencies such as OpenSSL::*. If we have the special header target, we can use GTest::gtest_headers, OpenSSL::*, arrow_testing_shared and GTest::gtest order.

kou · 2024-07-30T02:10:37Z

Ah, are you suggesting the existing brew uninstall googletest approach?

Sorry. I wrote this comment before I saw #43465 (review) .

conbench-apache-arrow · 2024-07-30T11:04:52Z

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

There were no benchmark performance regressions. 🎉

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

…dled GoogleTest (apache#43465) ### Rationale for this change If we use bundled GoogleTest and system other dependencies such as Boost, our include path options may be: * `-isystem /opt/homebrew/include` (for Boost) * `-isystem build_dir/_deps/googletest-src/googletest` (for bundled GoogleTest) * `-isystem build_dir/_deps/googletest-src/googlemock` (for bundled GoogleTest) With this order, GoogleTest headers in `/opt/homebrew/include/` are used with bundled GoogleTest. It may cause link errors. ### What changes are included in this PR? This change introduces a new CMake target `arrow::GTest::gtest_headers` that has include paths for bundled GoogleTest. And it's always used as the first link library of all test program. With this change, our include path options are: * `-isystem build_dir/_deps/googletest-src/googletest` (for bundled GoogleTest) * `-isystem build_dir/_deps/googletest-src/googlemock` (for bundled GoogleTest) * `-isystem /opt/homebrew/include` (for Boost) With this order, we can always use our bundled GoogleTest. `arrow::GTest::gtest_headers` is defined only when we use bundled GoogleTest. So this doesn't change the system GoogleTest case. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#43400 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>

…ixes. (#81) * apacheGH-30866: [Java] fix SplitAndTransfer throws for (0,0) if vector empty (apache#41066) This is addresses https://issues.apache.org/jira/browse/ARROW-15382 and is reopening of apache#12250 (which I asked to be reopened). I tried to address all the comments from the previous discussion, added some more tests and fixed an issue in the old commit. * GitHub Issue: apache#30866 Authored-by: Finn Völkel <finn.volkel@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com> * apacheGH-43463: [C++][Gandiva] Always use gdv_function_stubs.h in context_helper.cc (apache#43464) ### Rationale for this change `gdv_function_stubs.h` has declarations of functions in `context_helper.cc`. If we don't include `gdv_function_stubs.h`, it causes attribution mismatch error with unity build. ### What changes are included in this PR? Always include `gdv_function_stubs.h` in `context_helper.cc`. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#43463 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> * apacheGH-43119: [CI][Packaging] Update manylinux 2014 CentOS repos that have been deprecated (apache#43121) ### Rationale for this change Jobs are failing to find mirrorlist.centos.org ### What changes are included in this PR? Updating repos based on solution from: apache#43119 (comment) ### Are these changes tested? Via archery ### Are there any user-facing changes? No * GitHub Issue: apache#43119 Lead-authored-by: Raúl Cumplido <raulcumplido@gmail.com> Co-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com> * Update macos deployment target to 12 to match build machine. * apacheGH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest (apache#43465) ### Rationale for this change If we use bundled GoogleTest and system other dependencies such as Boost, our include path options may be: * `-isystem /opt/homebrew/include` (for Boost) * `-isystem build_dir/_deps/googletest-src/googletest` (for bundled GoogleTest) * `-isystem build_dir/_deps/googletest-src/googlemock` (for bundled GoogleTest) With this order, GoogleTest headers in `/opt/homebrew/include/` are used with bundled GoogleTest. It may cause link errors. ### What changes are included in this PR? This change introduces a new CMake target `arrow::GTest::gtest_headers` that has include paths for bundled GoogleTest. And it's always used as the first link library of all test program. With this change, our include path options are: * `-isystem build_dir/_deps/googletest-src/googletest` (for bundled GoogleTest) * `-isystem build_dir/_deps/googletest-src/googlemock` (for bundled GoogleTest) * `-isystem /opt/homebrew/include` (for Boost) With this order, we can always use our bundled GoogleTest. `arrow::GTest::gtest_headers` is defined only when we use bundled GoogleTest. So this doesn't change the system GoogleTest case. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#43400 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de> --------- Signed-off-by: David Li <li.davidm96@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de> Co-authored-by: Finn Völkel <FiV0@users.noreply.github.com> Co-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

…dled GoogleTest (apache#43465) ### Rationale for this change If we use bundled GoogleTest and system other dependencies such as Boost, our include path options may be: * `-isystem /opt/homebrew/include` (for Boost) * `-isystem build_dir/_deps/googletest-src/googletest` (for bundled GoogleTest) * `-isystem build_dir/_deps/googletest-src/googlemock` (for bundled GoogleTest) With this order, GoogleTest headers in `/opt/homebrew/include/` are used with bundled GoogleTest. It may cause link errors. ### What changes are included in this PR? This change introduces a new CMake target `arrow::GTest::gtest_headers` that has include paths for bundled GoogleTest. And it's always used as the first link library of all test program. With this change, our include path options are: * `-isystem build_dir/_deps/googletest-src/googletest` (for bundled GoogleTest) * `-isystem build_dir/_deps/googletest-src/googlemock` (for bundled GoogleTest) * `-isystem /opt/homebrew/include` (for Boost) With this order, we can always use our bundled GoogleTest. `arrow::GTest::gtest_headers` is defined only when we use bundled GoogleTest. So this doesn't change the system GoogleTest case. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#43400 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>

github-actions bot added the Component: C++ label Jul 29, 2024

Remove workaround

8aef170

github-actions bot added the awaiting committer review Awaiting committer review label Jul 29, 2024

kou requested review from assignUser and raulcd as code owners July 29, 2024 06:59

This was referenced Jul 29, 2024

[C++] Test linkage error when googletest 1.15.0 is installed system wide despite bundling #43400

Closed

[C++] Vendor googletest #43422

Closed

assignUser approved these changes Jul 30, 2024

View reviewed changes

assignUser merged commit a4a5562 into apache:main Jul 30, 2024

assignUser removed the awaiting committer review Awaiting committer review label Jul 30, 2024

kou deleted the cpp-gtest-homebrew branch July 30, 2024 03:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest#43465

GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest#43465
assignUser merged 2 commits intoapache:mainfrom
kou:cpp-gtest-homebrew

kou commented Jul 29, 2024 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented Jul 29, 2024

Uh oh!

kou commented Jul 29, 2024

Uh oh!

github-actions bot commented Jul 29, 2024

Uh oh!

assignUser commented Jul 30, 2024

Uh oh!

kou commented Jul 30, 2024

Uh oh!

assignUser left a comment

Uh oh!

kou commented Jul 30, 2024

Uh oh!

assignUser commented Jul 30, 2024

Uh oh!

kou commented Jul 30, 2024

Uh oh!

kou commented Jul 30, 2024

Uh oh!

conbench-apache-arrow bot commented Jul 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

kou commented Jul 29, 2024 • edited by github-actions bot 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?

Uh oh!

github-actions bot commented Jul 29, 2024

Uh oh!

kou commented Jul 29, 2024

Uh oh!

github-actions bot commented Jul 29, 2024

Uh oh!

assignUser commented Jul 30, 2024

Uh oh!

kou commented Jul 30, 2024

Uh oh!

assignUser left a comment

Choose a reason for hiding this comment

Uh oh!

kou commented Jul 30, 2024

Uh oh!

assignUser commented Jul 30, 2024

Uh oh!

kou commented Jul 30, 2024

Uh oh!

kou commented Jul 30, 2024

Uh oh!

conbench-apache-arrow bot commented Jul 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

kou commented Jul 29, 2024 •

edited by github-actions bot

Loading