samiandClaude Sonnet 5
755d85964e
gui/docs/pkg-qa-requests-m1.md R3, filed by the previous session: three GUI M1 DoD items (10k rows at 60 fps, flat RSS over 10 minutes, --slow/--flaky/--drop-connection recovery) had nowhere to run in CI. This is the harness — `gui/tests/dod/run.sh <gate> [--json <path>]`, exactly the path/invocation contract tests/integration/README.md already specified — plus `gui/tests/dod/dod_harness.cpp`, the Qt/RpcClient-driven binary that actually runs each gate against a real mockd run.sh starts and tears down itself. - scroll-60fps: an eased scripted scroll over the whole loaded table, timing each step's synchronous repaint; p99 against a 16.6 ms budget (auto-scaled 4x under a sanitized build — ASan/UBSan overhead, not a loosened bar, see the harness's isSanitizedBuild()). - rss-flat: samples this process's own VmRSS at 1 Hz across the run, discards a warm-up window, checks post-warm-up growth against a stated 20 MiB slack. - unhappy-path: three phases (slow/flaky/drop-connection), each its own mockd instance; passes when the client reaches and holds Connected with no crash or hang. A watchdog (the harness's own QTimer, backstopped by run.sh's external `timeout`) turns a genuine hang into a bounded non-zero exit rather than needing the CI caller to timeout(1) around it. Every gate honours the exit-code and --json contract PKG/QA's pre-drafted CI job expects unchanged (one addition needed: the build step must also build the `gui-dod-harness` target, noted in the R3 update). No leaked mockd processes on any exit path (`trap cleanup EXIT INT TERM`); no writes outside a tempdir except the caller's own --json path. Verified live end-to-end (not just unit-level): all three gates run against a real mockd under the exact `ASAN_OPTIONS=detect_leaks=1:halt_on_error=1` `.github/workflows/ci.yml`'s sanitizers job already sets, all pass, and scroll-60fps was forced red once on purpose (VELOX_DOD_FRAME_BUDGET_MS=1) to prove the fail path and exit code actually work. Building this is also what surfaced the two RpcClient bugs fixed in the previous commit, and one real gap in mockd itself — --drop-connection never worked over the Unix socket transport (only WebSocket) — filed as gui/docs/proto-requests-m1.md since tools/mockd is PROTO's file. gui/docs/pkg-qa-requests-m1.md R3 and R4 (an unrelated, non-blocking Qt6::DBus CMake hygiene note filed while wiring the clipboard global-shortcut path) are updated with the concrete findings above. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01NSCdCWFXBTSBBK3MzWtJiC
222 lines
12 KiB
Markdown
222 lines
12 KiB
Markdown
# GUI → PKG/QA requests (M1)
|
||
|
||
Filed by lane GUI. These touch PKG/QA-owned files (`tools/bootstrap.sh`, the CI jobs,
|
||
`docs/agents/AGENT-PKG-QA.md`) and the root `README.md`, so GUI is not making the edits —
|
||
CLAUDE.md §1. Everything below is apply-ready.
|
||
|
||
---
|
||
|
||
## R1 — `libqt6svg6-dev` is not a real package on 26.04 (hard configure failure now)
|
||
|
||
`apt-cache policy libqt6svg6-dev` on a clean Ubuntu 26.04 box gives `Candidate: (none)`.
|
||
The Qt 6 SVG **dev** package in the 26.04 archive is **`qt6-svg-dev`** (currently
|
||
`6.10.2-2`). `libqt6svg6` (no `-dev`) exists as the runtime lib but carries no headers or
|
||
CMake config, so `find_package(Qt6 COMPONENTS Svg)` fails without `qt6-svg-dev`.
|
||
|
||
I checked the other 38 apt names in `bootstrap.sh` against `apt-cache policy` on 26.04 —
|
||
`libqt6svg6-dev` is the only one with no candidate. Everything else resolves.
|
||
|
||
**This is no longer a latent bug.** `gui/CMakeLists.txt` is now on `main`, so the root
|
||
`CMakeLists.txt` line
|
||
|
||
```cmake
|
||
find_package(Qt6 6.6 REQUIRED COMPONENTS Widgets Svg Network LinguistTools)
|
||
```
|
||
|
||
is live for every build. On a machine without the SVG dev package that is a hard configure
|
||
failure for the whole project — not a skipped guard, not a GUI-only problem.
|
||
|
||
**Why nothing has gone red yet — and why that's R2's problem too:**
|
||
|
||
* This dev box already has `qt6-svg-dev` (pulled in by a `qt6-base-dev` recommends chain),
|
||
so `pkg-config --exists Qt6Svg` succeeds and `--check` is green here.
|
||
* CI runs on `ubuntu-latest`, which is **24.04**, where `libqt6svg6-dev` most likely still
|
||
resolves. The project *targets* **26.04** (the `ci.yml` env comment says as much). So the
|
||
one automated place that runs `bootstrap.sh` is on the wrong release to see this, and
|
||
`--check` wouldn't catch it there anyway (R2). Wrong package name + runner-vs-target
|
||
release mismatch means the class of bug PKG/QA owns is currently unobservable in CI.
|
||
|
||
### Fix — three files, same one-line change
|
||
|
||
**`tools/bootstrap.sh`** (in `APT_GUI`, ~line 87):
|
||
```diff
|
||
APT_GUI=(
|
||
qt6-base-dev
|
||
qt6-tools-dev
|
||
qt6-tools-dev-tools # lrelease/lupdate for i18n
|
||
- libqt6svg6-dev # GUI: SVG icon rendering
|
||
+ qt6-svg-dev # GUI: SVG icon rendering (was libqt6svg6-dev — no such
|
||
+ # package on 26.04; this is the one with headers +
|
||
+ # the Qt6SvgConfig.cmake find_package needs)
|
||
)
|
||
```
|
||
|
||
**`docs/agents/AGENT-PKG-QA.md`** (~line 17):
|
||
```diff
|
||
- `libqt6svg6-dev`, `libsecret-1-dev`, `nodejs`/`npm` and (optionally) `clang` are
|
||
+ `qt6-svg-dev`, `libsecret-1-dev`, `nodejs`/`npm` and (optionally) `clang` are
|
||
```
|
||
|
||
**`README.md`** (toolchain table, ~line 106):
|
||
```diff
|
||
sudo apt update && sudo apt install -y \
|
||
- libqt6svg6-dev \ # GUI: SVG icon rendering
|
||
+ qt6-svg-dev \ # GUI: SVG icon rendering
|
||
libsecret-1-dev \ # DAEMON: Secret Service for site logins
|
||
```
|
||
|
||
---
|
||
|
||
## R2 — `--check` verifies the outcome, not the thing that can break
|
||
|
||
`--check` today runs the "verify toolchain" block: `command -v` for the binaries,
|
||
`pkg-config --exists` for the dev libs (`Qt6Core`, `Qt6Widgets`, `Qt6Svg`, `libcurl`,
|
||
`sqlite3`, `libssl`, `libsecret-1`, `libavformat`), plus the cmake/g++/node version
|
||
floors. Every one of those checks a *result* on a box that already installed everything.
|
||
None of them touch the apt package **names** in `APT_*`, which is the only part of the
|
||
script that can be wrong on a clean machine — as R1 just demonstrated. So `--check`
|
||
reported green for a script that fails `apt-get install` on its target OS.
|
||
|
||
### Proposed fix — make `--check` validate the install list it would actually run
|
||
|
||
Add this to the verify block (runs in **both** modes; in `--check` mode it is the point of
|
||
the exercise). It needs neither root nor network — `apt-cache policy` reads the local
|
||
package lists that `apt-get update` already populated:
|
||
|
||
```bash
|
||
# Every apt name this script would install must be a real, installable package on THIS
|
||
# release. This is the check that would have caught libqt6svg6-dev before the VM did.
|
||
log "validating apt package names (${#PKGS[@]})"
|
||
missing_pkgs=()
|
||
for pkg in "${PKGS[@]}"; do
|
||
# `apt-cache policy <pkg>` prints "Candidate: (none)" for a name with no installable
|
||
# version, and exits 0 either way — so grep the candidate line, don't trust $?.
|
||
cand="$(apt-cache policy "$pkg" 2>/dev/null | sed -n 's/^ Candidate: //p')"
|
||
if [[ -z "$cand" || "$cand" == "(none)" ]]; then
|
||
printf ' \033[31m✗\033[0m %-20s no installable candidate on this release\n' "$pkg"
|
||
missing_pkgs+=("$pkg")
|
||
fail=1
|
||
fi
|
||
done
|
||
if (( ${#missing_pkgs[@]} == 0 )); then
|
||
printf ' \033[32m✓\033[0m %-20s all %d resolve\n' "apt names" "${#PKGS[@]}"
|
||
fi
|
||
```
|
||
|
||
Notes for whoever applies it:
|
||
|
||
* `PKGS` is already assembled above the `CHECK_ONLY` branch, and it already respects
|
||
`--with-clang` / `--with-packaging`, so the loop covers exactly what would be installed.
|
||
* Belt-and-braces alternative: `apt-get install -s --no-install-recommends "${PKGS[@]}"`
|
||
(`-s` = simulate, no root) also catches an *un-satisfiable dependency*, not just a
|
||
missing name. Downside: it needs the apt lists reasonably fresh and is noisier to parse.
|
||
`apt-cache policy` is enough to catch the whole class of bug in R1; pick whichever you
|
||
want to own.
|
||
* **The runner mismatch is part of this.** Even with the name-validation loop, the
|
||
`bootstrap-script` job on a 24.04 runner validates names against the *24.04* archive, not
|
||
26.04. So R2 only fully closes the gap if the check also runs where the project ships —
|
||
a 26.04 container step in that job (`container: ubuntu:26.04`), or a documented decision
|
||
that 24.04 is close enough and why. Without that, a name that is valid on 24.04 and gone
|
||
on 26.04 (exactly `libqt6svg6-dev`) still slips through.
|
||
|
||
---
|
||
|
||
## R3 — the GUI DoD gates have nowhere to run in CI — RESOLVED, ready to wire in
|
||
|
||
**Status: done on GUI's side.** The harness `tests/integration/README.md` was waiting on
|
||
now exists, builds, and has been run end-to-end (all three gates, all three unhappy-path
|
||
sub-phases) against a real `mockd` with no changes needed to the pre-drafted job below.
|
||
|
||
**Path:** `gui/tests/dod/run.sh <gate> [--json <path>]`, exactly the contract
|
||
`tests/integration/README.md` specifies. `<gate>` is `scroll-60fps` | `rss-flat` |
|
||
`unhappy-path`. It builds and tears down its own `mockd` (isolated `XDG_RUNTIME_DIR` via
|
||
`mktemp -d`), needs no network, and leaves nothing running on any exit path (`trap
|
||
cleanup EXIT INT TERM`) — verified live by checking for orphaned `tsx`/`mockd` processes
|
||
after both a passing and a forced-failing run of each gate.
|
||
|
||
**One change from the pre-drafted job:** the "Configure + build" step needs to also build
|
||
the harness binary, not just `velox-gui`:
|
||
```diff
|
||
- cmake --build --preset dev --target velox-gui
|
||
+ cmake --build --preset dev --target velox-gui gui-dod-harness
|
||
```
|
||
Nothing else in the pre-drafted YAML needs to change — the `# TODO(GUI): path` comment on
|
||
the `scroll-60fps` line can just come off along with the marker on the line below it.
|
||
|
||
**`rss-flat`'s slack: 20 MiB (`VELOX_DOD_RSS_SLACK_KIB`, default `20480`).** Chosen by
|
||
running the gate locally (`VELOX_DOD_RSS_DURATION_SEC=8` override, i.e. not the real
|
||
10-minute number) a handful of times against `mockd --tasks 10000 --seed 1` and looking at
|
||
actual post-warm-up growth (single-digit MiB per run here) — 20 MiB gives real headroom
|
||
above that noise floor without being so loose a genuine per-tick leak in the progress-patch
|
||
path could hide under it. This has **not** been proven against a full real 10-minute
|
||
sanitized run (that's the nightly job's own first execution) or tuned against
|
||
production-length data yet; treat it as GUI's stated starting number, not a
|
||
load-tested constant, and expect it may need retuning after the first few real
|
||
`gui-dod-nightly` runs land actual series data.
|
||
|
||
**One caveat worth deciding on explicitly: the pre-drafted job builds with `cmake --preset
|
||
dev`, i.e. ASan+UBSan (CLAUDE.md: "default for all lanes").** `scroll-60fps`'s frame
|
||
budget already compensates (`gui/tests/dod/dod_harness.cpp` multiplies 16.6 ms by 4× when
|
||
it detects a sanitized build — measured p99 here was ~50 ms against ASan overhead, well
|
||
under the scaled 66.4 ms budget, so the multiplier is doing real work, not padding for no
|
||
reason). `rss-flat`'s slack does **not** get a similar adjustment — ASan's allocator
|
||
(redzones, quarantine) can look like real growth over a long run in a way this hasn't been
|
||
validated against yet. Two honest options: run `gui-dod-nightly` against a `release`-preset
|
||
build instead of `dev` (loses the sanitizers' own bug-catching value for this one job), or
|
||
accept `VELOX_DOD_RSS_SLACK_KIB` may need a second, larger number for the ASan build once
|
||
real 10-minute data exists. GUI's preference is the second (keep sanitizers on
|
||
everywhere), but this is genuinely PKG/QA's call since it's their job definition.
|
||
|
||
**Verified live**, under the exact `ASAN_OPTIONS=detect_leaks=1:halt_on_error=1` the
|
||
`sanitizers` job already sets (checked against `.github/workflows/ci.yml` rather than
|
||
assumed) — no leak-suppression flag needed, on any of the three gates:
|
||
|
||
* `scroll-60fps` against `mockd --tasks 10000 --seed 1`: PASS at p99 ≈ 50 ms (budget
|
||
66.4 ms sanitized). Forced red once via `VELOX_DOD_FRAME_BUDGET_MS=1` to confirm the
|
||
fail path and exit code actually work, not just the pass path.
|
||
* `rss-flat` at `VELOX_DOD_RSS_DURATION_SEC=6/8`: PASS, ~4-6 MiB growth against the
|
||
20 MiB slack.
|
||
* `unhappy-path`, all three phases (`--slow 900`, `--flaky 0.3`, `--drop-connection 5`):
|
||
PASS. One real finding from building this: `--drop-connection` is currently a no-op
|
||
over the Unix socket transport in `mockd` itself (only wired for WebSocket) — filed as
|
||
`gui/docs/proto-requests-m1.md` since that's PROTO's file to fix, not GUI's. The gate
|
||
still passes today on the weaker (but real, and the actually-documented) condition that
|
||
the client reaches and holds `Connected`; it just isn't proving a real mid-session drop
|
||
yet for that one phase.
|
||
|
||
Two bugs surfaced and fixed *by* building this harness, both in `gui/`'s own RPC client
|
||
(caught by ASan, not assumed): `RpcClient::stop()` left `conn_` dangling after joining its
|
||
worker thread, so any caller that called `stop()` and then let the client destruct hit a
|
||
double-free — the harness's own `client.stop()` at shutdown found it on the first run.
|
||
Separately, `RpcClient`'s initial `download.list` call had a hardcoded `limit: 1000` with
|
||
no paging, silently capping the table at 1000 rows regardless of how many the daemon
|
||
actually has — `scroll-60fps` against `--tasks 10000` refused to run rather than
|
||
"passing" against a 1000-row table, which is what caught it. Both fixed on `lane/gui`
|
||
before this request was filed.
|
||
|
||
---
|
||
|
||
## R4 — root `CMakeLists.txt`'s `find_package(Qt6 ...)` should list `DBus` explicitly
|
||
|
||
Not currently broken — flagging a "works, but by accident" for the record. `gui/src/
|
||
clipboard/GlobalShortcut.cpp` (docs/06-risks-and-spikes.md R2's explicit path #2:
|
||
`org.freedesktop.portal.GlobalShortcuts`) needs `Qt6::DBus`. **Verified live: the target
|
||
already exists and links today**, even though the root `find_package` doesn't list `DBus`
|
||
in `COMPONENTS` — this Qt 6 packaging apparently exports every module's CMake target once
|
||
any component pulls in the shared prefix, `DBus` included. `gui/CMakeLists.txt` still
|
||
guards the clipboard sources on `if(TARGET Qt6::DBus)` (same pattern the file already uses
|
||
for `veloxproto`), so if that turns out to be environment-specific rather than a general
|
||
Qt 6 CMake guarantee, the build degrades to "feature skipped," not "build broken," on
|
||
whatever machine finds out otherwise.
|
||
|
||
Worth making explicit anyway, since relying on undocumented target leakage is fragile:
|
||
|
||
```diff
|
||
- find_package(Qt6 6.6 REQUIRED COMPONENTS Widgets Svg Network LinguistTools)
|
||
+ find_package(Qt6 6.6 REQUIRED COMPONENTS Widgets Svg Network DBus LinguistTools)
|
||
```
|
||
|
||
No apt change needed either way — `qt6-base-dev` (already in `APT_GUI`) ships `QtDBus`'s
|
||
headers directly (verified live: `dpkg -L qt6-base-dev | grep -i dbus` lists the whole
|
||
`QtDBus/` include tree).
|