Skip to content

MINOR: [C++] Use bundled gtest #13063

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.

Sign up for GitHub

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

Closed
assignUser wants to merge 5 commits into from
Closed

MINOR: [C++] Use bundled gtest #13063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
39bb98d
use bundled gtest
assignUser May 4, 2022
a29f8ab
fix typo
assignUser May 4, 2022
f74d85e
fix typo
assignUser May 4, 2022
c56853e
use system flag
assignUser May 4, 2022
10e6832
add step to download tz data
assignUser May 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dev/tasks/vcpkg-tests/cpp-build-vcpkg.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ cmake -G "Visual Studio 16 2019" -A x64 ^
-DARROW_WITH_SNAPPY=ON ^
-DARROW_WITH_ZLIB=ON ^
-DARROW_WITH_ZSTD=ON ^
-DGTest_SOURCE=SYSTEM ^
Copy link
Member

@pitrou pitrou May 4, 2022

Choose a reason for hiding this comment

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

Hmm, how does this work on Windows? "system" would mean system-provided packages on Linux, but I don't think those exist at all on Windows.

Copy link
Member

@pitrou pitrou May 4, 2022

Choose a reason for hiding this comment

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

cc @kou for advice.

Copy link
Member Author

@assignUser assignUser May 4, 2022

Choose a reason for hiding this comment

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

It looks like it worked (vcpkg is still installing 1.10 but the error is gone)? It now fails due to missing tzdata, which should be fixable via the download script

Copy link
Member

@kou kou May 4, 2022

Choose a reason for hiding this comment

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

We should not do this. We should fix https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L80-L86 instead.

(This is not a MINOR task.)

Copy link
Member Author

@assignUser assignUser May 5, 2022

Choose a reason for hiding this comment

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

Ok I'll close this PR and I think this might be an appropriate JIRA to use? I would gladly take your suggestion on how to fix this @kou , should this test only be used in case we also use conda? As it seems to work with vcpkg,

Copy link
Member

@kou kou May 5, 2022

Choose a reason for hiding this comment

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

It seems that ARROW-14560 is another problem and it has been fixed in upstream.

Could you open a new JIRA issue for it?

How about just removing https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L80-L86 ? The latest may solve the problem described in the comment.

If the problem isn't solved yet, we should report it to the conda's GTest package. We can use if(ARROW_DEPENDENCY_SOURCE STREQUAL "CONDA" AND MSVC AND "${GTest_SOURCE}" STREQUAL "") condition as a workaround for this case.

-DCMAKE_BUILD_TYPE=release ^
-DCMAKE_UNITY_BUILD=ON ^
.. || exit /B 1
Expand Down
3 changes: 3 additions & 0 deletions dev/tasks/vcpkg-tests/github.windows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ jobs:
git -C arrow fetch -t {{ arrow.remote }} {{ arrow.branch }}
git -C arrow checkout FETCH_HEAD
git -C arrow submodule update --init --recursive
- name: Download Timezone Database
shell: bash
run: arrow/ci/scripts/download_tz_database.sh
- name: Remove and Reinstall vcpkg
# When running vcpkg in Github Actions on Windows, remove the
# preinstalled vcpkg and install the newest version from source.
Expand Down