New logger and remove code bloat#443
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the loader’s spdlog-based logging with a built-in logger implementation, removes the vendored spdlog headers and build wiring, and updates loader/layer integration to share a single logger instance.
Changes:
- Remove vendored spdlog headers (and related CMake configuration) to eliminate external logging dependencies.
- Introduce
ZeLogger(source/utils/ze_logger.*) and migrate loader + validation layer to use it (including logger sharing between loader and validation layer). - Update documentation and version metadata to reflect the new logging behavior/configuration.
Reviewed changes
Copilot reviewed 64 out of 66 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| third_party/spdlog_headers/spdlog/version.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/tweakme.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/spdlog.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/spdlog-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/sinks/wincolor_sink.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/sinks/wincolor_sink-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/sinks/sink.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/sinks/sink-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/sinks/basic_file_sink.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/sinks/basic_file_sink-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/sinks/base_sink.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/sinks/base_sink-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/sinks/ansicolor_sink.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/sinks/ansicolor_sink-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/pattern_formatter.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/pattern_formatter-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/mdc.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/logger.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/logger-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/formatter.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/fmt/fmt.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/windows_include.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/synchronous_factory.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/registry.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/registry-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/periodic_worker.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/periodic_worker-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/os.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/os-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/null_mutex.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/log_msg_buffer.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/log_msg_buffer-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/log_msg.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/log_msg-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/fmt_helper.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/file_helper.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/file_helper-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/console_globals.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/circular_q.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/backtracer.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/details/backtracer-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/common.h | Remove vendored spdlog header |
| third_party/spdlog_headers/spdlog/common-inl.h | Remove vendored spdlog header |
| third_party/spdlog_headers/README.md | Remove spdlog vendoring note |
| third_party/spdlog_headers/LICENSE | Remove spdlog license file (dependency removed) |
| third_party/spdlog_headers/.version | Remove spdlog version pin file |
| source/utils/ze_logger.h | Add new logger public interface |
| source/utils/ze_logger.cpp | Add new logger implementation + env-driven factory |
| source/utils/logging.h | Remove old spdlog-based Logger wrapper |
| source/utils/logging.cpp | Remove old spdlog-based to_string implementation file |
| source/utils/CMakeLists.txt | Build utils with ZeLogger sources (remove spdlog wiring) |
| source/loader/ze_loader_internal.h | Switch loader context to ZeLogger |
| source/loader/ze_loader_api.h | Add exported accessor for shared logger |
| source/loader/ze_loader_api.cpp | Implement exported accessor for shared logger |
| source/loader/ze_loader.cpp | Use new createLogger, improve debug trace path reporting, update version logging |
| source/lib/ze_lib.h | Switch includes to ZeLogger (add missing STL include) |
| source/lib/ze_lib.cpp | Improve debug trace message when loader path not set |
| source/layers/validation/ze_validation_layer.h | Switch validation layer to ZeLogger |
| source/layers/validation/ze_validation_layer.cpp | Add loader-provided logger injection API + no-op sentinel logger |
| scripts/templates/ze_loader_internal.h.mako | Remove spdlog include from template (but still references logging.h) |
| README.md | Update user documentation for new built-in logging behavior |
| PRODUCT_GUID.txt | Bump product version/GUID |
| CMakeLists.txt | Remove global spdlog discovery/include configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
64de0c9 to
2233385
Compare
|
Took a look at the removed code and some of the additions, looks good. Will continue reviewing tomorrow probably |
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
+ Also update missing ZE_RESULT codes from older logger Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
Fix logger debug outout showing which actual files are loaded after path resolution. Without this not useful for debugging misconfigured systems. Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
+ Change from a pull to a push model, to push the logger instance from the loader to the validation layer to remove duplication. + Remove resource consumption when no logger is used Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
+ Removed references in mako files to old logger + Removed some dead code + Fixed recursive log directory creation corner case Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
| void log_fatal(const std::string &msg) { critical(msg); } | ||
| void log_performance(const std::string &msg) { warn("[performance] " + msg); } | ||
|
|
||
| // When true, callers may mirror certain messages to stdout/stderr. |
There was a problem hiding this comment.
just double-checking, is there actual mirroring of messages? The README says it's an either/or
There was a problem hiding this comment.
I second, before the logger either sent everything to a file or sent it to the console, but not both.
Ideally we would match that behavior such that we don't break compatibility with the previous behavior for now.
There was a problem hiding this comment.
Sounds good.. I will push a change here shortly going back to original limitation
* Add Ubuntu specific .deb packages to CPack * Fix duplicate install when adding Canonical packages + Fix missing Python3 warning in CI Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
* Fix typo in build debug command line +Include Lambda's in symbol resolution +Disable link time optimization to help ABI -fno-lto Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
-Remove verbage and boolean for optional stderr mirroring -Replace fd 2 w/define Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 72 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::cerr << debugTracePrefix << message << result << std::endl; | ||
| } | ||
|
|
||
| zel_logger->log_trace(message + result); |
There was a problem hiding this comment.
ZE_ENABLE_LOADER_DEBUG_TRACE is documented to print messages to stderr with the ZE_LOADER_DEBUG_TRACE: prefix, but debug_trace_message() now only calls zel_logger->log_trace(...). When logging is disabled (the default), createLogger() returns a no-op logger and debug tracing becomes silent, and the stderr prefix behavior is lost. Restore the unconditional stderr path (or force a stderr logger when debug tracing is enabled) to preserve the advertised debug-trace behavior.
| zel_logger->log_trace(message + result); | |
| const std::string traceMessage = message + result; | |
| std::cerr << "ZE_LOADER_DEBUG_TRACE: " << traceMessage << std::endl; | |
| zel_logger->log_trace(traceMessage); |
There was a problem hiding this comment.
To explain more clearly we have this in the https://github.com/oneapi-src/level-zero#debug-trace in the README.md, which says it will override the DEBUG_TRACE to be on stderr... Hence the code had the option if a normal ZEL_LOADER_LOGGING_LEVEL was enabled to a file and ZE_LOADER_DEBUG_TRACE was enable also, you'd effectively do both at the same time. This gets complicated, and I think we should fix this, to be standardized under ZEL_LOADER_LOGGING
Signed-off-by: Russell McGuire <russell.w.mcguire@intel.com>
This removes all dependencies on external code for logging purposes.
Removes all static globals resulting in gnu unique symbols
Retains external interface behaivor