Stop generating the export header and just check it in by dmah42 · Pull Request #1435 · google/benchmark

dmah42 · 2022-07-19T17:04:16Z

This also removes the dual licensing requirement.

As far as i can tell there is no behavioural change here so I'm not sure why it needed to be generated in the first place...

dmah42 · 2022-07-20T12:31:33Z

@oontvoo i think this solves the problem you were trying to solve with not generating the export header.

oontvoo · 2022-07-20T14:33:29Z

Thanks! The windows CI failed? (not clear to me why it's failing tho - the logs dont have much info)

dmah42 · 2022-07-20T14:34:59Z

Thanks! The windows CI failed? (not clear to me why it's failing tho - the logs dont have much info)

the windows release CI is quite flaky... i don't have a windows machine to test on but i think it's due to the benchmark running at a very different speed to what's expected and the regex test fails.

LebedevRI · 2022-07-20T17:22:29Z

 endif()

+if (NOT BUILD_SHARED_LIBS)
+  add_definitions(-DBENCHMARK_STATIC_DEFINE)


not sure about this one as there's multiple targets all of which need the definition.

LebedevRI

No particular opinion.
If CMake was the only build system, this would probably not make sense,
but since a workaround for non-CMake is needed, i suppose this is better.

sergiud · 2022-07-22T09:05:41Z

[...] I'm not sure why it needed to be generated in the first place...

The reason is separation of concerns. In particular, it does not make sense to check-in artifacts that can be generated. Additionally, CMake users can profit from improvements to GenerateExportHeader without relying on benchmark updates.

Now there's an automatically generated header file which is part of the code base that will either never be updated or will be patched manually. This approach does not sound very convincing.

dmah42 · 2022-07-22T11:47:26Z

we don't only support cmake so extra effort was required for other build systems, and critically this broke some important integrations
it's not automatically generated and shouldn't need to change. if you go through the contents it's a pretty straightforward bit of logic ensuring that the right attributes are set for the right builds. i'll be very surprised if this needs to change outside of specific platform/compiler support.

sergiud · 2022-07-29T12:43:50Z

If there are Bazel issues with the export header, I don't see why changing the CMake export is necessary. This PR introduced not only unnecessary complexity and copy-pasted code but also seemingly broke the build in some configurations. See [BUG] Failed build on VS #1450, [BUG] #1435 breaks automated builds on Windows #1441, [BUG] 1.6.2 breaks: fatal error: 'benchmark/export.h' file not found #1436. Why not provide a dedicated export header for Bazel builds only instead?
I disagree: GenerateExportHeader output does not have to be static, e.g., because GNU style attributes could be replaced by C++11 style attributes depending on the compiler's capabilities. There is no good reason why benchmark should decide what functionality outside of library's domain is best for its users.

dmah42 · 2022-07-29T13:59:21Z

i changed it because it broke other configurations that couldn't handle the generation of the header.
so cmake should make that decision instead? i don't understand your point about deciding functionality: this is about ensuring the symbols are available in the library in different build configurations, not about introducing functionality outside its domain.

sergiud · 2022-07-29T14:04:27Z

I don't understand. What configurations did not work in CMake?
Of course, this is what's a build system is for: to provide abstractions to common functionality.

dmah42 · 2022-07-29T14:10:07Z

not CMake ones.

if we want to bring back generating the header then it needs to work across all the systems we need to support. unfortunately some of these systems are not available to be tested by CI bots and only show up on integration.

i think that means we need to have a static header for all but cmake, which means we may as well also use that static header for cmake to avoid maintaining two systems, one of which might change under our noses if cmake decides to change their implementation.

…e#1435)" This reverts commit 7b3ac07. Revert "Fix SOVERSION of shared library" This reverts commit d4bc509. Revert "Also fix the SOVERSION for benchmark_main" This reverts commit d845b7b. Revert "use target_compile_definitions (google#1440)" This reverts commit e27c930. Revert "Remove stray comment and added missing header (google#1444)" This reverts commit 141b554. Revert "remove unnecessary generated include directory (google#1451)" This reverts commit 7d48eff.

…e#1435)" This reverts commit 7b3ac07. Revert "Fix SOVERSION of shared library" This reverts commit d4bc509. Revert "Also fix the SOVERSION for benchmark_main" This reverts commit d845b7b. Revert "Remove stray comment and added missing header (google#1444)" This reverts commit 141b554. Revert "remove unnecessary generated include directory (google#1451)" This reverts commit 7d48eff.

bstordrup · 2024-01-03T14:33:53Z

I had failing build on a project in Visual Studio 2022 where I used a build of a fork of benchmark (latest version).

As soon as I reverted this pull request, rebuild and deployed benchmark off my local benchmark repository, the previously failing build started to succeed.

Seems like generating the export.h file is vital for the link process in Visual Studio.

bstordrup · 2024-01-03T15:23:15Z

I got my failing build working in a different approach, but it required a change in the steps to build the benchmark library off my local fork.

I reverted all changes, and then I build the library using the following steps:

cmake -E chdir "build" cmake -DBENCHMARK_DOWNLOAD_DEPENDENCIES=on -DBUILD_SHARED_LIBS=on -DCMAKE_BUILD_TYPE=Release ../
cmake --build "build" --config Release
cmake --build "build" --config Release --target install

Using the -DBUILD_SHARED_LIBS=on is required to get the export.h functioning correct when it comes to the BENCHMARK_EXPORT define when you want to use it as a shared lib.

dmah42 added 8 commits July 19, 2022 17:58

Stop generating the export header and just check it in

da93f9d

format the new header

895f99a

support windows

914d9c5

format the header again

155d05b

avoid depending on internal macro

50d2af2

ensure we define the right thing for windows static builds

d323b09

support older cmake

f331ac0

and for tests

556e2a7

dmah42 requested a review from LebedevRI July 20, 2022 12:31

LebedevRI reviewed Jul 20, 2022

View reviewed changes

Comment thread src/CMakeLists.txt

LebedevRI reviewed Jul 20, 2022

View reviewed changes

dmah42 merged commit 7b3ac07 into main Jul 20, 2022

dmah42 deleted the export branch July 21, 2022 10:50

DanielLiamAnderson mentioned this pull request Jul 21, 2022

[BUG] #1435 breaks automated builds on Windows #1441

Open

Conversation

dmah42 commented Jul 19, 2022

Uh oh!

dmah42 commented Jul 20, 2022

Uh oh!

oontvoo commented Jul 20, 2022

Uh oh!

dmah42 commented Jul 20, 2022

Uh oh!

Uh oh!

LebedevRI Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

dmah42 Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

LebedevRI left a comment

Choose a reason for hiding this comment

Uh oh!

sergiud commented Jul 22, 2022

Uh oh!

dmah42 commented Jul 22, 2022

Uh oh!

sergiud commented Jul 29, 2022

Uh oh!

dmah42 commented Jul 29, 2022

Uh oh!

sergiud commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmah42 commented Jul 29, 2022

Uh oh!

bstordrup commented Jan 3, 2024

Uh oh!

bstordrup commented Jan 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sergiud commented Jul 29, 2022 •

edited

Loading