diff --git a/README.md b/README.md index b930488..304ddd2 100644 --- a/README.md +++ b/README.md @@ -2,11 +2,13 @@ A monolithic kernel work wrapper, to make it easy. -> **Status: scaffold.** The command surface parses, every verb is wired -> through `main` to its module, and the paths resolve. Every body is a -> `todo!()` stub. Nothing builds a kernel, boots qemu, scrapes bugzilla, or -> sends mail yet. What exists is the shape of the tool and the seams to fill -> in, and *Where to pitch in* below is the list. +> **Status: the loop closes.** Every verb below runs. The chain from a random +> open bug to a patch that `git send-email` accepts has been walked end to end, +> including the v2 re-roll threaded off the original. `kernel test` builds a +> tree and boots it under qemu, `doctor` says what is missing before you start, +> and `init` writes the config file. `Next` in `ROADMAP.md` has landed; what is +> left is `Later` and `Far`, plus the three gaps named under *What is not +> proven* below. ## Why @@ -32,10 +34,12 @@ What each verb actually ends up running: |---|---| | `patch check` / `patch format` | `scripts/checkpatch.pl` **from your tree** | | `patch commit` | `git commit` | -| `patch create` | `git diff ` | +| `patch create` | `git format-patch` | +| `patch update` | a rename, or `git format-patch --reroll-count` for a series | | `patch submit` | `scripts/get_maintainer.pl` **from your tree**, then `git send-email` | | `kernel test` | `make`, then `qemu-system-x86_64` | -| `kernel quest` | HTTP against `bugzilla.kernel.org` | +| `kernel quest` | the bugzilla.kernel.org REST API | +| `doctor` | git, the tree's own scripts, qemu, and the paths | checkpatch.pl and get_maintainer.pl are worth calling out. They come from the tree you are working in, so spectral cannot drift out of date with the @@ -49,17 +53,19 @@ the command yourself. ## The loop -What the finished tool should feel like. None of this runs yet: +What the finished tool feels like. Every line below has been run: ```console $ spectral kernel quest 12345 usb: xhci: device does not enumerate after resume +component: Drivers/USB https://bugzilla.kernel.org/show_bug.cgi?id=12345 # ... go and fix it, in the tree ... $ spectral patch check $ spectral patch format # checkpatch --fix, then check again +$ git add -A # `patch commit` commits what you staged $ spectral patch commit "usb: xhci: re-arm the port after resume" --signoff $ spectral kernel test # build, then boot it under qemu $ spectral patch create 000-xhci-port-rearm @@ -75,11 +81,35 @@ $ spectral patch update 000-xhci-port-rearm.patch -v 2 $ spectral patch submit v2-000-xhci-port-rearm.patch --in-reply-to '' ``` +Staging is yours on purpose. `patch commit` runs `git commit`, so what goes in +is what you put there; spectral will not sweep a dirty tree into your patch, +because a stray file in a kernel patch is a review comment you did not want. + +For a set rather than a single patch: + +```console +$ spectral patch create xhci-port-rearm --range master..HEAD \ + --cover-letter "Re-arm the port after resume" +~/.spectral/patches/xhci-port-rearm +$ spectral patch submit xhci-port-rearm --dry-run +$ spectral patch update xhci-port-rearm # -v is inferred; every file moves +``` + +The series directory holds the patches plus a `series.toml` recording the range +it was generated for (both ends as hashes), the revision, the file list and the +cover letter, which is what lets a re-roll happen without retyping the range. +Re-rolling regenerates exactly that range: a commit that landed on the branch +since `create` does not join the set, because a re-roll should not mail commits +nobody asked to send. The blurb's first line becomes the cover letter's subject. +Leave `--cover-letter` off and the set is written with git's template subject in +it, which `git send-email` refuses: an unsendable series is better than a cover +letter nobody wrote. + One deviation from the obvious design: `patch update` is a single subcommand taking `-v N` rather than a generated `update-v2` / `update-v3`. clap subcommands are static, and a flag beats a subcommand you cannot tab-complete. Leave `-v` off and it infers the next revision from the file's current `vN-` -prefix. +prefix, or from the sidecar for a series. ## Install @@ -108,26 +138,58 @@ You also need: ## Configuration -There is no config file yet. Two paths matter, and both come from the -environment with `$HOME`-relative defaults: +Two paths and a couple of settings, resolved in this order: the environment, +then `~/.config/spectral/config.toml`, then the built-in default. -| what | resolution | -|---|---| -| kernel tree | `$SPECTRAL_KERNEL`, else `~/.spectral/linux` | -| patch dir | `~/.spectral/patches` | +| what | environment | file key | default | +|---|---|---|---| +| kernel tree | `$SPECTRAL_KERNEL` | `kernel_tree` | `~/.spectral/linux` | +| patch dir | `$SPECTRAL_PATCH_DIR` | `patch_dir` | `~/.spectral/patches` | +| always CC | | `always_cc` | none | +| send-email identity | | `send_email.from` | git's own `user.email` | ```console $ export SPECTRAL_KERNEL=$HOME/src/linux +$ spectral init --tree $HOME/src/linux # or --clone [--depth N] +wrote /home/you/.config/spectral/config.toml +tree: /home/you/src/linux ``` +`init` verifies the tree before it writes anything, and refuses to overwrite a +config that points somewhere else unless you pass `--force`. Run it twice with +the same tree and the second run says so and changes nothing. + A tree is accepted only if it has `scripts/checkpatch.pl`. Otherwise you get `not a kernel source tree` rather than a confusing failure three steps later. A missing tree reports the path and the variable that would have set it. The tree is not needed for `kernel quest`, which only wants the network. -`~/.config/spectral/config.toml` and a `spectral init` to clone the tree are -planned, not built. `src/config.rs` is the only file that has to change to -add either. +A path in the config file has to be absolute, and an unknown key is an error +rather than a shrug: a mistyped setting that silently does nothing is worse +than one that stops the run and says which line is wrong. + +Run `spectral doctor` to see what got resolved and from where: + +```console +$ spectral doctor +ok kernel tree /home/you/src/linux (from the config file) +ok kernel tree shape scripts/checkpatch.pl is there +ok checkpatch.pl scripts/checkpatch.pl (the only thing that decides kernel style) +ok get_maintainer.pl scripts/get_maintainer.pl (the only thing that knows who to mail) +ok MAINTAINERS the maintainer map the script reads +ok git git version 2.55.0 +ok git send-email runs +ok sender identity you@example.com +warn mail route no sendemail.* config: --dry-run works, a real send will not + -> configure `git send-email` before dropping --dry-run +ok qemu QEMU emulator version 11.1.1 +ok patch directory /home/you/.spectral/patches (exists, the built-in default) +ok config file /home/you/.config/spectral/config.toml +doctor: nothing failed, 1 warned +``` + +A warning keeps the exit code at 0; a failed check exits 1 and the summary names +the first one, because that is the one to fix first. ## Commands @@ -140,75 +202,81 @@ Usage: spectral Commands: kernel Find work, build it, boot it patch Carry a change from working tree to mailing list + doctor Check the tree, the scripts, the mail setup and the paths + init Point the config at a kernel tree, verifying or cloning one help Print this message or the help of the given subcommand(s) ``` -`spectral kernel quest` · `kernel test` +`spectral kernel quest`, `kernel test` -`spectral patch check` · `format` · `commit` · `create` · `submit` · `update` +`spectral patch check`, `format`, `commit`, `create`, `submit`, `update` -Every one of them has `--help` that says more than this README does, and for -now a body that panics with a description of what it is supposed to do. +`spectral doctor`, `init` + +Every one of them has `--help` that says more than this README does. + +## What is not proven + +Named rather than papered over, because a thin wrapper is only worth trusting +where it says where it stops: + +- **Real mail is unverified.** No patch from this project has been sent to a + mailing list by spectral. The acceptance evidence is `git send-email + --dry-run` reaching `Dry-OK` with the recipients spectral chose. +- **A full kernel build is not part of the test suite.** `kernel test`'s build + step is exercised against a Makefile that writes the image; the boot is + exercised against a real qemu with a real `bzImage`, but nothing here builds + a kernel from source, because that is hours of work that proves nothing about + this crate. +- **`patch format`'s fix round trip is proven against a made-up patch**, not + against a real kernel posting whose damage checkpatch can repair. + +## Tests + +```console +$ cargo test --locked +``` + +No test needs a kernel tree or a network, and none needs `git send-email` +configured. Each one builds a throwaway kernel-shaped git repository in a temp +directory, points `HOME`, `SPECTRAL_KERNEL` and git's global config at it, and +drives the built binary through `CARGO_BIN_EXE_spectral`. The fixture's `PATH` +and git config are the test's, not the machine's, which is how a missing qemu, a +missing sender identity or a missing `git send-email` is produced deliberately +instead of inherited. + +One host tool is assumed: git with its `send-email` support. The `doctor` tests +wrap it so a machine without that support still passes, and the send-email flow +tests use the real one, because that is the tool under test. + +Three tests do want the host's own tools, and they are ignored unless asked for: + +```console +$ SPECTRAL_REAL_CHECKPATCH=1 SPECTRAL_REAL_TREE=/usr/src/linux \ + cargo test --locked -- --ignored real_checkpatch +$ SPECTRAL_REAL_GETMAINTAINER=1 SPECTRAL_REAL_TREE=/usr/src/linux \ + cargo test --locked -- --ignored real_get_maintainer +$ SPECTRAL_LIVE=1 cargo test --locked -- --ignored live_bugzilla +``` + +They are where a kernel-version change to checkpatch's summary line, or to +get_maintainer's role words, or to bugzilla's REST answer, shows up as a test +failure rather than inside someone's patch workflow. ## Where to pitch in -Every stub is a small function that already has its signature, its doc -comment, and its caller in `main`. Pick one and you only need to read the -file it lives in. +The verbs are written and the loop closes. What is left is in `ROADMAP.md`: +`Later` holds more quest sources, more checks, review tracking and packaging; +`Far` holds the TUI and `spectral auto`. -Roughly in order of how much they unblock. +Two of those are more useful than they look. `kernel/quest.rs` is behind the +`QuestSource` trait, so a syzbot source is one impl and no change to the +command. `patch/series.rs` holds the sidecar a series carries, and review +tracking is the feature that will want to add to it. -### patch submit - -The most self-contained win, and the only one that needs no network. It does -need a kernel tree and a working `git send-email`. - -- `patch/maintainers.rs` · `lookup` runs `get_maintainer.pl --git` over the - patch, splits the output into `To:` and `Cc:`, and collects the files the - patch touches. -- `patch/maintainers.rs` · `add_cc` folds `--cc` flags in without duplicates. -- `patch/mod.rs` · `submit` ties those two together with `git send-email`, - with `--dry-run` stopping one step short. - -### patch check, format, commit, create - -The everyday verbs. `patch/checkpatch.rs` · `run` already has the `Target` -enum (working tree, a revision, or a patch file) and `Report` with -`is_clean`. What is missing is the process call and pulling the error and -warning counts out of checkpatch's output. - -### patch update - -Small, and worth writing a test for. `patch/mod.rs` · `reroll_path` holds the -naming rule in one pure function: strip one leading `vN-`, then prepend -`v-`. Running it twice at the same revision should do nothing, -which is easier to assert than to explain. - -### kernel quest - -The fun one if you like HTML. `kernel/quest.rs` · `Bugzilla::fetch_open` is -the only place `reqwest` and `scraper` earn their place in `Cargo.toml`, and -`run` then applies `--filter` and picks one issue. The `QuestSource` trait is -where you would add syzbot or a lore.kernel.org thread later. - -### kernel test - -Needs a machine you are willing to boot kernels on. `kernel/qemu.rs` · -`build` then `boot`. Streaming serial output as it arrives beats buffering it -until qemu exits, and it is the difference between a boot hang you can -diagnose and one you cannot. - -### git plumbing - -`git.rs` · `current_branch`, `diff_against`, `commit`, `rev_parse`. Each is a -few lines over the `run` that is already written, which makes these the -easiest place to start. - -There are no tests yet. The naming rule in `reroll_path` and the recipient -split in `lookup` are the two that most want one. - -Longer-term work, and the things spectral deliberately will not do, are in -`ROADMAP.md`. +The three gaps at the end of *What is not proven* are honest small tasks: a +real send, a real kernel build under `kernel test`, and the fix round trip +against a real kernel patch. ## Development @@ -216,9 +284,10 @@ Longer-term work, and the things spectral deliberately will not do, are in $ cargo fmt --all $ cargo clippy --all-targets -- -D warnings $ cargo test +$ cargo doc --no-deps ``` -All three are clean on `main` and are the bar for a change. +All four are clean on `main` and are the bar for a change. A few conventions: @@ -228,12 +297,16 @@ A few conventions: - Command modules return what they produced and `main` prints it, so output formatting lives in one place. - Errors go through the one `Error` enum and `?`. Nothing in the command - paths calls `unwrap`. -- A new stub gets a `todo!()` naming the command it will end up running. A - module that is unreachable until its caller exists carries a single - `#[allow(dead_code)]` with the reason attached. There are no crate-wide - allows, so the warnings come back as the stubs get filled in. -- No `unsafe` and no FFI. + paths calls `unwrap`. An external command that fails reports what it said, + from stdout as well as stderr, because git puts some refusals on stdout. +- Every module that shells out has a comment saying why that tool and not + another API: `checkpatch.pl` and `get_maintainer.pl` come from the tree, and + `git format-patch` does the series naming because that is already its job. +- No `unsafe` and no FFI. `std::env::set_var` is `unsafe` under edition 2024, + which is why environment-dependent behaviour is tested through the binary + rather than in a unit test. +- No `todo!()` is left in the command path. If you add one, say in the test + suite what will prove it gone. ## License diff --git a/ROADMAP.md b/ROADMAP.md index 5a4e84a..dac130b 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -6,61 +6,73 @@ ranges from one afternoon to a design conversation first. ## Now -Implement the stubs. `README.md` lists them grouped by how much they -unblock, and `patch submit` is where most people should start, since it is -the only verb that needs no network. +Done. Every verb runs, and the loop closes end to end: a commit went from a +working tree to a `git send-email` plan, then round again as v2 threaded off +the original. `README.md` has the walkthrough and the acceptance evidence. -The milestone worth having is the loop closing: +The milestone was this: ```text quest -> edit -> check -> format -> commit -> create -> submit -> v2 ``` One patch through one review cycle without dropping to a shell to run git or -checkpatch by hand. Until that works, nothing else on this page is worth -building. +checkpatch by hand. The one step that stays yours on purpose is `git add`: +`patch commit` commits what you staged, because sweeping a dirty tree into a +kernel patch is a review comment nobody asked for. -Two tests belong here too, both named in the README: the `vN-` naming rule in -`reroll_path`, and the `To:`/`Cc:` split in `lookup`. +The two tests this section named are in place: the `vN-` naming rule in +`reroll_path`, and the `To:`/`Cc:` split in `lookup`, the second of which is +also asserted against the host's real `get_maintainer.pl` in an opt-in test. + +`rg -n 'todo!' src/` finds nothing. What is left overall is the three gaps in +the README's *What is not proven*, and `Later` and `Far` below. ## Next +All three landed, in this order. The sections below say what each one does now. + ### A series instead of a single patch -Everything today produces one file. Real submissions are often a patch set -with a cover letter, which means: +Done. `patch create xhci-port-rearm --range master..HEAD --cover-letter +""` writes a numbered set through `git format-patch`, cover letter +included; `patch submit` takes a directory or several files and sends them in +one `git send-email` invocation; `patch update ` re-rolls the whole set +through `--reroll-count`, so every file moves to vN together, and the +superseded files go only once the new revision is complete. The blurb's first +line becomes the cover letter's subject, and leaving `--cover-letter` off +leaves git's template in place, which `git send-email` refuses. -- `patch create` writing a numbered set from a commit range, and a - `0000-cover-letter.patch` alongside it -- `patch submit` taking several files at once, which `git send-email` - already supports -- `patch update` re-rolling the whole set, so every file in it moves to v2 - together, the way `git format-patch --reroll-count` does it -- per-patch version numbers, for the same reason - -This is the widest gap between what spectral does and how kernel patches are -actually sent, so it comes first once the stubs are done. +A `series.toml` in the directory records the range it was generated for: both +ends as hashes, the revision, the files, and the blurb. The range is what makes +a re-roll safe, since regenerating `base..HEAD` instead would quietly pick up +every commit that landed on the branch since `create` and mail it with the set. ### spectral doctor -The README asks for three things before spectral is useful: a kernel tree, -`git send-email` that works, and `qemu-system-x86_64` on `$PATH`. Today you -find out that `send-email` was never configured at the moment you try to -submit, which is the worst time to learn it. +Done. It checks, in the order a first run hits them: the tree path and where it +came from, that the path is a kernel tree, `scripts/checkpatch.pl`, +`scripts/get_maintainer.pl`, `MAINTAINERS`, git, `git send-email`, a sender +identity, whether any mail route is configured, qemu on `$PATH`, that the patch +directory is writable, and whether a config file exists. -A `doctor` subcommand would check the tree, the two scripts inside it, the -`send-email` config, qemu, and the resolved paths, then name whichever one is -missing. The `Error` variants for the tree cases already exist. +One line per check with the fix under it, and the summary names the first hard +failure. Exit 1 when something hard failed; a missing qemu or a missing mail +route warns and keeps exit 0, because `--dry-run` is where the loop actually +stops on this machine. ### Config file and init -`src/config.rs` carries the TODO. Two additions: +Done. `~/.config/spectral/config.toml` (or `$XDG_CONFIG_HOME`), holding the +tree path, the patch directory, always-CC addresses and a send-email identity. +Precedence is the environment over the file over the built-in default, and +`$SPECTRAL_PATCH_DIR` joined `$SPECTRAL_KERNEL` for symmetry. -- `~/.config/spectral/config.toml` holding the tree path, the patch - directory, your send-email identity, and any addresses you always want - CC'd. The environment variable keeps winning over the file. -- `spectral init`, to clone a tree or point at one you already have and - verify it, so nobody has to read the Configuration section twice. +`spectral init --tree ` verifies a tree, or `--clone [--depth N]` +fetches one, then writes the config. A second run with the same tree says so +and writes nothing; a run that would change an existing answer is refused until +`--force`. Malformed TOML, an unknown key and a relative path are named errors +rather than surprises three commands later. ## Later @@ -93,7 +105,8 @@ re-roll. That needs spectral to record what it sent and where, which nothing does today. Where that state lives is the design question to settle before the -feature. +feature. A series already carries `series.toml`, which is the smallest version +of that record: it is what a re-roll reads. ### Packaging and polish @@ -164,7 +177,8 @@ should not produce a v3. `reroll_path` is already specified as a no-op at the same revision, and `auto` inherits that requirement. It depends on the verbs, on `doctor` to fail fast instead of halfway, and on -the config and state files the sections above describe. +the config and state files the sections above describe. `doctor` and the config +file are here now; the state file is what review tracking still has to decide. ## Not planned