spdlog 1.11.0 with fmt 1.10.0 has test_daily_logger:126 failing

[commel]

I am using stable spdlog 1.11.0 with stable fmt 10.0.0. To make it work I've added patch 0ca574a. The compilation now works, but the test unit test_daily_logger now fails on me.

[   23s] daily_file_sink::daily_filename_format_calculator
[   23s] -------------------------------------------------------------------------------
[   23s] /home/abuild/rpmbuild/BUILD/spdlog-1.11.0/tests/test_daily_logger.cpp:126
[   23s] ...............................................................................
[   23s] 
[   23s] /home/abuild/rpmbuild/BUILD/spdlog-1.11.0/tests/test_daily_logger.cpp:126: FAILED:
[   23s] due to unexpected exception with message:
[   23s]   invalid format

Has anyone any insight, what might have changed here due to the patch? If I build spdlog with the bundled fmt it works.

[commel]

I've patched the double brackets out now, for example the test_daily_logger:

spdlog/tests/test_daily_logger.cpp

Line 126 in ad0e89c

TEST_CASE("daily_file_sink::daily_filename_format_calculator", "[daily_file_sink]]")

vs

spdlog/tests/test_daily_logger.cpp

Line 126 in 57a9fd0

TEST_CASE("daily_file_sink::daily_filename_format_calculator", "[daily_file_sink]")

but the test still fails. Has anyone a hint on what could be the problem here?

[tt4g]

[ 23s] /home/abuild/rpmbuild/BUILD/spdlog-1.11.0/tests/test_daily_logger.cpp:126: FAILED:
[ 23s] due to unexpected exception with message:
[ 23s] invalid format

It appears that the following line failed to format.

spdlog/tests/test_daily_logger.cpp

Lines 132 to 133 in 57a9fd0

	REQUIRE(filename ==
	spdlog::fmt_lib::format(SPDLOG_FILENAME_T("example-{:04d}-{:02d}-{:02d}.log"), tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday));

However, I could not figure out why this formatting started to fail in fmt 10.

[gabime]

Are we sure this exception comes from fmt ?

[commel]

@tt4g does it fail for you as well? or is the problem "only" in the opensuse-factory builds?

@gabime When I switch to embedded fmt (9.x) it works. Also the build worked before the fmt 1.10 release.

[tt4g]

Tried to reproduce with Godbolt, but the same problem did not occur.

[tt4g]

May be related fmtlib/fmt#3440.

[commel]

I could reproduce it outside of openSUSE factory with spdlog 1.11.0 and fmt 1.10.0. The offending line is:

spdlog/tests/test_daily_logger.cpp

Line 130 in ad0e89c

auto filename = spdlog::sinks::daily_filename_format_calculator::calc_filename(SPDLOG_FILENAME_T("example-%Y-%m-%d.log"), tm);

if i exchange it with a otherwise defined filename, the test is executed.

[tt4g]

From that line, I examined the likely source of the error and found that the following source code violates the fmt format specification.

spdlog/include/spdlog/sinks/daily_file_sink.h

Lines 78 to 86 in 57a9fd0

	// generate fmt datetime format string, e.g. {:%Y-%m-%d}.
	filename_t fmt_filename = fmt::format(SPDLOG_FMT_STRING(SPDLOG_FILENAME_T("{{:{}}}")), filename);
	// MSVC doesn't allow fmt::runtime(..) with wchar, with fmtlib versions < 9.1.x
	# if defined(_MSC_VER) && defined(SPDLOG_WCHAR_FILENAMES) && FMT_VERSION < 90101
	return fmt::format(fmt_filename, now_tm);
	# else
	return fmt::format(SPDLOG_FMT_RUNTIME(fmt_filename), now_tm);
	# endif

I expect the behavior of fmt version 9.1.0 and earlier was unintended. I have opened an issue (fmtlib/fmt#3445) in the fmt library for confirmation.

@commel If you make the following modifications, all should be well. Could you try it?

 struct daily_filename_format_calculator
 {
     static filename_t calc_filename(const filename_t &filename, const tm &now_tm)
     {
-#ifdef SPDLOG_USE_STD_FORMAT
         // adapted from fmtlib: https://github.com/fmtlib/fmt/blob/8.0.1/include/fmt/chrono.h#L522-L546
 
         filename_t tm_format;
         tm_format.append(filename);
         // By appending an extra space we can distinguish an empty result that
         // indicates insufficient buffer size from a guaranteed non-empty result
         // https://github.com/fmtlib/fmt/issues/2238
         tm_format.push_back(' ');
 
         const size_t MIN_SIZE = 10;
         filename_t buf;
         buf.resize(MIN_SIZE);
         for (;;)
         {
             size_t count = strftime(buf.data(), buf.size(), tm_format.c_str(), &now_tm);
             if (count != 0)
             {
                 // Remove the extra space.
                 buf.resize(count - 1);
                 break;
             }
             buf.resize(buf.size() * 2);
         }
 
         return buf;
-#else
-        // generate fmt datetime format string, e.g. {:%Y-%m-%d}.
-        filename_t fmt_filename = fmt::format(SPDLOG_FMT_STRING(SPDLOG_FILENAME_T("{{:{}}}")), filename);
-
-        // MSVC doesn't allow fmt::runtime(..) with wchar, with fmtlib versions < 9.1.x
-#    if defined(_MSC_VER) && defined(SPDLOG_WCHAR_FILENAMES) && FMT_VERSION < 90101
-        return fmt::format(fmt_filename, now_tm);
-#    else
-        return fmt::format(SPDLOG_FMT_RUNTIME(fmt_filename), now_tm);
-#    endif
-
-#endif
     }

[gabime]

@tt4g Thanks for the investigation.
So the fix would be replacing
“example-%Y-%m-%d.log”
with:
“example_%Y-%m-%d.log" ? or shoiuld it be “%Y-%m-%d.log” ?

[commel]

Just had the chance to look at it. At the moment the strftime is not working because the string.data() is const.

[commel]

With the applied patch 0ca574a.patch, the removed double brackets ]] from some tests from 7f09c88 and the applied change from above the tests now work :-)

The adapted diffed version of daily_file_sink.h now looks like this:

index f6f1bb1d..31516ee2 100644
--- a/include/spdlog/sinks/daily_file_sink.h
+++ b/include/spdlog/sinks/daily_file_sink.h
@@ -18,6 +18,7 @@
 #include <ctime>
 #include <mutex>
 #include <string>
+#include <vector>
 
 namespace spdlog {
 namespace sinks {
@@ -48,7 +49,6 @@ struct daily_filename_format_calculator
 {
     static filename_t calc_filename(const filename_t &filename, const tm &now_tm)
     {
-#ifdef SPDLOG_USE_STD_FORMAT
         // adapted from fmtlib: https://github.com/fmtlib/fmt/blob/8.0.1/include/fmt/chrono.h#L522-L546
 
         filename_t tm_format;
@@ -59,7 +59,7 @@ struct daily_filename_format_calculator
         tm_format.push_back(' ');
 
         const size_t MIN_SIZE = 10;
-        filename_t buf;
+        std::vector<char> buf;
         buf.resize(MIN_SIZE);
         for (;;)
         {
@@ -73,19 +73,7 @@ struct daily_filename_format_calculator
             buf.resize(buf.size() * 2);
         }
 
-        return buf;
-#else
-        // generate fmt datetime format string, e.g. {:%Y-%m-%d}.
-        filename_t fmt_filename = fmt::format(SPDLOG_FMT_STRING(SPDLOG_FILENAME_T("{{:{}}}")), filename);
-
-        // MSVC doesn't allow fmt::runtime(..) with wchar, with fmtlib versions < 9.1.x
-#    if defined(_MSC_VER) && defined(SPDLOG_WCHAR_FILENAMES) && FMT_VERSION < 90101
-        return fmt::format(fmt_filename, now_tm);
-#    else
-        return fmt::format(SPDLOG_FMT_RUNTIME(fmt_filename), now_tm);
-#    endif
-
-#endif
+        return std::string(buf.cbegin(), buf.cend());
     }
 
 private:

std::string.data() and std::string.c_str() are both const char*, so I improvised for a std::vector.

[tt4g]

Trailing literals are allowed, so %Y-%m-%d.log works.
However, @commel patch is required for compatibility.

[gabime]

PR is welcome @commel

[commel]

I opened a PR the second you wrote :-) GCC12 with CPP20 has a problem, I'll look into that tomorrow. Thank you both for your time, much appreciated!

[gabime]

Thank you! Let me know if you manage to fix this. I will wait with the merge till then.
Btw I think the initial buffer size of 10 might be too small. Better start with, lets say 64.