Skip to content

Conversation

@kszucs
Copy link
Member

@kszucs kszucs commented Oct 20, 2021

Pin budled gtest to 1.10.0 as required by the thirdparty toolchain: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L1814

vcpkg crossbow build which was failing with the same reason as the source release verification script: https://github.com/ursacomputing/crossbow/runs/3951554360

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@kszucs
Copy link
Member Author

kszucs commented Oct 20, 2021

It fixes the earlier failing vcpkg build and should fix the windows verification build as well because we install gtest=1.10 using conda.

@kszucs kszucs requested a review from lidavidm October 20, 2021 14:22
@kszucs kszucs closed this in 4ac62d5 Oct 20, 2021
@ursabot
Copy link

ursabot commented Oct 20, 2021

Benchmark runs are scheduled for baseline = 80ecf33 and contender = 4ac62d5. 4ac62d5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.27% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

bkmgit added a commit to bkmgit/arrow that referenced this pull request Oct 21, 2021
address review apache#11477
@pitrou
Copy link
Member

pitrou commented Nov 1, 2021

Why was this done? If you're using the conda package, you shouldn't use the bundled version, and vice-versa.

@lidavidm
Copy link
Member

lidavidm commented Nov 1, 2021

IIRC the issue was: we required GTest 1.11, but still installed 1.10 via Conda, and in the end we would use the headers from 1.10 but link to 1.11, because the -I directive for the Conda prefix's include dir came first. There's a longer discussion on Zulip.

@pitrou
Copy link
Member

pitrou commented Nov 1, 2021

ARROW_GTEST_BUILD_VERSION is not the required version, it's only the version used when building a bundled version. It shouldn't conflict with the conda-installed version.

@pitrou
Copy link
Member

pitrou commented Nov 1, 2021

However, it's an error to both install it with Conda and ask CMake to build a bundled version. I have no clue where this happens, because the release script is a mess.

@lidavidm
Copy link
Member

lidavidm commented Nov 1, 2021

Ah, sorry, I misunderstood then - but regardless, the failing builds were building GTest from source, so we still had the version mismatch.

The Zulip discussion starts here sorry, here is the full thread

The better fix would probably have been to prioritize the bundled include directory over the Conda one.

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.

4 participants