Systematically add -DCMAKE_VERBOSE_MAKEFILE=ON in ci.yml

[rwgk]

Description

@henryiii @Skylion007 I didn't really expect the added -DCMAKE_VERBOSE_MAKEFILE=ON to work everywhere, but it does!

The size difference in logs_*.zip is:

$ ls -l logs_25759.zip logs_25892.zip
3.9M Dec 12 02:47 logs_25759.zip
5.2M Dec 12 11:05 logs_25892.zip

I'm thinking, relative to all the binaries getting generated, that's probably a tiny increase, but it makes the difference between day a night for questions like we want to answer here. E.g. I see that -Wodr isn't actually specified explicitly on any command line, and I can now systematically mine the logs for clues what linker options (-O3, -flto?) trigger the ODR warnings.

I.e. I think it'll be great to merge this PR.

---

To obtain full command lines related to -Wodr.

#4395 (comment)

https://github.com/pybind/pybind11/actions/runs/3659994679/jobs/6186623695

Suggested changelog entry:

[rwgk]

The ODR warnings (link in PR description) seem to originate from

eigen3/unsupported/Eigen/CXX11/src/Tensor/TensorStorage.h, which was brought in via PR #4201.

@lalaland have you seen those before?

A quick Google search for "Eigen TensorStorage.h Wodr" doesn't pull up anything:

https://www.google.com/search?sxsrf=ALiCzsbf1_qwrjfCNnNTRDHsSFW41iRVjQ:1670844029730&q=Eigen+TensorStorage.h+%22Wodr%22&sa=X&ved=2ahUKEwj1yPeG-_P7AhUFmeAKHTarBKsQ5t4CegQIKBAB&biw=2099&bih=1042&dpr=2

Another thing I don't understand: I see -DPYBIND11_WERROR=ON in log files that have the warning, but the -Wodr warnings are not treated as errors anyway.

I created this PR to find out what's going on.

[EthanSteinberg]

My initial guess is that it's due to our weird unit tests (where we have eigen with and without std::array).

Some of those symbols must be accidentally being made public, causing odr violations.

Easiest test would be to disable one of the two (disable the no std::array) test

[rwgk]

My initial guess is that it's due to our weird unit tests (where we have eigen with and without std::array).

Oh, of course, I believe it immediately!
Two definitions, in test_eigen_tensor.cpp & test_eigen_tensor_avoid_stl_array.cpp, but linked together into pybind11_tests.so, ouch. I completely missed that before, but it's very obvious in hindsight.

(I'm intrigued now btw that some compilers/linkers discover that. Before I implemented the type_caster_odr_guard I looked around pretty extensively, hoping to find an existing tool that does it.)

Some of those symbols must be accidentally being made public, causing odr violations.

Everything linked together into pybind11_tests.so has mutual visibility. The only mechanism for hiding symbols from each other inside the same .so is to put them into different namespaces, incl. the unnamed namespace. (Or maybe also C-style static entities, not sure, but I don't think that's relevant here.)

Easiest test would be to disable one of the two (disable the no std::array) test

Another option is to put test_eigen_tensor_avoid_stl_array into a separate extension, then the hiding will work. The only problem is that tests/CMakeLists.txt will become a little more complicated, but there are already similar cases:

• master:

  pybind11/tests/CMakeLists.txt

  Lines 219 to 221 in 9727dcd

  	# And add additional targets for other tests.
  	tests_extra_targets("test_exceptions.py" "cross_module_interleaved_error_already_set")
  	tests_extra_targets("test_gil_scoped.py" "cross_module_gil_utils")

• smart_holder:

  pybind11/tests/CMakeLists.txt

  Lines 243 to 246 in 53b80b4

  	tests_extra_targets("test_class_sh_module_local.py"
  	"class_sh_module_local_0;class_sh_module_local_1;class_sh_module_local_2")
  	tests_extra_targets("test_exc_namespace_visibility.py"
  	"namespace_visibility_1;namespace_visibility_2")

Luckily PR #3590 by @iwanders has made this pretty easy, thanks again!

@lalaland is there a chance you could help with this?

I think the end-result will be a double win:

• We're resolving the existing ODR.
• If we work on the visibility handling in the future (e.g. Remove cmake PROPERTIES CXX_VISIBILITY_PRESET "hidden" #4072) we know some compilers will tell us if we make a mistake, by generating the ODR warnings for TensorStorage.h again.

[EthanSteinberg]

@lalaland is there a chance you could help with this?

Yeah, I introduced the bug, so I should probably introduce a fix. I'll be able to work on it this weekend.

[rwgk]

Thanks Aaron!

[henryiii]

Instead of changing every single invocation and making an already long file longer, why not just set env: VERBOSE: 1 at the top? It's been supported since CMake 3.14: https://cmake.org/cmake/help/latest/envvar/VERBOSE.html

[rwgk]

Instead of changing every single invocation and making an already long file longer, why not just set env: VERBOSE: 1 at the top? It's been supported since CMake 3.14: https://cmake.org/cmake/help/latest/envvar/VERBOSE.html

Oh, nice! I didn't know. I'll try that out.