Possibly use fmt bundled with spdlog

[henriksod]

https://github.com/mvukov/rules_ros2/blob/bfb0b3048e5777acad167c33cb604096d3f78e00/repositories/spdlog.BUILD.bazel#L15

This line removes all headers under include/spdlog/fmt/. This means not only fmt.h is removed, but also the following headers:

bin_to_hex.h chrono.h compile.h ostr.h ranges.h std.h xchar.h

Some projects depend on some of these headers. E.g. Micro-XRCE-DDS-Agent:

#include <spdlog/fmt/ostr.h>
#include <spdlog/fmt/bin_to_hex.h>

Suggested fix:

Change L15 in repositories/spdlog.BUILD.bazel to:

exclude = ["include/spdlog/fmt/fmt.h"],

[henriksod]

I will open a PR shortly

[mvukov]

Well, spdlog comes with fmt included. At the moment, this repo uses fmt as a separate repo. Using something like

#include <spdlog/fmt/ostr.h>
#include <spdlog/fmt/bin_to_hex.h>

looks like a bug to me, should really be

#include <fmt/ostr.h>
#include <fmt/bin_to_hex.h>

You can patch the other code.

Now, I was thinking that this repo starts using fmt from the spdlog repo, as spdlog not always uses newest fmt. So, with a bundled fmt I can also enable renovate updating spdlog in this repo. This is on my TODO list, but if you have a bit of time you can refactor this.

[henriksod]

I will try what you suggested, to patch into

#include <fmt/ostr.h>
#include <fmt/bin_to_hex.h>

I think, however, that using the built in fmt of spdlog is not a bug on their end. I guess it all comes down to whether or not the devs want to add fmt as an additional dependency to keep it up to date, or if they want to stick with the bundled version with spdlog.

[henriksod]

Got this error message

external/com_github_eprosima_micro_xrce_dds_agent/include/uxr/agent/logger/Logger.hpp:24:10: fatal error: fmt/ostr.h: No such file or directory
   24 | #include <fmt/ostr.h>

I think these headers are additions by spdlog and are not provided by fmt. That means that this issue is still valid and my suggestion stands. Or we refactor spdlog to use the built in fmt as you suggested,

[mvukov]

https://github.com/mvukov/rules_ros2/pull/156

[mvukov]

Closed via #156 .

[mikebauer]

Hey! I've run into a slight non-blocking issue with this change, but first I want to start off by thanking you for your work on this amazing library since it's my first time posting here. 👋

Rather embarrassingly, I can't seem to reproduce this behavior today (I think because I've changed what the library I'm developing depends on slightly). Consider this only a description of an observation I made yesterday.

I've been running into symbol collisions when linking with some ROS2 libraries since this landed because my project independently links fmt. I'm able to hackily get around this pretty easily with a patch. Based on the above, I think there's really not an easy way around this aside from using spdlog's fmt everywhere I currently use fmt. Does this match your understanding? This isn't a blocking thing for me, but I figured I would raise it in case anyone else runs into the same thing.

Thanks!

[ahans]

Since spdlog probably doesn't export the symbols from fmt, but just uses it internally, forcing spdlog to always link dynamically would avoid any potential conflict. I think that's what the ROS 2 default way of linking is, so there you can use different versions of a library in different binaries, be it executables or shared objects.

[mvukov]

TBH, I'd avoid using different versions of the same dep in a project.

Here is how you can solve your problem. As you probably saw in repositories/repositories.bzl, all deps are declared with maybe: that means one can override any dep. That can be done by simply defining http_archive in your master workspace before you call ros2_repositories(). So, you can copy the old spdlog.BUILD.bazel from this repo to your workspace and tell to http_archive to use that one. I will document this soon.

There is a bit more involved alternative with exposing fmt from spdlog. I can elaborate on that if the procedure above isn't satisfactory.

[ahans]

Wouldn't it be much easier to go back to the old way things were, where fmt is its own target and the one packaged with spdlog is excluded? I'm not sure what the original issue with this was. If it's about using the correct version of fmt, we could just use the exact same that spdlog ships with. Declare that as maybe as well. In case the user wants to override it, they can, but at their own risk, because it might happen that the user-provided fmt doesn't work with spdlog.

[mvukov]

The main reason fast to unblock @henriksod on his work on fastdds and Micro-XRCE-DDS-Agent (see the first post). Not sure whether fastdds also includes fmt from spdlog.

The latest fmt release doesn't work with the latest version of spdlog (at the moment of this writing). And I kinda wanted to enable renovate updating spdlog; such that I don't need to think about fmt any more... So that was a secondary reasoning I had.

We can revert #156 if it creates more problems than benefits. However, we should find a compromise solution for fastdds + Micro-XRCE-DDS-Agent eventually.

[mvukov]

Alternative, that reverts #156 , proposed in https://github.com/mvukov/rules_ros2/pull/162.

[ahans]

Alright, just wanted to share my thoughts. Since @mikebauer reported that the problem magically went away, I guess there's no need to do anything for now.

[mvukov]

Merged https://github.com/mvukov/rules_ros2/pull/162. We can reopen this if needed. @mikebauer The behavior should be the same as before.

[mikebauer]

Thanks so much for all the help on this! I'll let you know if I observe anything else interesting! 😁