Rationale of using vendored fmt

[traversaro]

Comment:

Hello @conda-forge/spdlog ! I was debugging an issue in downstream code that depended on spdlog but that used fmt, and I noticed that at the moment this package uses the fmt vendored in spdlog, instead of using the external fmt provided by conda-forge. I wanted to ask if there is any reason behind that. Thanks in advance!

I also try to look a bit in the repo history, and I see that the use of the conda-forge fmt was added in #33, but then removed in #35 .

[traversaro]

fyi @conda-forge/fmt, as I guess this could be of interest also for fmt mantainers.

[bluescarni]

FWIW, I agree that in conda-forge we should try to use the external fmt rather than the embedded one.

The only potential issue that needs to be addressed, in my opinion, is that, due to fmt API changes/deprecations, spdlog sometimes does not work properly with the latest fmt. So, I think that perhaps we should have some sort of version pinning mechanism in the spdlog recipe to ensure that spdlog depends on an fmt version known to be compatible with the current spdlog release.

[traversaro]

If I am not wrong, something like what you describe should be already provided by the usual conda-forge ABI migration/pinning system. If we add fmt as an host dependency, then the fmt version specified in https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/84e3caacb4fd5e0dbc0693a52683aa14dfdc9211/recipe/conda_build_config.yaml#L439 is used. To migrate to a new fmt version, an automatic PR will be opened by @regro-cf-autotick-bot (like conda-forge/heavydb-ext-feedstock#13). If a given version of spdlog is not compatible with the new version of fmt, the migration PR will not be merged.

[bluescarni]

I agree that this would work as a first line of defence. My only concern is that, at least in my experience, the incompatibilities between specific versions of fmt/spdlog can be subtle and hard to detect (e.g., I had one specific case in which compilation would fail only when the spdlog/fmt headers were included in a specific order).

So I am a bit hesitant to automatically build spdlog against the latest fmt available (if that is indeed what you are suggesting) and I think a manual pinning in the spdlog recipe would be safer (i.e., just read up in the spdlog docs which specific version of fmt is vendored and pin to the same major version in the spdlog recipe).

[traversaro]

So I am a bit hesitant to automatically build spdlog against the latest fmt available (if that is indeed what you are suggesting) and I think a manual pinning in the spdlog recipe would be safer (i.e., just read up in the spdlog docs which specific version of fmt is vendored and pin to the same major version in the spdlog recipe).

There is already an implicit pinning at the conda-forge level that is propagated to individual packages via automatically generated (but manually merged) PRs, but if you prefer to add some local recipe pinning I do not think it is a problem.

[bluescarni]

Perhaps I am just overthinking this, let's just go with the standard conda-forge pinning. We can always add local pinning if issues arise.

[h-vetinari]

+1 for unvendoring

[h-vetinari]

I'm seeing some missing symbols related to spdlog, fmt and - strangely - C++11 vs. C++17¹ in conda-forge/bear-feedstock#23, c.f.

Process.cc:(.text._ZN6spdlog6logger4log_IJRKiPKcEEEvNS_10source_locENS_5level10level_enumEN3fmt2v917basic_string_viewIcEEDpOT_[_ZN6spdlog6logger4log_IJRKiPKcEEEvNS_10source_locENS_5level10level_enumEN3fmt2v917basic_string_viewIcEEDpOT_]+0x13c): undefined reference to `spdlog::details::log_msg::log_msg(spdlog::source_loc, fmt::v9::basic_string_view<char>, spdlog::level::level_enum, fmt::v9::basic_string_view<char>)'
/home/conda/feedstock_root/build_artifacts/bear_1662430040791/_build_env/bin/../lib/gcc/x86_64-conda-linux-gnu/10.4.0/../../../../x86_64-conda-linux-gnu/bin/ld: ../libsys/CMakeFiles/sys_a.dir/source/Process.cc.o:Process.cc:(.text._ZN6spdlog6logger4log_IJiRKNSt7__cxx114listINS2_12basic_stringIcSt11char_traitsIcESaIcEEESaIS8_EEEEEEvNS_10source_locENS_5level10level_enumEN3fmt2v917basic_string_viewIcEEDpOT_[_ZN6spdlog6logger4log_IJiRKNSt7__cxx114listINS2_12basic_stringIcSt11char_traitsIcESaIcEEESaIS8_EEEEEEvNS_10source_locENS_5level10level_enumEN3fmt2v917basic_string_viewIcEEDpOT_]+0x147): more undefined references to `spdlog::details::log_msg::log_msg(spdlog::source_loc, fmt::v9::basic_string_view<char>, spdlog::level::level_enum, fmt::v9::basic_string_view<char>)' follow

Bear needs to follow [snip details] the ABI of our shared abseil builds, which is C++17, but I'm quite surprised that the C++ standard version has some impact here (outside of abseil, that is). I couldn't see abseil being used in upstream spdlog or fmt.

Footnotes

1. I'm going by the "cxx11" in the mangled symbol name. ↩

[traversaro]

I may be wrong, but the problem is that someone is passing a fmt::v9::<...> object to a spdlog function that was compiled with vendored fmt, that is v8, so if I am not wrong this is exactly something that would go away if we de-vendor fmt.

[h-vetinari]

so if I am not wrong this is exactly something that would go away if we de-vendor fmt.

Yes it would, because then we'd be compiling all packages that are exposed to the fmt-ABI in a consistent manner, and migrate in a controlled manner from e.g. fmt 8 -> 9.

[bluescarni]

I've finally been bitten by incompatibilities between the bundled fmt version in spdlog and conda's fmt package, I've thus opened #50 to attempt de-vendoring fmt.

Pinging @traversaro

[bluescarni]

I will be closing this issue as fmt is now unvendored.