Skip to content

Commit ac3640d

Browse files
committed
polish design to be aligned with the repo
1 parent b69d051 commit ac3640d

11 files changed

Lines changed: 95 additions & 215 deletions

File tree

README.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ cmake --install build
9696
| `ICEBERG_BUILD_BUNDLE` | `ON` | Build the battery-included library |
9797
| `ICEBERG_BUILD_REST` | `ON` | Build REST catalog client |
9898
| `ICEBERG_BUILD_REST_INTEGRATION_TESTS` | `OFF` | Build REST catalog integration tests |
99+
| `ICEBERG_SPDLOG` | `ON` | Use spdlog as the default logging backend |
99100
| `ICEBERG_BUILD_HIVE` | `OFF` | Build Hive (HMS) catalog client |
100101
| `ICEBERG_BUILD_SQL_CATALOG` | `OFF` | Build SQL catalog client |
101102
| `ICEBERG_SQL_SQLITE` | `OFF` | Build the SQLite connector for the SQL catalog |
@@ -124,6 +125,7 @@ Meson-specific options (configured via `-D<option>=<value>`):
124125
|--------|---------|-------------|
125126
| `rest` | `enabled` | Build REST catalog client |
126127
| `rest_integration_test` | `disabled` | Build integration test for REST catalog |
128+
| `spdlog` | `enabled` | Use spdlog as the default logging backend |
127129
| `tests` | `enabled` | Build tests |
128130

129131
### Build Examples

meson.options

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ option(
5454
option(
5555
'spdlog',
5656
type: 'feature',
57-
description: 'Use spdlog as the default logging backend (CMake: ICEBERG_SPDLOG)',
57+
description: 'Use spdlog as the default logging backend',
5858
value: 'enabled',
5959
)
6060

src/iceberg/CMakeLists.txt

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,17 +18,6 @@
1818
set(ICEBERG_INCLUDES "$<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/src>"
1919
"$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src>")
2020

21-
# Generate the logging backend config header. ALWAYS generated (not gated by
22-
# ICEBERG_SPDLOG) so logging/logger.cc can include it in both ON and OFF builds;
23-
# only the definedness of ICEBERG_HAS_SPDLOG varies. Generated into the build
24-
# tree (already on ICEBERG_INCLUDES), included as "iceberg/logging/config.h", and
25-
# NOT installed (it must never appear in a public/installed header).
26-
if(ICEBERG_SPDLOG)
27-
set(ICEBERG_HAS_SPDLOG ON)
28-
endif()
29-
configure_file("${CMAKE_CURRENT_SOURCE_DIR}/logging/config.h.in"
30-
"${CMAKE_CURRENT_BINARY_DIR}/logging/config.h")
31-
3221
set(ICEBERG_SOURCES
3322
arrow_c_data_guard_internal.cc
3423
arrow_c_data_util.cc
@@ -67,6 +56,7 @@ set(ICEBERG_SOURCES
6756
location_provider.cc
6857
logging/cerr_logger.cc
6958
logging/logger.cc
59+
logging/spdlog_logger.cc
7060
manifest/manifest_adapter.cc
7161
manifest/manifest_entry.cc
7262
manifest/manifest_filter_manager.cc
@@ -188,10 +178,8 @@ list(APPEND
188178
"$<IF:$<BOOL:${NLOHMANN_JSON_VENDORED}>,iceberg::nlohmann_json,$<IF:$<TARGET_EXISTS:nlohmann_json::nlohmann_json>,nlohmann_json::nlohmann_json,nlohmann_json::nlohmann_json>>"
189179
"$<IF:$<BOOL:${UTF8PROC_VENDORED}>,iceberg::utf8proc,utf8proc::utf8proc>")
190180

191-
# spdlog backend: linked and compiled only when ICEBERG_SPDLOG is ON. When OFF,
192-
# the core library has no spdlog dependency and CerrLogger is the default sink.
193181
if(ICEBERG_SPDLOG)
194-
list(APPEND ICEBERG_SOURCES logging/internal/spdlog_logger.cc)
182+
set(ICEBERG_HAS_SPDLOG ON)
195183
list(APPEND ICEBERG_STATIC_BUILD_INTERFACE_LIBS spdlog::spdlog)
196184
list(APPEND ICEBERG_SHARED_BUILD_INTERFACE_LIBS spdlog::spdlog)
197185
list(APPEND ICEBERG_STATIC_INSTALL_INTERFACE_LIBS
@@ -200,6 +188,9 @@ if(ICEBERG_SPDLOG)
200188
"$<IF:$<BOOL:${SPDLOG_VENDORED}>,iceberg::spdlog,spdlog::spdlog>")
201189
endif()
202190

191+
configure_file("${CMAKE_CURRENT_SOURCE_DIR}/logging/config.h.in"
192+
"${CMAKE_CURRENT_BINARY_DIR}/logging/config.h")
193+
203194
add_iceberg_lib(iceberg
204195
SOURCES
205196
${ICEBERG_SOURCES}

src/iceberg/logging/config.h.in

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,7 @@
1919

2020
#pragma once
2121

22-
// Internal, build-generated configuration for the logging backend.
23-
// This header is NOT installed and must only be included from .cc files
24-
// (logger.cc, internal/spdlog_logger.cc) -- never from a public header.
25-
//
26-
// ICEBERG_HAS_SPDLOG is defined when the project is built with -DICEBERG_SPDLOG=ON
27-
// and left undefined otherwise. Always test it with #ifdef / #ifndef, never #if
28-
// (it carries no value).
22+
// Internal, build-generated configuration; not installed.
23+
// ICEBERG_HAS_SPDLOG is defined when the spdlog backend is available.
2924

3025
#cmakedefine ICEBERG_HAS_SPDLOG

src/iceberg/logging/logger.cc

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,8 @@
2626
#include <tuple>
2727
#include <utility>
2828

29-
// Build-generated, .cc-only (never from a public header). Defines
30-
// ICEBERG_HAS_SPDLOG when built with -DICEBERG_SPDLOG=ON; tested with #ifdef.
3129
#include "iceberg/logging/cerr_logger.h"
32-
#include "iceberg/logging/config.h"
33-
#ifdef ICEBERG_HAS_SPDLOG
34-
# include "iceberg/logging/internal/spdlog_logger.h"
35-
#endif
30+
#include "iceberg/logging/spdlog_logger_internal.h"
3631

3732
namespace iceberg {
3833

@@ -48,10 +43,7 @@ class NoopLogger final : public Logger {
4843
bool IsNoop() const override { return true; }
4944
};
5045

51-
/// \brief Construct the process default logger for this build configuration.
52-
///
53-
/// Prefers the spdlog backend when compiled in; otherwise the always-available
54-
/// std::cerr logger.
46+
/// \brief Construct the configured process default logger.
5547
std::shared_ptr<Logger> MakeDefaultLogger() {
5648
#ifdef ICEBERG_HAS_SPDLOG
5749
return std::make_shared<internal::SpdLogger>();

src/iceberg/logging/meson.build

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,21 +15,14 @@
1515
# specific language governing permissions and limitations
1616
# under the License.
1717

18-
# Generate the .cc-only logging backend config header. ALWAYS generated (logger.cc
19-
# includes it in both configurations); only the definedness of ICEBERG_HAS_SPDLOG
20-
# varies with the `spdlog` feature option -- mirroring CMake's ICEBERG_SPDLOG. When
21-
# disabled, CerrLogger is the default sink and spdlog is neither compiled nor
22-
# linked. Generated into build/src/iceberg/logging/config.h (resolved via
23-
# include_directories('..'), which exposes both the source and build trees); not
24-
# installed.
18+
# Generate the internal, non-installed logging backend config.
2519
logging_config_data = configuration_data()
2620
if spdlog_enabled
2721
logging_config_data.set('ICEBERG_HAS_SPDLOG', 1)
2822
endif
2923
configure_file(output: 'config.h', configuration: logging_config_data)
3024

31-
# Public logging headers. The build-generated config.h and the internal
32-
# SpdLogger header are intentionally NOT installed.
25+
# Install only public logging headers.
3326
install_headers(
3427
[
3528
'cerr_logger.h',

src/iceberg/logging/internal/spdlog_logger.cc renamed to src/iceberg/logging/spdlog_logger.cc

Lines changed: 12 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,13 @@
1717
* under the License.
1818
*/
1919

20-
#include "iceberg/logging/internal/spdlog_logger.h"
20+
#include "iceberg/logging/spdlog_logger_internal.h"
2121

2222
#ifdef ICEBERG_HAS_SPDLOG
2323

2424
# include <memory>
2525
# include <string>
2626
# include <unordered_map>
27-
# include <utility>
2827

2928
# include <spdlog/common.h>
3029
# include <spdlog/sinks/stdout_color_sinks.h>
@@ -47,65 +46,46 @@ spdlog::level::level_enum ToSpdLevel(LogLevel level) noexcept {
4746
return spdlog::level::err;
4847
case LogLevel::kCritical:
4948
case LogLevel::kFatal:
50-
// spdlog has no "fatal"; the process abort is owned by the macro layer.
49+
// spdlog has no fatal level; the macro layer handles termination.
5150
return spdlog::level::critical;
5251
case LogLevel::kOff:
5352
return spdlog::level::off;
5453
}
5554
return spdlog::level::off;
5655
}
5756

58-
/// \brief The built-in sink: a color stderr spdlog logger.
59-
std::shared_ptr<spdlog::logger> MakeDefaultSpdLogger() {
60-
return std::make_shared<spdlog::logger>(
61-
"iceberg", std::make_shared<spdlog::sinks::stderr_color_sink_mt>());
62-
}
63-
6457
} // namespace
6558

66-
SpdLogger::SpdLogger(LogLevel level) : SpdLogger(MakeDefaultSpdLogger(), level) {}
59+
SpdLogger::SpdLogger(LogLevel level)
60+
: logger_("iceberg", std::make_shared<spdlog::sinks::stderr_color_sink_mt>()),
61+
level_(level) {
62+
logger_.set_level(spdlog::level::trace);
63+
}
6764

6865
Status SpdLogger::Initialize(
6966
const std::unordered_map<std::string, std::string>& properties) {
7067
if (auto it = properties.find(std::string(kPatternProperty)); it != properties.end()) {
71-
logger_->set_pattern(it->second);
68+
logger_.set_pattern(it->second);
7269
}
73-
// Apply "level" via the base implementation.
7470
return Logger::Initialize(properties);
7571
}
7672

77-
SpdLogger::SpdLogger(std::shared_ptr<spdlog::logger> logger, LogLevel level)
78-
: logger_(std::move(logger)), level_(level) {
79-
// logger_ is non-null for the rest of this object's life, so Initialize/Log/Flush
80-
// may dereference it unconditionally. Enforced by substitution rather than an
81-
// assertion, which would vanish under NDEBUG and leave a release-build crash: a
82-
// null argument falls back to the same stderr-backed logger the default
83-
// constructor builds, so a caller mistake degrades to the default sink.
84-
if (!logger_) {
85-
logger_ = MakeDefaultSpdLogger();
86-
}
87-
logger_->set_level(spdlog::level::trace); // filtering is done by ShouldLog
88-
}
89-
9073
void SpdLogger::Log(LogMessage&& message) noexcept {
9174
try {
9275
spdlog::source_loc loc{message.location.file_name(),
9376
static_cast<int>(message.location.line()),
9477
message.location.function_name()};
95-
// Raw-message overload: the text is already formatted, so hand spdlog the bytes
96-
// directly instead of running them back through fmt (which would re-parse and
97-
// copy the whole message, allocating for long ones). It also means braces in the
98-
// message can never be interpreted as format placeholders.
99-
logger_->log(loc, ToSpdLevel(message.level),
100-
spdlog::string_view_t{message.message.data(), message.message.size()});
78+
// LogMessage is already formatted; use spdlog's raw-message overload.
79+
logger_.log(loc, ToSpdLevel(message.level),
80+
spdlog::string_view_t{message.message.data(), message.message.size()});
10181
} catch (...) {
10282
// Logging must never throw.
10383
}
10484
}
10585

10686
void SpdLogger::Flush() noexcept {
10787
try {
108-
logger_->flush();
88+
logger_.flush();
10989
} catch (...) {
11090
}
11191
}

src/iceberg/logging/internal/spdlog_logger.h renamed to src/iceberg/logging/spdlog_logger_internal.h

Lines changed: 3 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -19,52 +19,25 @@
1919

2020
#pragma once
2121

22-
/// \file iceberg/logging/internal/spdlog_logger.h
23-
/// \brief spdlog-backed logging sink.
24-
///
25-
/// INTERNAL, NOT INSTALLED. Included only from .cc files (logger.cc and
26-
/// spdlog_logger.cc). It pulls in the build-generated config.h itself and gates
27-
/// its entire body on ICEBERG_HAS_SPDLOG, so it compiles to nothing unless the
28-
/// project was built with ICEBERG_SPDLOG=ON. SpdLogger is not a
29-
/// consumer-constructible public type -- applications obtain it via the default
30-
/// logger or the "logger-impl"="spdlog" registry factory.
22+
/// \file iceberg/logging/spdlog_logger_internal.h
23+
/// \brief Internal spdlog-backed logging sink.
3124

3225
#include "iceberg/logging/config.h"
3326

3427
#ifdef ICEBERG_HAS_SPDLOG
3528

3629
# include <atomic>
37-
# include <memory>
3830

3931
# include <spdlog/logger.h>
4032

41-
# include "iceberg/logging/log_level.h"
4233
# include "iceberg/logging/logger.h"
4334

4435
namespace iceberg::internal {
4536

46-
/// \brief Logger backed by spdlog (synchronous only in v1).
47-
///
48-
/// Synchronous because spdlog::source_loc holds non-owning const char* that are
49-
/// unsafe to forward into an async logger (spdlog #3227).
50-
/// ICEBERG_EXPORT so the symbol is linkable from in-tree tests (and any
51-
/// internal consumer) under -fvisibility=hidden / MSVC DLL builds. The header
52-
/// is still not installed -- this is a binary-visibility detail, not public API.
5337
class ICEBERG_EXPORT SpdLogger : public Logger {
5438
public:
55-
/// \brief Construct over a default stderr-backed spdlog logger.
5639
explicit SpdLogger(LogLevel level = LogLevel::kInfo);
5740

58-
/// \brief Construct over a caller-provided spdlog logger.
59-
///
60-
/// The logger MUST be synchronous. Log() forwards spdlog::source_loc, which
61-
/// borrows the std::source_location's const char* pointers; an async spdlog
62-
/// logger would queue them past their lifetime (spdlog #3227 -> UB). This is a
63-
/// caller contract -- spdlog exposes no reliable sync/async query to assert on.
64-
explicit SpdLogger(std::shared_ptr<spdlog::logger> logger,
65-
LogLevel level = LogLevel::kInfo);
66-
67-
/// \brief Apply the "pattern" property (spdlog set_pattern), then "level".
6841
Status Initialize(
6942
const std::unordered_map<std::string, std::string>& properties) override;
7043

@@ -81,7 +54,7 @@ class ICEBERG_EXPORT SpdLogger : public Logger {
8154
void Flush() noexcept override;
8255

8356
private:
84-
std::shared_ptr<spdlog::logger> logger_;
57+
spdlog::logger logger_;
8558
std::atomic<LogLevel> level_;
8659
};
8760

src/iceberg/meson.build

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -63,12 +63,11 @@ configure_file(
6363
install_dir: get_option('includedir') / 'iceberg',
6464
)
6565

66-
# spdlog logging backend, mirroring CMake's ICEBERG_SPDLOG. Resolved before
67-
# subdir('logging') because config.h's ICEBERG_HAS_SPDLOG depends on it.
68-
spdlog_enabled = get_option('spdlog').allowed()
66+
# Resolve spdlog before generating logging/config.h. The feature value controls
67+
# whether a missing dependency is allowed.
68+
spdlog_dep = dependency('spdlog', required: get_option('spdlog'))
69+
spdlog_enabled = spdlog_dep.found()
6970

70-
# Generate iceberg/logging/config.h (must precede the library() that compiles
71-
# the logging sources which include it).
7271
subdir('logging')
7372

7473
iceberg_include_dir = include_directories('..')
@@ -109,6 +108,7 @@ iceberg_sources = files(
109108
'location_provider.cc',
110109
'logging/cerr_logger.cc',
111110
'logging/logger.cc',
111+
'logging/spdlog_logger.cc',
112112
'manifest/manifest_adapter.cc',
113113
'manifest/manifest_entry.cc',
114114
'manifest/manifest_filter_manager.cc',
@@ -204,12 +204,6 @@ iceberg_sources = files(
204204
'util/uuid.cc',
205205
)
206206

207-
# The spdlog sink is compiled only when the backend is enabled; with it off the
208-
# core library has no spdlog dependency and CerrLogger is the default sink.
209-
if spdlog_enabled
210-
iceberg_sources += files('logging/internal/spdlog_logger.cc')
211-
endif
212-
213207
iceberg_data_sources = files(
214208
'data/data_writer.cc',
215209
'data/delete_filter.cc',
@@ -256,10 +250,7 @@ zlib_dep = dependency('zlib')
256250

257251
iceberg_deps = [nanoarrow_dep, nlohmann_json_dep, utf8proc_dep, zlib_dep]
258252

259-
# spdlog is looked up and linked only when the backend is enabled, so a
260-
# -Dspdlog=disabled build needs neither the dependency nor the sink.
261253
if spdlog_enabled
262-
spdlog_dep = dependency('spdlog')
263254
iceberg_deps += spdlog_dep
264255
endif
265256

src/iceberg/test/logger_test.cc

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@
2727

2828
#include <gtest/gtest.h>
2929

30+
#include "iceberg/logging/cerr_logger.h"
3031
#include "iceberg/logging/log_level.h"
32+
#include "iceberg/logging/spdlog_logger_internal.h"
3133
#include "iceberg/test/logging_test_helpers.h"
3234
#include "iceberg/test/matchers.h"
3335

@@ -43,7 +45,15 @@ TEST(LoggerTest, NoopIsSharedImmortalAndSilent) {
4345
EXPECT_EQ(noop.get(), Logger::Noop().get());
4446
}
4547

46-
TEST(LoggerTest, DefaultLoggerIsNeverNull) { EXPECT_NE(GetDefaultLogger(), nullptr); }
48+
TEST(LoggerTest, DefaultLoggerMatchesBuildConfiguration) {
49+
auto logger = GetDefaultLogger();
50+
ASSERT_NE(logger, nullptr);
51+
#ifdef ICEBERG_HAS_SPDLOG
52+
EXPECT_NE(dynamic_cast<internal::SpdLogger*>(logger.get()), nullptr);
53+
#else
54+
EXPECT_NE(dynamic_cast<CerrLogger*>(logger.get()), nullptr);
55+
#endif
56+
}
4757

4858
TEST(LoggerTest, SetAndGetDefaultLogger) {
4959
auto capturing = std::make_shared<CapturingLogger>();

0 commit comments

Comments
 (0)