Quoted #include pattern picks up wrong (packaged) fmt library

[gh-andre]

Thank you for the logging library. It fits background logging quite well, leaving threading to the application, which many other logging libraries lack. One thing I wanted to mention is a small build issue in configurations that already have a packaged version of fmt installed.

A quoted #include pattern always looks in the current directory and only then in the search path. If fmtlog is installed on a system that already has a packaged version of fmt installed (e.g. via dnf), this causes the wrong headers picked up.

That is, fmtlog.h contains this line:

#include "fmt/format.h"

All compilers always check current directory, which always fails, and then the search path is checked. On systems where fmt is installed via a package manager, such as CentOS 8, there will be a version in /usr/include/fmt/, which would be picked via the search path.

Given that libfmt includes fmt as a Git submodule, it should always include the version of fmt in its own source subtree, which can be done by using quoted #include pattern to include the header relative to the directory of the fmtlog.h file:

#include "fmt/include/fmt/format.h"

In this case this line can be removed from CMakeLists.txt as well:

include_directories(fmt/include)

These two changes allow both versions of fmt coexist without a conflict - one is in the packaged location and one under /usr/local/include/fmtlog/fmt/, or any other directory suitable for a given project.

[gh-andre]

@MengRao

I made a few changes that build and install fmtlog without conflicting with standalone installations of fmt, whether it is packaged or built from the source. You can see changes here - all those after tag v2.1.1.

https://github.com/gh-andre/fmtlog

I'm not creating a pull request, though, because I think you would have more people using fmtlog if you would instead drop submodules altogether and just document dependencies, so people can build fmtlog against standalone versions of fmt. Here're a few suggestions I think would work well for users.

• Drop all submodules completely and change fmtlog.h to include fmt headers from standard locations, like this:

  #include <fmt/format.h>
  

• Figure out the lowest fmt version the fmtlog can work with and enforce it via checking FMT_VERSION in fmtlog.h. This will allow people to use earlier versions of fmt and will include users those who cannot install the bleeding edge fmt v8.1.0 in their build environments (e.g. CentOS 8 has v6, Fedora 33 has v7, Fedora 35 has v8.0.1, Ubuntu 20 has v6).

• For NanoLog and spdlog either grab their source with curl/wget/etc and build it as an optional component or explore CMake external projects or just document how to build the benchmark suite. I would guess that very few fmtlog users build it and it would just simplify building fmtlog from the source if those were out of the way.

• The shared library only makes sense when it's completely self-contained and on Windows a DLL cannot even be built with undefined symbols. On Linux it works, but people need to link against fmtlog-shared.so and libfmt.a, which sort of defies the purpose of using a shared library.

  libfmt.a would need to be build with -fPIC, which can be done with -DCMAKE_POSITION_INDEPENDENT_CODE=TRUE, or linked in as a shared library with BUILD_SHARED_LIBS=TRUE.

• Consider adding CMake configurations to the build, so libfmtlog-shared.so can be used in debug builds , which fail today with undefined references because debug builds of projects that include fmtlog don't define NDEBUG, which triggers assert_fail calls from fmt and libfmtlog-shared.so doesn't expose this function, so libfmt.a has to be linked in, even though its entire copy is already in libfmtlog-shared.so.

I think this would make it simpler for many to start using fmtlog. In the meantime, there's nothing to do for this issue, I suppose, so I will close it.

EDIT:

• Updated libfmt.a note with regards to -fPIC.
• Added debug vs. release configurations note.

[gh-andre]

@MengRao I made some CMake changes to build one configuration at a time (Debug or Release) and link against matching fmt libraries, if you are interested.

https://github.com/gh-andre/fmtlog/tree/cmake-debug-release

Not creating a pull request because the part with the bundled fmt isn't complete and is there just to show the idea and also because library names were changed in a breaking way to allow one library name per configuration, such as libfmtlog.a vs. libfmtlogd.a, etc.

[gh-andre]

@MengRao

I didn't want to create a new non-issue issue, so I will just post in this thread.

I added GitHub workflow for a test build. You can see the results in a couple of latest runs on Windows and Ubuntu.

https://github.com/gh-andre/fmtlog/actions

I didn't create a pull request because it uses patched CMake files, which you didn't port. The one in main is empty and exists only because GitHub requires it. Here's the one that works against latest patched main:

https://github.com/gh-andre/fmtlog/blob/main-patches/.github/workflows/fmtlog-verify-build.yaml

In their current configuration they build only manually in Actions, but you can change on to include push to run a test build whenever there are changes, which can help catch compile and link errors early.

Note that on Windows it cannot build DLL because there's no __declspec(dllexport) in any of the source files, so while it builds a DLL, there's no import library built because there are no exported symbols, so it's an empty DLL.

Also, worth noting that all builds are made with Debug and Release configurations because otherwise fmt doesn't work properly because when the application includes their headers without NDEBUG, they generate assert calls, which fails to link against a release-only fmt built within fmtlib.