Resolve Clang compilation issue with benchmark 1.5.3

[Seiwert]

Would like to upgrade to latest version of Google Benchmark 1.6.1 to resolve this issue that shows up when generating release builds:

Cloning into 'benchmark-1.6.1'...
HEAD is now at c05843a [sysinfo] Fix CPU Frequency reading on AMD Ryzen CPU's (#1117)
/home/adam/Documents/r2/build/release/google_benchmark_vendor/benchmark-1.6.1-prefix/src/benchmark-1.6.1/src/complexity.cc:85:10: error: variable 'sigma_gn' set but not used [-Werror,-Wunused-but-set-variable]
  double sigma_gn = 0.0;
         ^
1 error generated.
gmake[5]: *** [src/CMakeFiles/benchmark.dir/build.make:174: src/CMakeFiles/benchmark.dir/complexity.cc.o] Error 1
gmake[5]: *** Waiting for unfinished jobs....
gmake[4]: *** [CMakeFiles/Makefile2:100: src/CMakeFiles/benchmark.dir/all] Error 2
gmake[3]: *** [Makefile:136: all] Error 2
gmake[2]: *** [CMakeFiles/benchmark-1.6.1.dir/build.make:86: benchmark-1.6.1-prefix/src/benchmark-1.6.1-stamp/benchmark-1.6.1-build] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/benchmark-1.6.1.dir/all] Error 2
gmake: *** [Makefile:136: all] Error 2
---
Failed   <<< google_benchmark_vendor [10.7s, exited with code 2]

Summary: 0 packages finished [11.3s]
  1 package failed: google_benchmark_vendor
  1 package had stderr output: google_benchmark_vendor

I've confirmed this is resolved with the latest 1.6.1 version but the older version referenced in the cmake does contain this erroneous variable.

[clalancette]

Thanks for the improvement! I've got a few questions below.

First, can you please give us an idea of which compiler is giving you this warning? I don't seem to see it when we are building Google benchmark on Ubuntu 22.04 (g++ 11.3.0). I just want to know how to reproduce the warning.

Second, we generally prefer that the version we are vendoring here matches the version of the package that is shipped in Ubuntu 22.04. In this case, we are already vendoring 1.6.1, and that is what is in Ubuntu 22.04. So instead of updating the hash, I think instead we should instead add in a patch file which has just the change you are interested in.

Finally, can you please target this to the rolling branch instead of humble? We'll get it in there first and then backport from there.

[Seiwert]

Hi @clalancette , thanks for reviewing!

1. This error is produced when utilizing clang 14.0.0 (seemingly anything newer than clang 12 should see the problem) on Ubuntu 22.04. I did a short test with a fresh humble pull and you can quickly replicate with these build arguments:

colcon build --cmake-args -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER=clang++ --packages-select google_benchmark_vendor

2. That is a fair point. I did some more digging into the issue and found the exact commit addressing the problem in benchmark. Seems like it would be straight forward enough to apply this with a patch file if you still prefer this method.
3. Will do.

[clalancette]

colcon build --cmake-args -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER=clang++ --packages-select google_benchmark_vendor

Hm, that's weird. I cannot reproduce that on Rolling here. Oh, but I can reproduce it on Humble.

And in turn, that is because there is a bug in the packaging for this on Humble. It claims to be versioning 1.6.1, but it is actually versioning 1.5.3 (this is due to 2 PRs in flight at the same time, where one of them undid what the other one did). This is fixed on Rolling, but not on Humble.

So I'm sorry, but my advice here has changed. What I think we should do here instead is to retarget this to humble, but update the CMakeLists.txt to (correctly) show that we are vendoring 1.5.3 in Humble. Then we can add in the patch to fix the particular issue you are talking about. That will at least make it so we can more easily understand things in the future. And there should then be no need to touch rolling at all, since it already seems to build fine. Does that make sense?

[Seiwert]

That makes sense and I believe I have made all the requested changes correctly. The only uncertainty I had was regarding the precise name for the new patch file. I kept it as the name of the commit it was generated from for traceability purposes. However, if you think it should be renamed or if there are any other additional changes needed, please let me know.

[clalancette]

This looks good to me, thanks for iterating. I'll run CI on it next.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows