feat: oxidize the emitter by bepri · Pull Request #411 · canonical/craft-cli

bepri · 2026-01-22T15:58:31Z

Have you followed the guidelines for contributing?
Have you signed the CLA?
Have you successfully run make lint && make test?

Oxidizes the emitter class. The PyO3 docs and Maturin docs may help with reviewing. So far, the only intended breaking change is the removal of the global emit. Rust, reasonably so, does not play nicely with globally mutable state or partially initialized variables. If anything else is breaking, it's either because it's not yet implemented or I didn't mean to break it, so definitely ask if you spot anything!

Remaining work:

Streaming brief
Emitter.open_stream
External log handling (from logging)
Devise testing strategy
Incremental progress bars
Fix project versioning (see below)

Overall structure

In order to build with Maturin, the build backend just needed to be switched. This was mostly simple, but Maturin has an unfortunate limitation in that it completely lacks anything like tool.setuptools_scm for configuring tag-based dynamic versioning. This can be mostly achieved via environment variables, but for local development builds with uv (maturin develop --uv), a version must be present in pyproject.toml. For now, the version is hard-coded to 0.0.0+dev.

The field in pyproject setting the module name just makes the compiled .so appear with that import path. Otherwise, it would overwrite the Python portion of Craft CLI. This new submodule was named _rs.

To test what has been done so far, clone this, update your venv, and then run uv run maturin develop --uv. This builds to a .so, then installs it as an editable wheel.

Comments will be made on code in this PR for any other higher level design choices.

CRAFT-4970

bepri · 2026-01-22T15:59:43Z

craft_cli/__init__.py

 # names included here only to be exposed as external API; the particular order of imports
 # is to break cyclic dependencies
-from .messages import EmitterMode, emit  # isort:skip
+from ._rs.emitter import Verbosity as EmitterMode


I want to rename this to Verbosity as it just makes more sense to me, but I didn't want the extra noise of doing that here. I think it's worth it for clarity, but I can back off on that new name if wanted.

bepri · 2026-01-22T16:00:43Z

craft_cli/dispatcher.py

            raise self._build_usage_exc(
                "The 'verbose', 'quiet' and 'verbosity' options are mutually exclusive."
            )
-        if global_args["quiet"]:


This can't be done with the current implementation getting rid of the global emitter. This responsibility should fall on Craft Application anyways, since it already configures other aspects of the emitter.

This change (not having a global emitter) is very, very breaking. There's a lot of code that emits messages in free functions, far away from the application object (example). What's the plan for those?

I've got a few ideas with varying degrees of work involved. I don't want Craft CLI to operate upon a global emitter that it holds at all anymore, if possible.

In an ideal world (but perhaps more work involved), we move closer to how logging works. A new emitter for each file we want to log from, enabling an obvious tie between a given log message and its point of origin.

In a less ideal, but more realistic world, we kick the can down the road and begin creating a global emitter in Craft Application.

The primary issue here is that the current architecture has both Emitter.__init__(), which does effectively nothing, and Emitter.init(), which actually creates a useful emitter. craft_cli.emit does the former, then Craft Application does the latter. Allowing this partial initialization state between the two calls would considerably increase the complexity of the code here, just to accommodate what I would consider to be a poor design choice (global state). In my view, if we punt the issue over to Craft Application, we've at least removed an unnecessary partial initialization state.

bepri · 2026-01-22T16:01:56Z

src/craft_cli_utils.rs

This was the smallest thing I could find to prototype converting into Rust. It ended up being pretty easy, and is probably the better thing to focus on reviewing as an appetizer for the larger modules below.

src/craft_cli_utils.rs

src/emitter.rs

bepri · 2026-01-22T16:07:57Z

src/emitter.rs

+#[derive(Clone, Copy)]
+#[pyclass]
+pub enum Verbosity {
+    /// Quiet output. Most messages should not be output at all.


In Rust, a triple-slash comment is a docstring attached to the next expression it sees (as opposed to a double-slash, which is just a comment). PyO3 parses these and attaches them to __doc__ for the resulting compiled object, so these comments need not be duplicated in the type stubs file (Ruff hates doing that, anyways)

bepri · 2026-01-22T16:10:23Z

src/emitter.rs

+    greeting: String,
+}
+
+#[pymethods]


impl blocks with this decorator are the set of methods exposed to Python on this object.

bepri · 2026-01-22T16:10:40Z

src/emitter.rs

+#[pymethods]
+impl Emitter {
+    /// Construct a new `Emitter` from Python.
+    #[new]


This decorator designates this method as the __init__ method in Python.

bepri · 2026-01-22T16:11:37Z

src/emitter.rs

+    /// Construct a new `Emitter` from Python.
+    #[new]
+    fn new(
+        py: Python<'_>,


The py parameter is a specially-handled field by PyO3, which gives a handle to some of PyO3's internals in a Python-facing function. This parameter does not show up in the function signature on the Python side of things.

bepri · 2026-01-22T16:13:45Z

src/emitter.rs

+    }
+}
+
+impl Emitter {


Since this lacks #[pymethods], these are effectively private methods.

bepri · 2026-01-22T16:14:26Z

src/emitter.rs

+    }
+}
+
+impl Drop for Emitter {


This is how destructors are declared.

src/lib.rs

bepri · 2026-01-22T16:21:33Z

src/printer.rs

+/// Since an mpsc channel is being used, this struct can be arbitrarily cloned,
+/// but should always use an existing `InnerPrinter` rather than constructing its
+/// own new one.


These are guidelines for internal usage, not necessarily something being done now (there isn't yet a need to construct multiple printers, but an idea @lengau had for replacing the global emit object would involve doing this)

bepri · 2026-01-22T16:23:39Z

src/test_utils.rs

@@ -0,0 +1,54 @@
+//! Utilities for testing
+#![cfg(test)]


This file isn't used currently, but I devised them several months ago and they would be very helpful in designing tests in the future. This attribute globally marks the file to only be compiled when building in "test" mode, so this code won't appear even in dev builds.

bepri · 2026-01-22T16:25:34Z

src/utils.rs

+#[allow(unused, clippy::allow_attributes)]
+/// Log a message for debugging purposes only.
+pub fn log(message: impl Into<String>) {
+    #[cfg(debug_assertions)]


The debug_assertions tag means this block will not appear when building in "release mode". The outcome is something similar to typing.cast, wherein this is a no-op (and thus probably optimized out) in prod builds.

I'm choosing to leave it in for development purposes, but it can be removed before the final merge if that's wanted.

Cargo.toml

lengau

Please add cargo clippy as a linter in the Makefile

Cargo.toml

src/craft_cli_utils.rs

Cargo.toml

lengau · 2026-01-22T20:59:22Z

Cargo.toml

+console = "0.16.2"
+indicatif = { version = "0.18.0", features = ["improved_unicode"] }
+jiff = "0.2.15"
+pyo3 = { features = ["extension-module"], workspace = true }


What does it mean that a version isn't listed? (Noble contains 0.20.2, Resolute contains 0.27.2)

See the comment under workspace.dependencies for an explanation here. In brief, workspace = true roughly means "merge these settings with what's defined by the workspace", so it's getting the version from there.

Cargo.toml

src/emitter.rs

pyproject.toml

bepri · 2026-01-26T13:52:25Z

If you're using rustup, try running rustup default stable. This should get you a much newer version of the toolchain (this was all done with the latest, 1.93, but should work starting somewhere around 1.80)

craft_cli/dispatcher.py

tigarmo · 2026-01-26T13:47:52Z

craft_cli/dispatcher.py

            raise self._build_usage_exc(
                "The 'verbose', 'quiet' and 'verbosity' options are mutually exclusive."
            )
-        if global_args["quiet"]:


This change (not having a global emitter) is very, very breaking. There's a lot of code that emits messages in free functions, far away from the application object (example). What's the plan for those?

src/craft_cli_utils.rs

TheSignPainter98

Nice work! Great to see oxidisation in progress :D

Here are some cosmetic notes on the Rust styles with a little general advice as appropriate. Please do take a look at the Canonical Rust Best Practices I wrote for more info on the common style we're trying to adopt and the reasons behind its various points

Also, please don't worry too much about the number of comments I've left, everyone gets the same treatment ;)

src/test_utils.rs

.gitignore

Cargo.toml

rust/craft_cli_utils.rs

src/emitter.rs

src/printer.rs

src/test_utils.rs

src/utils.rs

Copilot

Pull request overview

This PR attempts to rewrite the emitter and printer functionality from Python to Rust for improved performance and reliability. The change involves removing the Python implementation of messages.py and printer.py and replacing them with Rust code compiled to a Python extension module using PyO3 and Maturin.

Changes:

Replaced Python emitter/printer implementation with Rust code using PyO3
Switched build backend from setuptools to Maturin for Rust extension compilation
Removed the global emit instance, requiring users to instantiate Emitter directly
Updated dispatcher to use logging instead of direct emitter calls

Reviewed changes

Copilot reviewed 13 out of 16 changed files in this pull request and generated 20 comments.

Show a summary per file

File	Description
src/emitter.rs	New Rust implementation of the Emitter class with verbosity modes
src/printer.rs	New Rust implementation of threaded message printing infrastructure
src/lib.rs	Main Rust library entry point defining the Python module structure
src/utils.rs	Rust utility functions including import fixes and debug logging
src/craft_cli_utils.rs	Rust implementation of humanize_list utility function
src/test_utils.rs	Testing utilities for Rust unit tests
Cargo.toml	Rust package configuration with dependencies and build settings
Cargo.lock	Rust dependency lock file
pyproject.toml	Updated to use Maturin build backend and hardcoded development version
craft_cli/init.py	Removed emit global export, now only exports EmitterMode (as Verbosity)
craft_cli/dispatcher.py	Attempted to replace emit calls with logging (incomplete)
craft_cli/printer.py	Deleted - replaced by Rust implementation
craft_cli/messages.py	Deleted - replaced by Rust implementation
Makefile	Added Rust tooling and clippy linting
uv.lock	Updated with maturin dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/emitter.rs

pyproject.toml

src/printer.rs

src/emitter.rs

Cargo.toml

src/emitter.rs

src/printer.rs

craft_cli/__init__.py

craft_cli/dispatcher.py

cmatsuoka · 2026-01-27T14:59:50Z

src/craft_cli_utils.rs

+    fn humanize_list(values: Vec<String>, conjunction: Option<&str>) -> PyResult<String> {
+        let conjunction = conjunction.unwrap_or("and");
+        match values.as_slice() {
+            [] => Err(PyValueError::new_err("Cannot humanize empty list")),


Shouldn't a humanized empty list be an empty string?

I suppose it could go either way - the current behavior is to raise a confusing index error:

>>> humanize_list([]) Traceback (most recent call last): File "<python-input-1>", line 1, in <module> humanize_list([]) ~~~~~~~~~~~~~^^^^ File "/home/imani/work/snapcraft/.venv/lib/python3.14/site-packages/craft_cli/utils.py", line 22, in humanize_list return f"{start}, {conjunction} {values[-1]}" ~~~~~~^^^^ IndexError: list index out of range

WDYT @lengau ?

For reference, here's the new behavior:

>>> humanize_list([]) Traceback (most recent call last): File "<python-input-2>", line 1, in <module> humanize_list([]) ~~~~~~~~~~~~~^^^^ ValueError: Cannot humanize empty list

I don't mind whether it's an empty string or a useful ValueError as long as it's not an IndexError.

I think a ValueError makes more sense so we don't say "the three reasons this failed are ."

cmatsuoka · 2026-01-27T15:03:34Z

src/emitter.rs

+    #[pyo3(name = "DEBUG")]
+    Debug,
+
+    /// Trace mode. The absolute maximum amount of information should be printed.


Trace mode should include auto-generated call tracing (which we don't implement right now) but that's the biggest conceptual difference from debug (as it was originally designed).

I see - thanks, that clarifies a lot. To be clear, you mean to say that this behavior isn't currently in Python either?

Rust has a nice tracing crate that does this, as well as some official PyO3 docs suggesting how to implement this.

cmatsuoka · 2026-01-27T19:25:11Z

src/lib.rs

+
+//! Craft CLI
+//!
+//! The perfect foundation for your CLI situation.


For varying definitions of perfect :)

I figured it was high time we start flaunting our prowess. ;)

cmatsuoka · 2026-01-27T20:12:23Z

Does this version make it any easier to expand craft-cli to add support to user input or multi-line output? We should not follow the original implementation too closely to the point that makes this type of new feature very hard (or impossible) to implement.

bepri · 2026-01-27T20:51:05Z

It lands somewhere in-between, I'd say. User input would be trivial to add (and already exists in the Python version - I just haven't added it into this reimplementation yet).

I'm not exactly sure what you mean by multi-line output, but if you mean something similar to what's seen after a cargo clean && cargo build or pacman's multithreaded install output, then yes, this gets us closer. Part of the reason Sergio originally approved the use of the indicatif crate was because it has native support for that.

I do follow the current implementation closer than I'd like, but I'm fearful of moving too far away right now. This is especially due to a lack of tests and the overwhelming caution I'm taking given that Craft CLI is a fairly battle-tested library that is probably avoiding a lot of bugs I can't see.

I think a big issue that would've stopped a major redesign in the Python implementation was that many things were "accidentally" public since we didn't _-prefix them, but Rust pretty firmly enforces privacy so I can very, very easily look and see what is public on the Python side. This should let us modify the internals all we'd like, only worrying about the API.

bepri · 2026-01-27T22:21:21Z

src/craft_cli_utils.rs

+    #[pyo3(signature = (values, *, conjunction = "and"))]
+    fn humanize_list(values: Bound<'_, PyAny>, conjunction: &str) -> PyResult<String> {
+        // Check if it's actually iterable at runtime and collect values
+        // This could be dramatically simplified if [PyO3#5757](https://github.com/PyO3/pyo3/issues/5757)


Linking here for the sake of connections: PyO3/pyo3#5757

tigarmo

nice work, thanks!

TheSignPainter98

Just a couple of last points on the Rust front from me, mostly around the Drop implementations. Otherwise this is looking good

src/craft_cli_utils.rs

src/printer.rs

TheSignPainter98 · 2026-01-30T13:10:14Z

src/emitter.rs

+        {
+            eprintln!(
+                "Unwinding due to panic! The Printer was not stopped correctly. Please report this as a bug: {e}"
+            );


In the case that a panic is already occurring, it's probably best to avoid spamming extra messages in the log---the presence of a panic message implies a bug :) In this case, it's reasonable to use a best-effort approach, panicking if we're not unwinding, otherwise ignoring the error (the one which caused the panic is likely more important):

if let Err(err) = self.printer.stop() && !thread::panicking() { eprintln!("cannot stop printer: {err:?}"); }

TheSignPainter98 · 2026-01-30T13:12:30Z

src/printer.rs

+            if let Err(e) = console::Term::stdout().show_cursor() {
+                eprintln!("Unable to restore text cursor: {e}")
+            }
+        }


This is a little convoluted, would something like my above comments work?

I simplified this a bit, but I'm not sure if it's enough. I still want to try to restore a working state for a terminal if possible

…for progress

bepri requested a review from lengau January 22, 2026 15:58

bepri self-assigned this Jan 22, 2026

bepri requested a review from tigarmo as a code owner January 22, 2026 15:58

bepri commented Jan 22, 2026

View reviewed changes

src/craft_cli_utils.rs Outdated Show resolved Hide resolved

bepri commented Jan 22, 2026

View reviewed changes

src/emitter.rs Outdated Show resolved Hide resolved

bepri commented Jan 22, 2026

View reviewed changes

src/emitter.rs

}

}

impl Emitter {

Copy link

Member Author

bepri Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this lacks #[pymethods], these are effectively private methods.

bepri commented Jan 22, 2026

View reviewed changes

src/emitter.rs

}

}

impl Drop for Emitter {

Copy link

Member Author

bepri Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how destructors are declared.

bepri commented Jan 22, 2026

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

bepri commented Jan 22, 2026

View reviewed changes

Cargo.toml Show resolved Hide resolved

bepri added 3 commits January 22, 2026 11:30

build: add rust build infrastructure

cedbd06

feat: add rust-based emitter class

1ca1d92

feat: incorporate oxidized emitter

b1ffca2

bepri force-pushed the work/oxidize-emitter branch from 6803e30 to b1ffca2 Compare January 22, 2026 16:30

bepri commented Jan 22, 2026

View reviewed changes

Cargo.toml Show resolved Hide resolved

bepri requested a review from cmatsuoka January 22, 2026 19:55

lengau requested changes Jan 22, 2026

View reviewed changes

style: enable linting with clippy

12b4343

lengau reviewed Jan 22, 2026

View reviewed changes

lengau self-requested a review January 22, 2026 21:46

lengau reviewed Jan 22, 2026

View reviewed changes

pyproject.toml Show resolved Hide resolved

tigarmo reviewed Jan 26, 2026

View reviewed changes

TheSignPainter98 reviewed Jan 26, 2026

View reviewed changes

bepri added 4 commits January 27, 2026 09:19

chore: rename source directory from rust to src

34c10aa

fix: use a tuple of verbosities instead of frozenset

ed56f86

feat: use dirs instead of xdg for log filepath calculation

f7d1e3c

chore: remove extraneous whitespace in gitignore

a187e35

cmatsuoka requested a review from Copilot January 27, 2026 14:45

Copilot started reviewing on behalf of cmatsuoka January 27, 2026 14:46 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

cmatsuoka reviewed Jan 27, 2026

View reviewed changes

style: incorporate @TheSignPainter98's feedback

3b14807

chore: incorporate copilot feedback

2f5343e

bepri added 2 commits January 27, 2026 16:57

feat: finalize the humanize_list implementation

b9f0d35

docs: add comment indicating point of improvement

2f0fc17

bepri commented Jan 27, 2026

View reviewed changes

bepri requested review from TheSignPainter98, cmatsuoka, lengau and tigarmo January 29, 2026 14:52

tigarmo approved these changes Jan 30, 2026

View reviewed changes

TheSignPainter98 suggested changes Jan 30, 2026

View reviewed changes

bepri added 4 commits January 30, 2026 13:18

chore: clean up error cases

bf1fd9a

feat: add handling for warning and debug, allow dynamic destinations …

c0ebaa2

…for progress

perf: use buffered writing

d072721

fix: don't print execution log location every time

e885309

bepri mentioned this pull request Feb 3, 2026

feat: oxidize open_stream #416

Draft

3 tasks

Conversation

bepri commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overall structure

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lengau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bepri commented Jan 26, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheSignPainter98 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

bepri commented Jan 22, 2026 •

edited

Loading

TheSignPainter98 left a comment •

edited

Loading

cmatsuoka Jan 27, 2026 •

edited

Loading