Skip to content
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.

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

Update google-benchmark to 1.7.0 #3151

Merged
merged 6 commits into from
Oct 24, 2022

Conversation

AreaZR
Copy link
Contributor

@AreaZR AreaZR commented Oct 11, 2022

Additionally makes benchmarks separate from the STL build, since we keep having problems when google benchmark is built in the STL build.

@AreaZR AreaZR requested a review from a team as a code owner October 11, 2022 18:00
@CaseyCarter CaseyCarter added the test Related to test code label Oct 11, 2022
@StephanTLavavej
Copy link
Member

@strega-nil-ms will investigate why the build is failing.

@strega-nil-ms
Copy link
Contributor

strega-nil-ms commented Oct 13, 2022

It looks like there is a bug in google benchmark that means we can't update for now:

std_copy.cpp.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __cdecl benchmark::internal::FunctionBenchmark::FunctionBenchmark(char const *,void (__cdecl*)(class benchmark::State &))" (__imp_??0FunctionBenchmark@internal@benchmark@@QEAA@PEBDP6AXAEAVState@2@@Z@Z) referenced in function "void __cdecl `dynamic initializer for 'benchmark_uniq_10std_copy_call''(void)" (??__Ebenchmark_uniq_10std_copy_call@@YAXXZ)
std_copy.cpp.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: virtual __cdecl benchmark::internal::FunctionBenchmark::~FunctionBenchmark(void)" (__imp_??1FunctionBenchmark@internal@benchmark@@UEAA@XZ) referenced in function "public: virtual void * __cdecl benchmark::internal::FunctionBenchmark::`scalar deleting destructor'(unsigned int)" (??_GFunctionBenchmark@internal@benchmark@@UEAAPEAXI@Z)
benchmark-std_copy.exe : fatal error LNK1120: 2 unresolved externals

Edit: See google/benchmark#1450; there is a workaround.

@barcharcraz
Copy link
Member

barcharcraz commented Oct 15, 2022

I'm not 100% convinced separating out the benchmark build is an improvement. Our main build is a bit of an oddball though.

@strega-nil-ms
Copy link
Contributor

strega-nil-ms commented Oct 18, 2022

@barcharcraz there were multiple problems caused by having google benchmark as part of the main build (like, in google benchmark's build system); splitting it out removed those problems. I would prefer not to if there weren't those issues, but I also don't think creating workarounds is worth making the benchmark part of the main build.

@StephanTLavavej StephanTLavavej added the infrastructure Related to repository automation label Oct 19, 2022
benchmarks/CMakeLists.txt Outdated Show resolved Hide resolved
azure-devops/cross-build.yml Show resolved Hide resolved
azure-devops/cmake-configure-build.yml Outdated Show resolved Hide resolved
benchmarks/CMakeLists.txt Outdated Show resolved Hide resolved
benchmarks/CMakeLists.txt Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Oct 21, 2022
@StephanTLavavej
Copy link
Member

I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit e07c062 into microsoft:main Oct 24, 2022
@StephanTLavavej
Copy link
Member

Thanks @AtariDreams for updating this dependency and @strega-nil-ms for reworking the build system! ✅ 🚀 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Related to repository automation test Related to test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants