Support shared library building on Windows

[traversaro]

Developers tend to have strong opinions on shared libraries, that become even stronger when they see the effort required to get shared libraries to work on Windows. However, C++ binary package managers (such as conda/pixi/conda-forge) tend to use shared libraries also on Windows, so it would be convenient to be able to compile C++ bindings of rerun as shared libraries on Windows.

This PR does add support for compiling the rerun-cpp library as a shared library on Windows. The PR is a bit invasive, so feel free to reject it if you think the maintenance effort is not worth the benefit.

Why all the complexity? In a nutshell, Linux and macOS by default export all the symbols contained in a shared library. Instead, Windows does not support that, and require the developers that want to export the symbols to either annotate every class/attribute/method that needs to be visible, or pass the attributes with a .def file. As this tend to be cumbersome, CMake has implemented the WINDOWS_EXPORT_ALL_SYMBOLS property, that emulates the "export all symbols" behavior of Linux/macOS also on Windows. However, it does not support static variables. So for libraries that do use static variables, can just use WINDOWS_EXPORT_ALL_SYMBOLS and they are good to go. However, if static variables are used (as in the case of rerun) it is necessary to annotate every static variable with the appropriate storage-class attribute. The easiest way to do so is use GenerateExportHeader module to generate a rerun_sdk_export.hpp header file that define the RERUN_SDK_EXPORT macro, and add that macro to all static variables. As RERUN_SDK_EXPORT is added in the C++ source code, we need to make sure that this macro is also defined in Linux/macOS builds, so the use of generate_export_header is present even if MSVC AND BUILD_SHARED_LIBS is OFF. This annotation process is a bit cumbersome in rerun case, as it needs to be done in all codegen logic that generate static const attributes.

What

Checklist

• I have read and agree to Contributor Guide and the Code of Conduct
• I've included a screenshot or gif (if applicable)
• I have tested the web demo (if applicable):
  • Using examples from latest main build: rerun.io/viewer
  • Using full set of examples from nightly build: rerun.io/viewer
• The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
• If applicable, add a new check to the release checklist!

• PR Build Summary
• Recent benchmark results
• Wasm size tracking

To run all checks from main, comment on the PR with @rerun-bot full-check.

[Wumpf]

TIL WINDOWS_EXPORT_ALL_SYMBOLS. This makes the dllexport/dllimport dance a lot easier to swallow.

I've been hugely defensive of any official way of shipping the c++ libraries prebuilt: We're using standard library types in our interfaces and any compiler/stdlib mismatch can cause extremely painful to debug issues at random for the users if they're not using the exact same version as whatever combination was used when building the artifacts. On top of that certain compiler settings can change the layout of standard library types, breaking any passing between a pre-compiled and freshly compiled method. This is why today our releases only contain c the rerun_c libraries and expect source code distribution for everything else.
But I figure this problem isn't as bad nowadays as it used to be since layout of these types is more and more stabilized - afaik for instance msvc hasn't done any ABI breaking changes in a while now? (I remember shipping binaries for several combinations of MSVC versions on a closed source project to mitigate these issues back in the day...)

In any case with WINDOWS_EXPORT_ALL_SYMBOLS the effort seems to be quite reasonable, just wary of users having to deal with the aforementioned issues. But I'm getting more sympathetic to the idea. Looping in @jleibs for a second opinion.

On a quick glimpse (not a proper review yet) the PR looks pretty good. rerun_sdk_export.hpp is likely trivial, we should just hand write it and check it in for less cmake magic (and more importantly to not bar non-cmake users completely!)

Also, deeply appreciate the time end effort both on the code itself and the documentation of these changes.

[traversaro]

I've been hugely defensive of any official way of shipping the c++ libraries prebuilt: We're using standard library types in our interfaces and any compiler/stdlib mismatch can cause extremely painful to debug issues at random for the users if they're not using the exact same version as whatever combination was used when building the artifacts. On top of that certain compiler settings can change the layout of standard library types, breaking any passing between a pre-compiled and freshly compiled method. This is why today our releases only contain c the rerun_c libraries and expect source code distribution for everything else.
But I figure this problem isn't as bad nowadays as it used to be since layout of these types is more and more stabilized - afaik for instance msvc hasn't done any ABI breaking changes in a while now? (I remember shipping binaries for several combinations of MSVC versions on a closed source project to mitigate these issues back in the day...)

I totally agree. I do not think that it makes sense at all for a C++ project to ship official .dll binaries decoupled by a package manager that can deal with in a systematic way with ABI mismatches (similarly to how you would not think at all of providing .so binaries for Linux). From my point of view, BUILD_SHARED_LIBS=ON support on Windows is meant to be used by package managers. I am doing this for conda-forge packaging (see conda-forge/librerun-sdk-feedstock#3) but I guess this may be useful for other package managers that support C++, such as vcpkg, conan or spack, even if several of them have still problems managing C++ libraries that depend on Rust libraries (a typical check you can do to check if that is supported is to see if the ship a recent librsvg version).

In any case with WINDOWS_EXPORT_ALL_SYMBOLS the effort seems to be quite reasonable, just wary of users having to deal with the aforementioned issues. But I'm getting more sympathetic to the idea. Looping in @jleibs for a second opinion.

One thing that comes to my mind to mitigate this is to make BUILD_SHARED_LIBS=ON + MSVC fail by default, with a big scary message on the problems of distributing .dll on Windows, and just have a dedicated option to actually enable support for BUILD_SHARED_LIBS=ON + MSVC.

On a quick glimpse (not a proper review yet) the PR looks pretty good. rerun_sdk_export.hpp is likely trivial, we should just hand write it and check it in for less cmake magic (and more importantly to not bar non-cmake users completely!)

Yes, that it make sense. I typically do not suggest users to do that as it is quite easy to write those headers wrong (see ament/ament_cmake#201, JuliaStrings/utf8proc#96, google/glog#155, ros2/rclcpp#638 (comment) for some examples of this, but I think I can write a self-contained header that should work fine in the case of rerun.

[traversaro]

On a quick glimpse (not a proper review yet) the PR looks pretty good. rerun_sdk_export.hpp is likely trivial, we should just hand write it and check it in for less cmake magic (and more importantly to not bar non-cmake users completely!)

Yes, that it make sense. I typically do not suggest users to do that as it is quite easy to write those headers wrong (see ament/ament_cmake#201, JuliaStrings/utf8proc#96, google/glog#155, ros2/rclcpp#638 (comment) for some examples of this, but I think I can write a self-contained header that should work fine in the case of rerun.

Done in 07efab7 . I did not squashed it to simplify the review, I can curate better the commits once a decision on the PR is taken.

[traversaro]

I forgot about something: in this PR I only focused on the MSVC toolchain (i.e. rust triplet x86_64-pc-windows-msvc) not on the GNU/mingw-w64 one (x86_64-pc-windows-gnu) that I guess it is not supported/tested at the moment for the rerun c++ bindings.

[traversaro]

FYI I added this here just for consistencies, since CMake 3.14 these argument have default values that should just work fine, see https://cmake.org/cmake/help/v3.16/command/install.html#installing-targets .

[traversaro]

Sorry, I see now the spell checks and similar failures, I can fix them this (european) evening.

[Wumpf]

Sorry, I see now the spell checks and similar failures, I can fix them this (european) evening.

no worries, I kicked off ci just recently. Also, there's some jobs that don't get green right now for prs from forks unfortunately

[jleibs]

This all makes sense to me. The burden doesn't seem too problematic. It's probably worth making sure we set up a nightly CI job as well.

[traversaro]

It's probably worth making sure we set up a nightly CI job as well.

Sure, do you have any pointer to where we should add the CI job? Thanks!

[Wumpf]

ci is a bit tricky to deal with I can take care of it.
Also taking this opportunity to get the from-fork ci variant green here, brace for try-and-error-ish commits ;-)

[Wumpf]

alright, let's do this. Looking great!
I'll take care of the ci and remaining cosmetics. Thank you so much again!

[Wumpf]

uh, I ended up missusing this a bit as a testing ground for various ci & setup issues. apologies. hoping this is done now 🤞

[traversaro]

Thanks a lot!