Add logging features to the Rust side - #5094
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5094 +/- ##
==========================================
+ Coverage 73.13% 73.36% +0.24%
==========================================
Files 331 332 +1
Lines 17720 17939 +219
==========================================
+ Hits 12958 13160 +202
- Misses 4762 4779 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0f018c0 to
08a43b5
Compare
The CXX bindings are present in every CXX crate, but linking to the CXX crate just for the bindings creates an impression of a circular dependency that is not correct. The empty crate grants access to the bindings while being explicit about the intent.
12d21a4 to
2b98769
Compare
Race condition only on windows, potentially in other platforms as well
Let cargo run in all builds to let cargo itself check for file changes in the crates. Also, make the custom command job server aware.
There was a problem hiding this comment.
Pull request overview
Adds a Rust-side logging facility for Multipass by introducing a small CXX-based FFI bridge that forwards Rust log calls into the existing C++ logging backend.
Changes:
- Introduces new Rust crates (
rslogger,rust) and workspace updates to generate CXX glue and enable Rust-to-C++ logging calls. - Extends the C++ logging module with a Rust-facing entry point and updates build/link wiring (CMake + Cargo).
- Adds a C++ unit test exercising Rust->C++ logging and exception propagation.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_rust_integration.cpp | Adds integration tests for Rust logger forwarding and exception propagation. |
| tests/unit/CMakeLists.txt | Links the new rslogger bridge into the unit test binary. |
| src/logging/log.cpp | Implements the Rust-facing logging shim that forwards into multipass::logging::log_message. |
| src/logging/CMakeLists.txt | Links the C++ logging library against the Rust/CXX bridge target. |
| include/multipass/logging/log.h | Declares the Rust-facing logging entry point and adds Rust CXX header include. |
| rxx/CMakeLists.txt | Adds new crates to the Rust workspace build and adjusts the custom command invocation. |
| rxx/Cargo.toml | Registers new workspace members and adds the named-lock dependency. |
| rxx/Cargo.lock | Updates lockfile for new crates/dependencies (including named-lock and transitive deps). |
| rxx/namegen/Cargo.toml | Adds named-lock as a build dependency. |
| rxx/namegen/build.rs | Serializes CXX codegen with a named lock to avoid concurrent build issues. |
| rxx/rslogger/src/log.rs | Adds Rust logging helpers/macros and location-aware formatting. |
| rxx/rslogger/src/lib.rs | Defines the CXX bridge for logging + test-only Rust function exposed to C++. |
| rxx/rslogger/CMakeLists.txt | Links the rslogger crate against the C++ logger module. |
| rxx/rslogger/Cargo.toml | Declares the rslogger crate and its build dependencies. |
| rxx/rslogger/build.rs | Generates CXX bridge code under a named lock. |
| rxx/rust/src/lib.rs | Adds an (empty) generic CXX bridge crate for exposing Rust/CXX bindings to C++. |
| rxx/rust/CMakeLists.txt | Adds crate-level CMake stub (currently only header/license). |
| rxx/rust/Cargo.toml | Declares the rust crate used for building/linking CXX glue. |
| rxx/rust/build.rs | Generates CXX bridge code under a named lock. |
Comments suppressed due to low confidence (1)
tests/unit/test_rust_integration.cpp:92
- Same issue as above: structured binding copies the shared_ptr out of MockLogger::Scope, preventing Scope::~Scope() from resetting the global logger due to the use_count()==2 guard.
auto [mock_logger] = mpt::MockLogger::inject();
EXPECT_CALL(*mock_logger, log).WillOnce(Throw(std::runtime_error{"Exception"}));
EXPECT_THROW(rxt::logging::test_log(mpl::Level::warning, "category", "test_log"), rust::Error);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto [mock_logger] = mpt::MockLogger::inject(); | ||
|
|
||
| mock_logger->screen_logs(mpl::Level::warning); | ||
| mock_logger->expect_log(mpl::Level::warning, "test_log"); | ||
| rxx::test::logging::test_log(mpl::Level::warning, "category", "test_log"); |
There was a problem hiding this comment.
Is the additional shared_ptr not within the same scope @copilot?
There was a problem hiding this comment.
It’s in the same lexical block, but auto [mock_logger] = ... still creates an extra shared_ptr owner outside Scope::mock_logger. That extra owner can make use_count() higher when Scope::~Scope() runs, so the use_count()==2 guard may not pass and the global logger reset can be skipped. Keeping the Scope object explicit and using scope.mock_logger avoids that extra ownership ambiguity.
There was a problem hiding this comment.
Is this not the usage in other tests @copilot?
|
|
||
| void mpl::rust::log_message(Level level, ::rust::String category, ::rust::String message) | ||
| { | ||
| mpl::log_message(level, category.c_str(), message.c_str()); |
| add_custom_command( | ||
| OUTPUT ${CARGO_GENERATED_FILES} | ||
| OUTPUT ${CARGO_GENERATED_FILES} "${CMAKE_CURRENT_BINARY_DIR}/_force_cargo_rebuild" | ||
| COMMAND ${CARGO_CMD} build |
There was a problem hiding this comment.
The intent is to force cargo to re-run on every build. WDYT @copilot ?
There was a problem hiding this comment.
The intent makes sense, but relying on an undeclared/missing OUTPUT is fragile (especially with Ninja). If we want cargo to run every build, it’s cleaner to model it as a phony custom target (no OUTPUT) or explicitly touch a stamp file listed in OUTPUT.
| @@ -42,6 +44,11 @@ void set_logger(std::shared_ptr<Logger> logger); | |||
| Level get_logging_level(); | |||
| Logger* get_logger(); // for tests, don't rely on it lasting | |||
|
|
|||
| namespace rust | |||
| { | |||
| void log_message(Level level, ::rust::String category, ::rust::String message); | |||
| } | |||
| match ffi::log_message(level, category, message) { | ||
| Ok(()) => (), | ||
| Err(e) => { | ||
| println!("Log message exception: {e}"); | ||
| } |
There was a problem hiding this comment.
Unwrap may make more sense here
Description
Testing
Unit tests
Manual testing steps:
namegencrateChecklist
Additional Notes
MULTI-2717