Move the spdlog header files to logger.cc. (#4881)

[SC-44926](https://app.shortcut.com/tiledb-inc/story/44926/move-spdlog-header-files-to-logger-cc) Spdlog includes `Windows.h` on Windows which includes lots of code, including some problematic defines like `DELETE`. This PR refactors the `Logger` class to no longer need including spdlog. We accomplish this by forward-declaring the `spdlog::logger` class and updating the formatted logging template methods to format the string with `fmt` themselves, and pass the string to the overload that logs strings. Formatting the strings is avoided if the respective log level is enabled. I added a method to check that. --- TYPE: NO_HISTORY
TileDB-Inc · Apr 22, 2024 · 4fe78f8 · 4fe78f8
1 parent 8031e30
commit 4fe78f8
Show file tree

Hide file tree

Showing 31 changed files with 103 additions and 79 deletions.
diff --git a/test/support/src/helpers.h b/test/support/src/helpers.h
@@ -33,7 +33,6 @@
 #ifndef TILEDB_TEST_HELPERS_H
 #define TILEDB_TEST_HELPERS_H
 
-#include <tiledb/common/logger_public.h>
 #include "test/support/src/coords_workaround.h"
 #include "test/support/src/mem_helpers.h"
 #include "tiledb.h"

diff --git a/tiledb/common/logger.cc b/tiledb/common/logger.cc
@@ -34,6 +34,8 @@
 #include <spdlog/fmt/fmt.h>
 #include <spdlog/fmt/ostr.h>
 #include <spdlog/sinks/stdout_sinks.h>
+#include <spdlog/spdlog.h>
+
 #ifndef _WIN32
 #include <spdlog/sinks/stdout_color_sinks.h>
 #endif
@@ -42,6 +44,24 @@
 
 namespace tiledb::common {
 
+spdlog::level::level_enum to_spdlog_level(Logger::Level lvl) {
+  switch (lvl) {
+    case Logger::Level::FATAL:
+      return spdlog::level::critical;
+    case Logger::Level::ERR:
+      return spdlog::level::err;
+    case Logger::Level::WARN:
+      return spdlog::level::warn;
+    case Logger::Level::INFO:
+      return spdlog::level::info;
+    case Logger::Level::DBG:
+      return spdlog::level::debug;
+    default:
+      assert(lvl == Logger::Level::TRACE);
+      return spdlog::level::trace;
+  }
+}
+
 /* ********************************* */
 /*     CONSTRUCTORS & DESTRUCTORS    */
 /* ********************************* */
@@ -189,6 +209,10 @@ void Logger::fatal(const std::stringstream& msg) {
   exit(1);
 }
 
+bool Logger::should_log(Logger::Level lvl) {
+  return logger_->should_log(to_spdlog_level(lvl));
+}
+
 void Logger::set_level(Logger::Level lvl) {
   switch (lvl) {
     case Logger::Level::FATAL:

diff --git a/tiledb/common/logger.h b/tiledb/common/logger.h
@@ -29,36 +29,25 @@
  *
  * This file defines class Logger, which is implemented as a wrapper around
  * `spdlog`. By policy `spdlog` must remain encapsulated as an implementation
- * and not be exposed as a dependency of the TileDB library. Accordingly, this
- * header should not be included as a header in any other header file. For
- * inclusion in a header (notably for use within the definition of
- * template-dependent functions), include the header `logger_public.h`.
- *
- * The reason for this restriction is a technical limitation in template
- * instantiation. Part of the interface to `spdlog` consists of template
- * functions with variadic template arguments. Instantiation of such function
- * does not instantiate a variadic function (for exmaple `printf`) but rather a
- * function with a fixed number of arguments that depend upon the argument list.
- * Such variadic template argument lists cannot be forwarded across the
- * boundaries of compilation units, so exposing variadic template arguments
- * necessarily exposes the dependency upon `spdlog`. Thus this file `logger.h`,
- * which does have such arguments, must remain entirely within the library, but
- * `logger_public.h`, which does not have such arguments, may be exposed without
- * creating an additional external dependency.
+ * and not be exposed as a dependency of the TileDB library.
  */
 
 #pragma once
 #ifndef TILEDB_LOGGER_H
 #define TILEDB_LOGGER_H
 
-#include <spdlog/spdlog.h>
+#include <fmt/format.h>
 #include <atomic>
 #include <sstream>
 
 #include "tiledb/common/common.h"
 #include "tiledb/common/heap_memory.h"
 #include "tiledb/common/status.h"
 
+namespace spdlog {
+class logger;
+}
+
 namespace tiledb {
 namespace common {
 
@@ -123,12 +112,15 @@ class Logger {
    *
    * @param fmt A fmtlib format string, see http://fmtlib.net/latest/ for
    *     details.
-   * @param arg positional argument to format.
-   * @param args optional additional positional arguments to format.
+   * @param args positional arguments to format.
    */
-  template <typename Arg1, typename... Args>
-  void trace(std::string_view fmt, const Arg1& arg1, const Args&... args) {
-    logger_->trace(fmt::runtime(fmt), arg1, args...);
+  template <typename... Args>
+  void trace(fmt::format_string<Args...> fmt, Args&&... args) {
+    // Check that this level is enabled to avoid needlessly formatting the
+    // string.
+    if (!should_log(Level::TRACE))
+      return;
+    trace(fmt::format(fmt, std::forward<Args...>(args)...));
   }
 
   /**
@@ -157,12 +149,15 @@ class Logger {
    *
    * @param fmt A fmtlib format string, see http://fmtlib.net/latest/ for
    *     details.
-   * @param arg positional argument to format.
-   * @param args optional additional positional arguments to format.
+   * @param args positional arguments to format.
    */
-  template <typename Arg1, typename... Args>
-  void debug(std::string_view fmt, const Arg1& arg1, const Args&... args) {
-    logger_->debug(fmt::runtime(fmt), arg1, args...);
+  template <typename... Args>
+  void debug(fmt::format_string<Args...> fmt, Args&&... args) {
+    // Check that this level is enabled to avoid needlessly formatting the
+    // string.
+    if (!should_log(Level::DBG))
+      return;
+    debug(fmt::format(fmt, std::forward<Args>(args)...));
   }
 
   /**
@@ -191,12 +186,15 @@ class Logger {
    *
    * @param fmt A fmtlib format string, see http://fmtlib.net/latest/ for
    *     details.
-   * @param arg positional argument to format.
-   * @param args optional additional positional arguments to format.
+   * @param args positional arguments to format.
    */
-  template <typename Arg1, typename... Args>
-  void info(std::string_view fmt, const Arg1& arg1, const Args&... args) {
-    logger_->info(fmt::runtime(fmt), arg1, args...);
+  template <typename... Args>
+  void info(fmt::format_string<Args...> fmt, Args&&... args) {
+    // Check that this level is enabled to avoid needlessly formatting the
+    // string.
+    if (!should_log(Level::INFO))
+      return;
+    info(fmt::format(fmt, std::forward<Args>(args)...));
   }
 
   /**
@@ -225,12 +223,15 @@ class Logger {
    *
    * @param fmt A fmtlib format string, see http://fmtlib.net/latest/ for
    *     details.
-   * @param arg positional argument to format.
-   * @param args optional additional positional arguments to format.
+   * @param args positional arguments to format.
    */
-  template <typename Arg1, typename... Args>
-  void warn(std::string_view fmt, const Arg1& arg1, const Args&... args) {
-    logger_->warn(fmt::runtime(fmt), arg1, args...);
+  template <typename... Args>
+  void warn(fmt::format_string<Args...> fmt, Args&&... args) {
+    // Check that this level is enabled to avoid needlessly formatting the
+    // string.
+    if (!should_log(Level::WARN))
+      return;
+    warn(fmt::format(fmt, std::forward<Args>(args)...));
   }
 
   /**
@@ -258,12 +259,15 @@ class Logger {
    *
    * @param fmt A fmtlib format string, see http://fmtlib.net/latest/ for
    * details.
-   * @param arg1 positional argument to format.
-   * @param args optional additional positional arguments to format.
+   * @param args positional arguments to format.
    */
-  template <typename Arg1, typename... Args>
-  void error(std::string_view fmt, const Arg1& arg1, const Args&... args) {
-    logger_->error(fmt::runtime(fmt), arg1, args...);
+  template <typename... Args>
+  void error(fmt::format_string<Args...> fmt, Args&&... args) {
+    // Check that this level is enabled to avoid needlessly formatting the
+    // string.
+    if (!should_log(Level::ERR))
+      return;
+    error(fmt::format(fmt, std::forward<Args>(args)...));
   }
 
   /**
@@ -287,6 +291,22 @@ class Logger {
    */
   void critical(const std::stringstream& msg);
 
+  /**
+   * A formatted critical statment.
+   *
+   * @param fmt A fmtlib format string, see http://fmtlib.net/latest/ for
+   *     details.
+   * @param args positional arguments to format.
+   */
+  template <typename... Args>
+  void critical(fmt::format_string<Args...> fmt, Args&&... args) {
+    // Check that this level is enabled to avoid needlessly formatting the
+    // string.
+    if (!should_log(Level::FATAL))
+      return;
+    critical(fmt::format(fmt, std::forward<Args>(args)...));
+  }
+
   /**
    * Log a message from a Status object and return
    * the same Status object
@@ -324,17 +344,11 @@ class Logger {
   void fatal(const std::stringstream& msg);
 
   /**
-   * A formatted critical statment.
+   * Check whether events of a given level are logged.
    *
-   * @param fmt A fmtlib format string, see http://fmtlib.net/latest/ for
-   *     details.
-   * @param arg positional argument to format.
-   * @param args optional additional positional arguments to format.
+   * @param lvl The level of events to check.
    */
-  template <typename Arg1, typename... Args>
-  void critical(std::string_view fmt, const Arg1& arg1, const Args&... args) {
-    logger_->critical(fmt::runtime(fmt), arg1, args...);
-  }
+  bool should_log(Logger::Level lvl);
 
   /**
    * Set the logger level.

diff --git a/tiledb/common/logger_public.h b/tiledb/common/logger_public.h
@@ -28,8 +28,9 @@
  * @section DESCRIPTION
  *
  * This file defines simple logging functions that can be exposed (by expedient)
- * to the public API. Their implementations are in `logger.cc`. See the
- * documentation in `logger.h` for the full story.
+ * to the public API. Their implementations are in `logger.cc`. They are
+ * declared in a separate header from `logger.h` for historical reasons, when
+ * `logger.h` was leaking spdlog's headers.
  */
 
 #pragma once

diff --git a/tiledb/sm/array/consistency.h b/tiledb/sm/array/consistency.h
@@ -37,7 +37,6 @@
 #include <map>
 
 #include "tiledb/common/common.h"
-#include "tiledb/common/logger.h"
 #include "tiledb/sm/enums/query_type.h"
 #include "tiledb/sm/filesystem/uri.h"
 

diff --git a/tiledb/sm/array/test/unit_consistency.cc b/tiledb/sm/array/test/unit_consistency.cc
@@ -33,6 +33,7 @@
 #include <test/support/tdb_catch.h>
 #include <iostream>
 
+#include "tiledb/common/logger.h"
 #include "unit_consistency.h"
 
 using namespace tiledb;

diff --git a/tiledb/sm/array_schema/dimension.h b/tiledb/sm/array_schema/dimension.h
@@ -43,7 +43,6 @@
 
 #include "tiledb/common/blank.h"
 #include "tiledb/common/common.h"
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/macros.h"
 #include "tiledb/common/memory_tracker.h"
 #include "tiledb/common/status.h"

diff --git a/tiledb/sm/compressors/dict_compressor.h b/tiledb/sm/compressors/dict_compressor.h
@@ -34,7 +34,6 @@
 #define TILEDB_DICT_COMPRESSOR_H
 
 #include "tiledb/common/common.h"
-#include "tiledb/common/logger.h"
 #include "tiledb/sm/misc/endian.h"
 
 #include <map>

diff --git a/tiledb/sm/consolidator/array_meta_consolidator.h b/tiledb/sm/consolidator/array_meta_consolidator.h
@@ -35,7 +35,6 @@
 
 #include "tiledb/common/common.h"
 #include "tiledb/common/heap_memory.h"
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/status.h"
 #include "tiledb/sm/array/array.h"
 #include "tiledb/sm/consolidator/consolidator.h"

diff --git a/tiledb/sm/consolidator/commits_consolidator.h b/tiledb/sm/consolidator/commits_consolidator.h
@@ -35,7 +35,6 @@
 
 #include "tiledb/common/common.h"
 #include "tiledb/common/heap_memory.h"
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/status.h"
 #include "tiledb/sm/array/array.h"
 #include "tiledb/sm/consolidator/consolidator.h"

diff --git a/tiledb/sm/consolidator/consolidator.h b/tiledb/sm/consolidator/consolidator.h
@@ -35,7 +35,6 @@
 
 #include "tiledb/common/common.h"
 #include "tiledb/common/heap_memory.h"
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/status.h"
 #include "tiledb/sm/array/array.h"
 #include "tiledb/sm/storage_manager/storage_manager_declaration.h"

diff --git a/tiledb/sm/consolidator/fragment_consolidator.h b/tiledb/sm/consolidator/fragment_consolidator.h
@@ -35,7 +35,6 @@
 
 #include "tiledb/common/common.h"
 #include "tiledb/common/heap_memory.h"
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/pmr.h"
 #include "tiledb/common/status.h"
 #include "tiledb/sm/array/array.h"

diff --git a/tiledb/sm/consolidator/fragment_meta_consolidator.h b/tiledb/sm/consolidator/fragment_meta_consolidator.h
@@ -35,7 +35,6 @@
 
 #include "tiledb/common/common.h"
 #include "tiledb/common/heap_memory.h"
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/status.h"
 #include "tiledb/sm/array/array.h"
 #include "tiledb/sm/consolidator/consolidator.h"

diff --git a/tiledb/sm/filesystem/posix.h b/tiledb/sm/filesystem/posix.h
@@ -45,7 +45,6 @@
 #include <string>
 #include <vector>
 
-#include "tiledb/common/logger.h"
 #include "tiledb/common/status.h"
 #include "tiledb/sm/config/config.h"
 #include "tiledb/sm/filesystem/filesystem_base.h"

diff --git a/tiledb/sm/misc/utils.h b/tiledb/sm/misc/utils.h
@@ -41,7 +41,6 @@
 #include <vector>
 
 #include "constants.h"
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/status.h"
 
 namespace tiledb {

diff --git a/tiledb/sm/query/legacy/reader.h b/tiledb/sm/query/legacy/reader.h
@@ -37,7 +37,6 @@
 
 #include "tiledb/common/common.h"
 #include "tiledb/common/indexed_list.h"
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/status.h"
 #include "tiledb/sm/array_schema/dimension.h"
 #include "tiledb/sm/query/iquery_strategy.h"

diff --git a/tiledb/sm/query/query.h b/tiledb/sm/query/query.h
@@ -39,7 +39,6 @@
 #include <utility>
 #include <vector>
 
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/status.h"
 #include "tiledb/sm/array_schema/array_schema.h"
 #include "tiledb/sm/array_schema/dimension.h"

diff --git a/tiledb/sm/query/readers/dense_reader.h b/tiledb/sm/query/readers/dense_reader.h
@@ -36,7 +36,6 @@
 #include <atomic>
 
 #include "tiledb/common/common.h"
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/status.h"
 #include "tiledb/sm/array_schema/dimension.h"
 #include "tiledb/sm/array_schema/dynamic_array.h"

diff --git a/tiledb/sm/query/readers/ordered_dim_label_reader.h b/tiledb/sm/query/readers/ordered_dim_label_reader.h
@@ -36,7 +36,6 @@
 #include <atomic>
 
 #include "tiledb/common/common.h"
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/status.h"
 #include "tiledb/sm/array_schema/dimension.h"
 #include "tiledb/sm/misc/types.h"

diff --git a/tiledb/sm/query/readers/sparse_global_order_reader.h b/tiledb/sm/query/readers/sparse_global_order_reader.h
@@ -36,7 +36,6 @@
 #include <atomic>
 
 #include "tiledb/common/common.h"
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/status.h"
 #include "tiledb/sm/array_schema/dimension.h"
 #include "tiledb/sm/query/iquery_strategy.h"

diff --git a/tiledb/sm/query/readers/sparse_unordered_with_dups_reader.h b/tiledb/sm/query/readers/sparse_unordered_with_dups_reader.h
@@ -36,7 +36,6 @@
 #include <atomic>
 
 #include "tiledb/common/common.h"
-#include "tiledb/common/logger_public.h"
 #include "tiledb/common/status.h"
 #include "tiledb/sm/array_schema/dimension.h"
 #include "tiledb/sm/query/iquery_strategy.h"