Enable building Catch2 as a dynamic library

[codeinred]

Description

This PR enables users to opt in to building Catch2 as a shared library by passing -DBUILD_SHARED_LIBS=ON when building Catch2.

Having Catch2 available as a shared library reduces build times and enables faster compilation and faster iteration of test code.

Catch2 will still be built as a static library by default.

GitHub Issues

This PR request resolves Issue #2397, Enable Users to Opt-In to Building Catch2 as a Shared Library

[codecov]

Codecov Report

Merging #2398 (3cc3171) into devel (9dc2296) will decrease coverage by 0.28%.
The diff coverage is n/a.

❗ Current head 3cc3171 differs from pull request most recent head e1a3f05. Consider uploading reports for the commit e1a3f05 to get more accurate results

@@            Coverage Diff             @@
##            devel    #2398      +/-   ##
==========================================
- Coverage   91.36%   91.08%   -0.28%     
==========================================
  Files         157      153       -4     
  Lines        7430     7288     -142     
==========================================
- Hits         6788     6638     -150     
- Misses        642      650       +8     

[horenmar]

I thought about this a bit, and I am willing to merge it if you add a CMake warning when compiling dynamic libs under Windows platform (or default visibility set to hidden, as everyone should be doing anyway 😃 ).

[codeinred]

I set the default visibility to hidden. I had to add so many instances of CATCH_DLL_PUBLIC. Please have mercy on me. It took such a long time. I dealt with so many linker errors. Megabytes of linker errors. I fixed them all.

[codeinred]

The reason it shows so many changes is because I had to merge in devel since my branch was no longer current. If you look at the change on the commandline vs the current devel branch you'll see it's not so bad. It's just hundreds and hundreds of insertions of CATCH_DLL_PUBLIC. But I did what you wanted.

[codeinred]

If you want I can update the CI matrix to have both building with and without shared libraries

[horenmar]

I set the default visibility to hidden. I had to add so many instances of CATCH_DLL_PUBLIC. Please have mercy on me. It took such a long time. I dealt with so many linker errors. Megabytes of linker errors. I fixed them all.

huh, why? I only said I wanted to add a dev warning if people built the dynamic library with hidden visibility default.

...

Oh, I see how my last post could've been misinterpreted. RIP 😆

[codeinred]

Okay so... I'm sorry for the misunderstanding. I thought you wanted the default visibility to be hidden, and I figured it had been something you were putting off because of all the annotations it required, so I thought I'd take care of it.

If the default visibility is set to hidden for all targets, and Catch2 is built as a shared library, it'll fail to compile straight-up due to the linker errors, and I think a warning won't be enough.

What if we make the visibility default for the Catch2 target if BUILD_SHARED_LIBS=ON? That way if someone tries compiling when the default visibility set to hidden, Catch2 isn't affected? Would Windows still require a warning? This should enable Catch2 to work as a shared library without all the annotations.

set_target_properties(Catch2 PROPERTIES CXX_VISIBILITY_PRESET default)
set_target_properties(Catch2WithMain PROPERTIES CXX_VISIBILITY_PRESET default)

[horenmar]

Okay so... I'm sorry for the misunderstanding.

Not like it was my time that was spent doing this 🙃

I thought you wanted the default visibility to be hidden, and I figured it had been something you were putting off because of all the annotations it required, so I thought I'd take care of it.

This wouldn't be wrong some time ago, but I've spent the last ~year getting builds of a multilibrary, old, commercial, linux-only project to work on new platforms and be done in a sane manner, and I don't think that a reasonable annotation set for Catch2 is possible. The issue boils down to the fact that Catch2's API surface is massive (I would say that more than half of its symbols are exported naturally, most of the rest are exported transitively), so almost everything would have to be annotated. Annotated exports are good if your users can use only a small subset of the symbols in the binary (e.g. we export <100 C symbols for a C++ library with 50k+ symbols compiled in, which in turn more than halves the binary size).

This applies even if the visibility for tests is handled differently -- trying to link tests against dyn libraries is a common way to have massively larger export set than actually needed by the users.

If the default visibility is set to hidden for all targets, and Catch2 is built as a shared library, it'll fail to compile straight-up due to the linker errors, and I think a warning won't be enough.

As you say, if the visibility is hidden, the linking will fail. The reason to still have a warning is twofold:

• Have a nice user message about the issue, before hundreds of lines of linker error spew happen.
• Provide the information that there is an issue even before the linking attempt happens. If you build your deps separately from your own project (the big dependency managers, vcpkg and conan, both do this), the warning happens a lot earlier than the hard error.

There is an argument to make it a hard error, but then we would also need to provide a way to override this for users who "know better" for some reason (e.g. they have explicit export files).

What if we make the visibility default for the Catch2 target if BUILD_SHARED_LIBS=ON? That way if someone tries compiling when the default visibility set to hidden, Catch2 isn't affected? Would Windows still require a warning? This should enable Catch2 to work as a shared library without all the annotations.

I am not sure what would happen on Linux if the user also added -fvisibility=hidden to the compile flags via e.g. triplet file, but I suspect that the symbols would end up hidden anyway. Also this fundamentally doesn't work on Windows. CMake tries to provide a workaround via CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS, but it has multiple failure modes in various cases, so it doesn't work for all projects (I haven't actually checked if it would work with Catch2).

[horenmar]

@codeinred Are you planning to finish the PR?

[codeinred]

I am, yes!! I'm so sorry it's taken so long. I've been dealing with finals for school

[friendlyanon]

The thing about symbol visibility is that Windows does the right thing. On Windows, you have no options but to have everything hidden, unless annotated otherwise. Unfortunately for UNIX-like systems, legacy dictates different defaults and they can only recommend doing the right thing.

For C++ targets, you need to set two properties to effectively pass the -fvisibility=hidden and -fvisibility-inlines-hidden flags to compilers that CMake knows understand them. Everywhere else CMake does nothing, since the compilers do the right thing anyway.

CMake tries to provide a workaround via CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS, but it has multiple failure modes in various cases, so it doesn't work for all projects (I haven't actually checked if it would work with Catch2).

There are some issues indeed, since this is a workaround for a UNIX-ism. For the record, CMake parses the PE/COFF object files and dumps the COFF (or bigobj) symbol table from an OBJ file using filters that assume MSVC ABI (and its name mangling).

[stefanhaller]

@horenmar The CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS approach has the disadvantage that it only works with CMake. Our build system doesn't use CMake, and I have spend a considerable amount of time trying to do what CMake does, manually, with little success.

I have come to the conclusion that for us the only sane way to build Catch2 as a DLL is to annotate the source code. Would you consider accepting a PR that does this, if it is clean and done right? If not, we'll have to maintain a custom patch on our side, which is also an option. (It will probably break every time we upgrade to a newer version of Catch, but it shouldn't be hard to fix.)

[friendlyanon]

I have spend a considerable amount of time trying to do what CMake does

The feature is implemented entirely in this file: https://github.com/Kitware/CMake/blob/master/Source/bindexplib.cxx
It's also available as the __create_def tool, so you can just invoke CMake solely for this. CMake generated build files in fact do just that.

[stefanhaller]

Yes, I know, thanks @friendlyanon. I did try to use cmake -E __create_def. The problem is that our build system doesn't support pre-link steps, so on-the-fly creation of a .def file while building isn't possible for us. I had to create a .def file and commit it to our repo, but this doesn't work well because apparently even minor differences in compiler versions result in different inlining behavior, resulting in linker errors.

[horenmar]

@stefanhaller Do you have a strong reason not to use Catch2 as a static library?

Anyway, I would be willing to accept a patch that annotates the sources if it comes with good CI design. The current test projects are not suitable to testing proper annotations, because SelfTest (intentionally) tests functions that should not be exported to users, while the various examples do not even nearly cover the functionality that is supposed to be exported to the users.

[stefanhaller]

@stefanhaller Do you have a strong reason not to use Catch2 as a static library?

Yes, we have an architecture where Catch2 is used both from the executable and from one or more DLLs that the executable depends on. If we build as a static library, each of these get their own copy of Catch2's singletons (mostly IMutableContext::currentContext, I think).

Anyway, I would be willing to accept a patch that annotates the sources if it comes with good CI design. The current test projects are not suitable to testing proper annotations, because SelfTest (intentionally) tests functions that should not be exported to users, while the various examples do not even nearly cover the functionality that is supposed to be exported to the users.

This seems to make the approach infeasible (like you said earlier 😄), so for now I'm going with maintaining my own local patch. Thanks.