[Question] Consider making BENCHMARK_STATIC_DEFINE a public compile definition

[jcar87]

Describe the bug Linking a consumer project against the generated CMake imported targets fails on Windows/MSVC due to missing symbols.

System Which OS, compiler, and compiler version are you using:

• OS: Windows
• Compiler and version: Visual C++

To reproduce Issue very well described here: https://github.com/google/benchmark/issues/1450 and also happening here: https://github.com/conan-io/conan-center-index/pull/11912

External projects that link against the generated CMake imported targets, now need to define BENCHMARK_STATIC_DEFINE on their end. I think this effectively means that users on Windows would have to define that on their side when updating to 1.7.0.

This gives me the impression that this could be a "public" compile definition that is propagated transitively to consumers, such that they do not have to define this on their end. I believe introduced here: https://github.com/google/benchmark/pull/1435/files.

I would venture a guess that prior to this change, the value was already set in the generated file.

I'm happy to open a pull request to address it, just want to make sure it would not negatively impact other platforms.

[dmah42]

i expected that to be solved by the change to src/CMakeLists.txt that explicitly sets that define if cmake determines that we're building a static library: https://github.com/google/benchmark/blob/main/src/CMakeLists.txt#L58

on bazel we always define this as only static linking is (currently) supported.

i did notice this needed to be set on integration with another project on which i work but it didn't occur to me that this was anything but an unusual integration.

i'm happy for a PR that solves it.. it's just not obvious to me why the current configuration is wrong.

oh! unless it should be explicitly defined to 0 in the case of a shared library?

[scamille]

The problem with the target compile definition in https://github.com/google/benchmark/blob/main/src/CMakeLists.txt#L58 is that it is PRIVATE and not PUBLIC.

Thus when googlebenchmark as a lib is built, the define is correctly present, and no __declspec(dllimport) are added when compiling it as a static library.

In the exported cmake library, the compile time definition isn't automatically propagated. Thus, when a consumer includes benchmark/export.h (indirectly through including benchmark/benchmark.h), the define isn't set, and the generated function declarations are generated with the __declspec(dllimport) prefix.

MSVC then complains with LNK4217 about the mismatch.

Since I just recently stumbled upon this problem as well when trying to integrate version 1.7.0, I would look forward to getting this resolved. It might be as simple as changing the quoted line of code to

target_compile_definitions(benchmark PUBLIC -DBENCHMARK_STATIC_DEFINE)

Edit: I tested it with the suggested change to make the target_compile_definition for BENCHMARK_STATIC_DEFINE PUBLIC instead of PRIVATE, and cmake is correctly exporting it to the created target and targets depending on it get can now build without the mentioned linker error.