You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2024/07/29 06:59:15 UTC
[PR] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest [arrow]
kou opened a new pull request, #43465:
URL: https://github.com/apache/arrow/pull/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.
--
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
Re: [PR] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #43465:
URL: https://github.com/apache/arrow/pull/43465#issuecomment-2257323672
> 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.
--
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
Re: [PR] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #43465:
URL: https://github.com/apache/arrow/pull/43465#issuecomment-2257315721
Ah, are you suggesting the existing `brew uninstall googletest` approach?
--
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
Re: [PR] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #43465:
URL: https://github.com/apache/arrow/pull/43465#issuecomment-2258084313
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit a4a5562e6b5a82ff297faeac45eb2ce1f391bfec.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/28099117189) has more details. It also includes information about 19 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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
Re: [PR] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #43465:
URL: https://github.com/apache/arrow/pull/43465#issuecomment-2257310458
> 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?
--
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
Re: [PR] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #43465:
URL: https://github.com/apache/arrow/pull/43465#issuecomment-2257322343
@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?
--
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
Re: [PR] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #43465:
URL: https://github.com/apache/arrow/pull/43465#issuecomment-2257302243
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...
--
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
Re: [PR] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #43465:
URL: https://github.com/apache/arrow/pull/43465#issuecomment-2257324472
> Ah, are you suggesting the existing `brew uninstall googletest` approach?
Sorry. I wrote this comment before I saw https://github.com/apache/arrow/pull/43465#pullrequestreview-2206426207 .
--
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
Re: [PR] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43465:
URL: https://github.com/apache/arrow/pull/43465#issuecomment-2255094789
Revision: 8aef1702bc0822702abee0f036ca8d06de4afee8
Submitted crossbow builds: [ursacomputing/crossbow @ actions-6a37a18c2b](https://github.com/ursacomputing/crossbow/branches/all?query=actions-6a37a18c2b)
|Task|Status|
|----|------|
|java-jars|[](https://github.com/ursacomputing/crossbow/actions/runs/10139624713/job/28033269083)|
--
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
Re: [PR] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #43465:
URL: https://github.com/apache/arrow/pull/43465#issuecomment-2255091248
@github-actions crossbow submit java-jars
--
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
Re: [PR] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser merged PR #43465:
URL: https://github.com/apache/arrow/pull/43465
--
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
Re: [PR] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43465:
URL: https://github.com/apache/arrow/pull/43465#issuecomment-2255090543
:warning: GitHub issue #43400 **has been automatically assigned in GitHub** to PR creator.
--
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