Repository navigation
Conversation
The library prints progress bars and messages directly to the shell, so callers like boulder can't control how moss output looks, and library users have nothing typed to work with. Add an event module where the library reports typed tasks to an observer provided by the caller, so each caller can decide how to present them.
Port `pkg fetch` as the first user of the event module. The CLI renders events through a new shell module, picked with `--format`: * human: the existing progress bars and messages (default) * json: one event per line on stdout
Indexing moves through phases on two levels: each package is hashed then read, and the whole index is read then written. Add task phases to report these, instead of changing progress bar messages in the library.
Each extracted package is reported as its own task. Packages without layout records are skipped rather than worked on, so add task details to report them as a fact about the overall extraction.
Blitting is shared by every operation that writes a filesystem tree, so it shouldn't decide where it sits in the task tree. Add child sinks, which nest anything started from them under a task, and report blit on the sink it's given. Extract passes each package's child sink, so its blits show up nested under that package.
Progress can update thousands of times a second, far more often than is useful to consumers. Write it at most 10 times a second per task, always writing the latest progress before any other update for that task so consumers never see stale progress.
Since tasks know their parent, the CLI can show where time went as a task tree once the command finishes. The report goes to stderr so it combines with any `--format` without mixing into stdout. Observers are combined with a new `Tee`, and the CLI now creates its observer once and hands it to each command.
Document and outline the design examples the previous commits arrived at: moss reports typed task events to an observer the caller provides, instead of printing to the shell. It also outlines a suggestion for where this design proposal might go next. Signed-off-by: Rune Morling <ermo@aerynos.com>
... at the end to flesh out the idea a bit more. Signed-off-by: Rune Morling <ermo@aerynos.com>
There was a problem hiding this comment.
Overall, I think the design and direction make sense to me! I'm still unfamiliar with the internal workings of moss, so I fear most of my comments aren't actually super insightful, but I've left a few as I was reading through things!
Following The Pragmatic Programmer's advice about "tracer bullets", perhaps I should start by getting some small part of this new system working end-to-end? Maybe my first PR could just be refactoring progress reporting? Moving from the tracing event_type = "progress_*" thing to something typed?
EDIT: Maybe that's not the best approach, actually. Perhaps it's better how you seem to have divided your commits — I start by doing all of the reporting for a single moss command?
I can take inspiration from this RFC, simplify or tweak it where that seems helpful, and as we do more of the transition and need richer functionality, we can add that in (continuing to draw inspiration from this RFC)?
| ## Summary | ||
|
|
||
| This proposal outlines how the moss library can stop printing and instead report | ||
| **typed tasks** to an `Observer` that the caller provides. |
There was a problem hiding this comment.
Boring bikeshedding, but I needed to look up Observer patterns to make sure I knew what they were — I'm a bit more familiar with them being called Publishers and Subscribers (PubSub), but it seems people use these terms somewhat interchangeably: https://refactoring.guru/design-patterns/observer/rust/example
Ultimately, I'm fine with whichever :)
| pub trait Task { | ||
| type Phase; // what it is doing now | ||
| type Detail; // facts found along the way | ||
| type Outcome; // what it came to |
There was a problem hiding this comment.
Sorry, naming again, Outcome had me thinking this would be something like a Result — it would be an enum that captures both the successful and unsuccessful outcomes, but it seems like it's only returned when the task completes successfully?
Maybe Output could be a good alternative name?
|
|
||
| impl Task for IndexPackage { | ||
| type Phase = IndexPackagePhase; | ||
| type Detail = (); |
There was a problem hiding this comment.
It would be lovely to not have to specify these = () things every time — alas, associated Rust types are still an RFC: https://rust-lang.github.io/rfcs/2532-associated-type-defaults.html
| let total = index_packages.start_progress(Some(count), task::Unit::Items); | ||
|
|
||
| let index_package = index_packages.start_subtask(task::IndexPackage { path }); | ||
| index_package.enter_phase(task::IndexPackagePhase::Hashing); |
There was a problem hiding this comment.
It's hard to tell if this would be over-designed or not, but perhaps these phases could follow a typestate pattern? https://blog.cesc.cool/implementing-the-state-pattern-in-rust
Something that ensures the phases come in the right order and that all phase / state transitions are valid at compile time?
If moss is the only code that will be moving between phases though, then that might be overkill...
| let index_package = index_packages.start_subtask(task::IndexPackage { path }); | ||
| index_package.enter_phase(task::IndexPackagePhase::Hashing); | ||
| let bytes = index_package.start_progress(Some(size), task::Unit::Bytes); | ||
| // bytes.inc(n) as we hash... |
There was a problem hiding this comment.
Do you have a preference as to where this should be throttled?
Will bytes.inc be called in a hot loop and its the job of the type to only dispatch updates at a given Hz? That would probably be the nicest API, but perhaps performance intensive? Would need testing, I suppose.
|
|
||
| Rules: | ||
|
|
||
| * **Called inline, so it must return quickly.** moss downloads on a single-thread runtime; a |
There was a problem hiding this comment.
My own curiosity: is downloading likely to be multi-threaded in the future?
There was a problem hiding this comment.
Brooks from the future: downloading happens concurrently, so this isn't the limitation I thought it was
| let observer = Arc::new(observer::Tee::new([format.observer(), timing.clone()])); | ||
| ``` | ||
|
|
||
| Rules: |
There was a problem hiding this comment.
Could be worth thinking about how many of these can be somehow enforced via the design!
| /// Some operations, such as cloning into a container, need the process to be single | ||
| /// threaded. Any thread an observer starts must stop by the time the task it was | ||
| /// started for finishes, so slow work should be buffered and done once moss returns. | ||
| pub trait Observer: Send + Sync { |
There was a problem hiding this comment.
Instead of a trait and things like a Noop implementation, would something more like the observer.rs on https://refactoring.guru/design-patterns/observer/rust/example make sense?
There is just one struct that receives all of moss's events, but only dispatches to "subscribers" that are looking for something in particular? No more Noop, just an empty HashMap or whatever.
| pub count: u64, | ||
| } | ||
|
|
||
| impl Task for FetchPackages { |
There was a problem hiding this comment.
I'll need to look at the code for a lot longer to know for sure, but I suspect this Task API might be nicer if Task were an enum instead of a trait?
A trait makes sense if users of the moss library crate will be defining their own task types, but I suspect that that's not the case. If that's true, and only the moss lib should be implementing this trait, then perhaps it doesn't need to be a trait at all!
I'd have to play around with the code to know for sure...
Tarkah did some R&D work on his end and came up with a proposal to unblock us. He says he used an LLM to help him overcome his own burnout on the API design structure, but also stressed that this is only meant as a suggestion w/ a set of code examples for discussion.
The idea with the proposal and the included RFC is to align on what the RFC should look like for real, and then let the code PRs follow the API in small, bite-sized commits that we can each put up when we have the free time to do so.
Again, note that this proposal is not intended to be merged directly. It is only here as a reference for discussion.