Add support for compilation and test on Windows

[traversaro]

I tested this on Visual Studio 2022.

The modifications are mainly three:

• 1. Use WINDOWS_EXPORT_ALL_SYMBOLS to export automatically all defined functions. This ensures that rsl.lib is actually created, before this change only the rsl.dll library was created resulting in downstream errors such as the one reported in Windows Failure February 2024 RoboStack/ros-humble#136 (comment)
• 2. Set CMAKE_RUNTIME_OUTPUT_DIRECTORY so all the compiled .dll and .exe are placed in the same folder, ensuring that the test-rsl.exe can find Catch2.dll , Catch2Main.dll and rsl.dll
• 3. I excluded try.cpp files from the tests on Windows as they were not compiling on Windows
• 4. I modified the test in static_string.cpp to avoid requiring an implicit conversion from an iterator to a pointer

[ChrisThrasher]

What’s stopping the static string tests from compiling on windows?

[traversaro]

What’s stopping the static string tests from compiling on windows?

Sorry, I did not look into that originally, I checked it now.

It seems that the line auto const* begin = static_string.begin(); in

RSL/tests/static_string.cpp

Line 25 in cebc67c

auto const* begin = static_string.begin();

results in:

C:\Users\straversaro\RSL\tests\static_string.cpp(25): error C3535: cannot deduce type for 'const auto *' from 'std::_Array_const_iterator<_Ty,14>'
        with
        [
            _Ty=char
        ]
C:\Users\straversaro\RSL\tests\static_string.cpp(25): error C2440: 'initializing': cannot convert from 'std::_Array_const_iterator<_Ty,14>' to 'const int *'
        with
        [
            _Ty=char
        ]
C:\Users\straversaro\RSL\tests\static_string.cpp(25): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
ninja: build stopped: subcommand failed.

To be honest, I do not know if that test wanted to test implicit conversions from a iterator to a pointer, so I am not sure how it should be fixed.

[ChrisThrasher]

I recognize this problem. I think it can be fixed by replacing auto* with something like std::array::iterator. I’m away from my computer so I can’t test it out at the moment but we can eventually fix that.

[traversaro]

I recognize this problem. I think it can be fixed by replacing auto* with something like std::array::iterator. I’m away from my computer so I can’t test it out at the moment but we can eventually fix that.

I think if there is no need/intention to test implicit conversion to pointer in the test, just changing auto* to auto should work, let me try.

[traversaro]

Changing the line to auto begin = static_string.begin(); everything is working, let me update the PR.

[traversaro]

I updated and rebase the PR in c9331a2, and updated the original description.

[ChrisThrasher]

I didn’t propose that because I believe it will cause clang-tidy to fail

[traversaro]

I didn’t propose that because I believe it will cause clang-tidy to fail

Ah, I had no idea. I will try, if clang-tidy complains I will just use the explicit iterator.

[traversaro]

clang-tidy error confirmed:

/__w/RSL/RSL/tests/static_string.cpp:25:13: error: 'auto begin' can be declared as 'const auto *begin' [readability-qualified-auto,-warnings-as-errors]
            auto begin = static_string.begin();
            ^~~~~
            const auto *

[traversaro]

clang-tidy error confirmed:

/__w/RSL/RSL/tests/static_string.cpp:25:13: error: 'auto begin' can be declared as 'const auto *begin' [readability-qualified-auto,-warnings-as-errors]
            auto begin = static_string.begin();
            ^~~~~
            const auto *

Sorry, I forgot to reply. In the end I got rid of the auto and directly used the iterator.

[ChrisThrasher]

clang-tidy error confirmed:

/__w/RSL/RSL/tests/static_string.cpp:25:13: error: 'auto begin' can be declared as 'const auto *begin' [readability-qualified-auto,-warnings-as-errors]
            auto begin = static_string.begin();
            ^~~~~
            const auto *

Sorry, I forgot to reply. In the end I got rid of the auto and directly used the iterator.

Can you submit a PR that makes this change in isolation? This PR is currently doing a few things at once and I'd rather tackle one problem at a time.

[traversaro]

clang-tidy error confirmed:

/__w/RSL/RSL/tests/static_string.cpp:25:13: error: 'auto begin' can be declared as 'const auto *begin' [readability-qualified-auto,-warnings-as-errors]
            auto begin = static_string.begin();
            ^~~~~
            const auto *

Sorry, I forgot to reply. In the end I got rid of the auto and directly used the iterator.

Can you submit a PR that makes this change in isolation? This PR is currently doing a few things at once and I'd rather tackle one problem at a time.

Done in #120 .

[ChrisThrasher]

#119 fixes the issue with our tests using a GCC extension. I'm slowly chipping away at 1st class Windows support!

[ChrisThrasher]

See #122

[ChrisThrasher]

Thanks for the PR! I've addressed everything except your CMAKE_RUNTIME_OUTPUT_DIRECTORY request which I'd prefer to not act on since I'd like to avoid hardcoding CMAKE_ constants where possible in favor of users specifying this as a configuration parameter. I hope this helps you have a much smoother time using RSL on Windows 👍🏻

[traversaro]

I've addressed everything except your CMAKE_RUNTIME_OUTPUT_DIRECTORY request which I'd prefer to not act on since I'd like to avoid hardcoding CMAKE_ constants where possible in favor of users specifying this as a configuration parameter.

Ok, I totally understand! Just to comment on a possible compromise, to make tests work out of the box while leaving the users the possibility to override CMAKE_RUNTIME_OUTPUT_DIRECTORY one option is to make it a CMake cache variable, so that a suitable default can be set, but users can still override it.

[ChrisThrasher]

Can I ask why you want to use DLLs which are notoriosly annoying to work with as opposed to the comparably much simpler static libraries? I'm also curious why you want to run RSL's test suite in the first place.

[traversaro]

Sorry, I forgot to answer. Just to clarify, I do not have any specific interest in having RSL testsuite on Windows with BUILD_SHARED_LIBS working out of the box, I just commented on how one could have a default value for CMAKE_RUNTIME_OUTPUT_DIRECTORY without forcing the users to use the hardcoded version, in case somebody read this discussion in the future.

Can I ask why you want to use DLLs which are notoriosly annoying to work with as opposed to the comparably much simpler static libraries?

In this specific case the point was bringing RSL on https://robostack.github.io/index.html, a channel providing ROS software for the conda package manager on the top of the conda-forge community mantained channel.

conda is multi-os and multi-language package manager that for what regards C++ packages ships packages with both executables and libraries, as opposed for example to vcpkg that mostly focus on distributing libraries (not executables) in source code that then the users then compile on their machine. The conda-forge distribution suggests as much as possible to package C++ libraries as shared libraries (see https://github.com/conda-forge/cfep/blob/main/cfep-18.md). There are different motivations for this, but I guess the main one is to avoid to re-compile all the packages that contain executables that depends on a given library when a given library is updated. For similar reasons, also in robostack we try to stick to distributing shared library, at least if it is possible. An additional robostack-specific reason for preferring shared linking is that I am not even sure you are able to build all the ROS distribution as static on Windows, due to how visibility headers are handled in core packages, see ament/ament_cmake#201 (comment) (or uNetworking/uSockets#184 and ros/console_bridge#40 (comment) for a similar error other not ROS projects).

Note that if a user is using a different workflow (for example you build all your dependencies from source, and they all have CMake structure that works fine with static linking, or you use bazel bypassing CMake at all) I totally agree that static linking may be more convenient, even if I can also add that in my experience not all existing CMake packages are well written to handle being built as static libraries (both on Windows or *nix). For example, a common bug is that when you are linking statically, all target_link_libraries(lib PRIVATE depLib) kind of are treated as target_link_libraries(lib PUBLIC depLib). However, it can happen that maintainers do not realize this, so they do not add in their <pkg>-config.cmake files the find_dependency calls to ensure that depLib is properly defined (example issue: robotology/idyntree#1065). Another problem that you may encounter is that static linking is sensitive to link order (see https://github.com/ros2/rmw_fastrtps/pull/190/files) So building a complex stack of CMake/C++ software sometimes is more complex than actually building as shared.

Sorry for the wall of text! I just wanted to explain why while in theory in theory shared linking on Windows may be "notoriosly annoying", sometimes it is the only way of working, unless you want to fix huge parts of the dependency stack, but explaining that in detail is a bit tricky.

I'm also curious why you want to run RSL's test suite in the first place.

When I opened the PR for the Windows fixes, I wanted to also provide a way to test if the fixes were working, nothing else beside that.