TileDB icon indicating copy to clipboard operation
TileDB copied to clipboard

Bug in PR5521

Open OgreTransporter opened this issue 10 months ago • 5 comments

https://github.com/TileDB-Inc/TileDB/blob/7bdc42f39d6d3166603636a572268ae9942a3865/tiledb/common/CMakeLists.txt#L63 https://github.com/TileDB-Inc/TileDB/blob/7bdc42f39d6d3166603636a572268ae9942a3865/tiledb/common/CMakeLists.txt#L64

#5521: Why is fmt integrated here? This can lead to problems in connection with spdlog (#5438, #5006) because spdlog has its own version of fmt. Either replace fmt with spdlog at this point or remove spdlog completely and use fmt.

OgreTransporter avatar Jun 04 '25 14:06 OgreTransporter

#5536 will resolve this.

As for "why" - transient issues using spdlog::spdlog while working on #5521 by a developer (i.e. me) who was previously unaware of the prior issues.

rroelke avatar Jun 05 '25 01:06 rroelke

We have direct dependencies on both fmt and spdlog, and I'm not sure that using fmt through spdlog is a good idea. I'm also not sure that spdlog's bundled copy is intended to be used by external code. If I'm right, then we should revert #5008.

@OgreTransporter, if you are getting configure errors that fmt is not found, you should make sure it is installed, in addition to spdlog.

The current arrangement might enable the use of multiple copies of fmt in the library, but there are no places where fmt and fmt-through-spdlog interact with each other, and the use of a package manager like vcpkg ensures that only one copy of fmt gets compiled.

teo-tsirpanis avatar Jun 05 '25 01:06 teo-tsirpanis

The problem is that spdlog has unfortunately integrated a version of fmt. There are problems when you link with static libraries. There are linker errors that the objects of fmt are already defined and integrated.

When integrating spdlog, an option for fmt is also set.

set(SPDLOG_FMT_EXTERNAL OFF)
set(SPDLOG_FMT_EXTERNAL_HO OFF)
set(config_targets_file spdlogConfigTargets.cmake)

if(SPDLOG_FMT_EXTERNAL OR SPDLOG_FMT_EXTERNAL_HO)
    include(CMakeFindDependencyMacro)
    find_dependency(fmt CONFIG)
endif()

A possible solution would be to check this option and request that fmt is external. However, this is not so easy, because it depends on the spdlog version whether these variables exist or not.

Another option would be to supply spdlog and fmt as submodules of TileDB and integrate them into the build (https://github.com/gabime/spdlog/wiki/CMake#use-as-static-library-with-externalproject_add-as-git-submodule). This way you can control that fmt is built externally and is also used externally by spdlog. If you do not compile spdlog, you can also use it as header-only.

A third variant would be to use CMake to check the compiler for std::format and then use std::format instead of fmt if this is the case.

OgreTransporter avatar Jun 05 '25 05:06 OgreTransporter

A possible solution would be to check this option and request that fmt is external. However, this is not so easy, because it depends on the spdlog version whether these variables exist or not.

Exactly, I don't think that introspecting the built spdlog configuration is a sustainable solution.

Another option would be to supply spdlog and fmt as submodules of TileDB and integrate them into the build

This is not a direction we want to follow. Our existing approach of using vcpkg to manage our dependencies is better and provides features like build isolation and artifact caching.

A third variant would be to use CMake to check the compiler for std::format and then use std::format instead of fmt if this is the case.

That option sounds better, but I would prefer it to be opt-in instead of automatically checking the compiler.


@OgreTransporter could you get into more detail about your build environment? How are you acquiring dependencies? Are you getting spdlog from an external source and cannot build it with -DSPDLOG_FMT_EXTERNAL=ON?

teo-tsirpanis avatar Jun 08 '25 17:06 teo-tsirpanis

Yes, I use a prebuild binary version of spdlog which includes fmt.

OgreTransporter avatar Jul 12 '25 14:07 OgreTransporter

I'm going to close this issue. Per my comments above, the fmt dependency is expected and by design, and we are not planning to remove it at the moment.

For better dependency management, you are recommended to build your entire project with vcpkg, which now includes a tiledb port.

teo-tsirpanis avatar Dec 12 '25 00:12 teo-tsirpanis