dtourolle
f9154ad12e
2cde2874 made `cargo test -p traceability` fail on any write of a
rating, flag, colour label or trash membership that is not on a
reviewed list, and CONTRIBUTING.md, which lists what CI will stop a
change for, did not mention it. It is now the third invariant beside
the ui-names-no-operation test and the operation schema: what it finds,
the kinds of reason a write can be listed with, that the workspace test
run is what runs it, and `traceability -- verdicts` to print the list.
209 lines
9.3 KiB
Markdown
209 lines
9.3 KiB
Markdown
# Contributing to DarkRoom
|
|
|
|
There is a lot of documentation here — twenty-odd documents and 192 numbered
|
|
requirements — and almost all of it is written for someone who has already
|
|
decided to work on this. This file is the other thing: how to get a first
|
|
change landed without reading any of it.
|
|
|
|
## The shortest useful contribution
|
|
|
|
**A develop operation is one file.** Not one file plus a registration, plus a
|
|
shader edit, plus a control in the UI — one file:
|
|
|
|
```
|
|
core/dr-pipeline/ops/split_toning.yaml
|
|
```
|
|
|
|
`build.rs` finds it with `read_dir`, compiles it into Rust implementing
|
|
`Operation`, and from there it is indistinguishable from a hand-written node.
|
|
It arrives with controls built from its declared parameter kinds, a place in
|
|
the chain from `order:`, a place in the panel from `attributes:`, sidecar
|
|
persistence, and its own tests — which are declared in the same file and run
|
|
under `cargo test`.
|
|
|
|
Read [`core/dr-pipeline/ops/README.md`](core/dr-pipeline/ops/README.md) and
|
|
copy [`exposure.yaml`](core/dr-pipeline/ops/exposure.yaml). Split toning,
|
|
colour zones, selective colour and channel-mixer variants are all pure point
|
|
operations, which means all of them are declarations rather than code.
|
|
|
|
If you want to understand one thing about the architecture before starting,
|
|
make it this: **the core describes its capabilities and the interface composes
|
|
them.** No code in `ui/` names an operation, and a test enforces that
|
|
(`ui/dr-ui/tests/ui_names_no_operation.rs`). It is why your node needs no UI
|
|
change.
|
|
|
|
## Getting it to build
|
|
|
|
**Git LFS is required.** Model weights are stored in LFS, and a clone made
|
|
without it leaves a ~130-byte text pointer where an 11 MB model should be:
|
|
|
|
```bash
|
|
git lfs install && git lfs pull
|
|
```
|
|
|
|
Forget this and `dr-segment`'s build script stops with an instruction rather
|
|
than embedding the pointer and failing at inference time — but it is easier to
|
|
run the two commands now.
|
|
|
|
**The toolchain pins itself.** `rust-toolchain.toml` selects 1.92.0 and rustup
|
|
fetches it on first use. Do not override it; `cargo fmt` and `clippy` are both
|
|
version-sensitive and CI runs exactly this version.
|
|
|
|
**System packages.** Slint and winit need these at build time. On Debian or
|
|
Ubuntu:
|
|
|
|
```bash
|
|
sudo apt-get install pkg-config libfontconfig1-dev libxkbcommon-dev
|
|
```
|
|
|
|
**Then:**
|
|
|
|
```bash
|
|
cargo run -p darkroom-desktop
|
|
```
|
|
|
|
The first build resolves some 850 crates and takes a while — on a laptop, long
|
|
enough to look like a hang. It is not one.
|
|
|
|
Android is a containerised toolchain and is not needed for most work; see
|
|
[`docker/android/README.md`](docker/android/README.md) if you get there.
|
|
|
|
## What CI will check
|
|
|
|
All four of these run on every push, so run them before you send anything:
|
|
|
|
```bash
|
|
cargo fmt --all -- --check
|
|
cargo clippy --workspace --all-targets -- -D warnings
|
|
cargo test --workspace
|
|
cargo build --workspace --release
|
|
```
|
|
|
|
GPU tests skip themselves where there is no adapter rather than failing — a
|
|
test that cannot run is not evidence either way — so a green run on a machine
|
|
without a GPU is expected, and does not mean the GPU paths were exercised.
|
|
|
|
There is a fifth check, and it is not in that list because you are unlikely to
|
|
break it by accident:
|
|
|
|
```bash
|
|
cargo run --release -p dr-bench -- check
|
|
```
|
|
|
|
That is the benchmark suite (`docs/dev/requirements.md` §8), which builds a
|
|
synthetic 50,000-image catalog and fails the build if a performance target is
|
|
missed or a measurement has drifted past its tolerance. It runs on every push in
|
|
its own workflow. [`docs/dev/benchmarks.md`](docs/dev/benchmarks.md) says what it
|
|
measures, what it deliberately does not, and how to read a failure. If you have
|
|
touched the catalog, the decoder, the thumbnail store or the exporter, run it
|
|
before you send.
|
|
|
|
## Requirements and traceability
|
|
|
|
[`requirements.md`](docs/dev/requirements.md) is the register of record.
|
|
[`traceability.md`](docs/dev/traceability.md) is generated from `TRACES:` tags in
|
|
the source and must never be hand-edited:
|
|
|
|
```rust
|
|
// TRACES: FR-DEV-3a | FR-DEV-3c
|
|
```
|
|
|
|
Tags are read from `.rs`, `.slint`, `.wgsl` and `.yaml` — the last so a
|
|
declared operation can record the requirement it satisfies, since the Rust it
|
|
generates lands in `OUT_DIR` and is not scanned.
|
|
|
|
A pre-commit hook regenerates the matrix and stages it whenever you touch
|
|
something that can carry a tag, so you should not have to think about it. If
|
|
you do need to run it by hand:
|
|
|
|
```bash
|
|
cargo run -p traceability -- report
|
|
```
|
|
|
|
Note that it tracks line numbers, so a change that only moves code still moves
|
|
the matrix. Never regenerate it with a stale prebuilt binary.
|
|
|
|
**One convention that the tooling cannot enforce.** A tag proves that a tag
|
|
exists, not that the code under it does the thing — `docs/dev/code-health.md`
|
|
CH-4 has the details, and two requirements currently read as covered on the
|
|
strength of plumbing a future feature would use. So: **close a requirement
|
|
with a test that would fail if the behaviour were removed.** Coverage that
|
|
moves slowly and means something beats coverage that moves quickly.
|
|
|
|
**Keys and gestures are held the same way.** A key handler in Slint compares
|
|
one canonical chord, `Keys.chord(event) == "Ctrl+Z"`, under a `// KEYMAP:`
|
|
comment naming its section of the gesture book, and every key it binds must be
|
|
named by a `GESTURE:` block beside it. `cargo run -p traceability -- gestures`
|
|
regenerates [`docs/gestures.md`](docs/gestures.md) and the in-app help sheet
|
|
from those blocks; `-- gestures-check` fails when a handler binds a key no tag
|
|
names, or a tag names a key no handler binds. A `manual:` field in a block
|
|
links the gesture to a section of the manual, and a heading that is not there
|
|
fails the scan.
|
|
|
|
**The manual is checked too.** `docs/manual/index.html` is rendered from
|
|
`docs/manual/README.md` by `-- manual` and `-- manual-check` fails when they
|
|
differ; `tools/manual/record.sh --check` fails when the manual shows a picture
|
|
no scene in `tools/manual/scenes.py` makes. If you change what a pictured
|
|
screen looks like, [`tools/manual`](tools/manual/README.md) says how to record
|
|
it again. The pre-commit hook regenerates the matrix, the gesture book and the
|
|
page; CI runs all three checks.
|
|
|
|
## Three invariants the build defends
|
|
|
|
Worth knowing before you trip one, because each failure names a requirement
|
|
rather than a line:
|
|
|
|
- **No operation may be named in `ui/`** (FR-DEV-3a). Special-casing one
|
|
operation in the panel to fix a layout problem is how a generated interface
|
|
stops being generated. If a node needs presentation the panel cannot give it,
|
|
the answer is a `presentation:` hint in the declaration and a `WidgetKind`,
|
|
not a branch in `develop.rs`.
|
|
- **The operation schema rejects ambiguity at build time**: a duplicate
|
|
`order:`, a filename disagreeing with its `id:`, a default outside its own
|
|
range, an expression naming something that is not a parameter. Each error
|
|
names the key you got wrong and exits rather than panicking.
|
|
- **No verdict is written without a user action** (FR-CULL-13). A rating,
|
|
flag, colour label or trash membership is the photographer's to set, never a
|
|
signal's. `tools/traceability/src/verdicts.rs` finds every write of one in
|
|
the shipped code — the catalog setters, SQL that assigns those columns, the
|
|
sidecar's judgement amendment — and holds each to a reviewed list with its
|
|
reason: inside a Slint `on_*` callback, writing for callers that are checked
|
|
in turn, or carrying a verdict made elsewhere, such as a sidecar pull or the
|
|
sync merge. A new write fails `cargo test` (the `traceability` crate's tests,
|
|
part of the workspace run) until it is listed, and so does a listed one that
|
|
has gone; `cargo run -p traceability -- verdicts` prints the list.
|
|
|
|
## Commit messages
|
|
|
|
Imperative subject describing the change from the reader's side — "Offer the
|
|
merge when two people turn out to share a name", not "fix: merge dialog". No
|
|
conventional-commits prefixes.
|
|
|
|
The body is where the reasoning goes, and it is expected to be substantial when
|
|
the change is. This codebase records *why* far more than most, in commits and
|
|
in comments alike, and that is the single habit most worth adopting: the
|
|
constraint you worked around is invisible to whoever reads the diff next.
|
|
|
|
One commit per change. If you fixed two things, that is two commits.
|
|
|
|
## Where to read next, in order
|
|
|
|
| Document | Read it when |
|
|
|---|---|
|
|
| [`core/dr-pipeline/ops/README.md`](core/dr-pipeline/ops/README.md) | Adding or changing a develop operation — start here regardless |
|
|
| [`docs/dev/architecture.md`](docs/dev/architecture.md) | Anything touching the render path, catalog or sync |
|
|
| [`docs/dev/code-health.md`](docs/dev/code-health.md) | Deciding what to work on; grades each seam by what it costs |
|
|
| [`docs/dev/benchmarks.md`](docs/dev/benchmarks.md) | A change that could plausibly cost time or memory |
|
|
| [`docs/dev/technical-debt.md`](docs/dev/technical-debt.md) | Something looks wrong — check it was not chosen |
|
|
| [`docs/dev/distribution.md`](docs/dev/distribution.md) | Packaging a build, or adding a permission to one |
|
|
| [`docs/dev/requirements.md`](docs/dev/requirements.md) | Reference, not reading |
|
|
|
|
`technical-debt.md` is the one to check before "fixing" anything surprising.
|
|
It records compromises that were deliberate, each with the reasoning and a
|
|
falsifiable condition for when it stops being one — the point being that you
|
|
can tell a constraint from an accident without asking.
|
|
|
|
## Licence
|
|
|
|
GPL-3.0-or-later. By contributing you agree your work is licensed the same way.
|