samiandClaude Sonnet 5
9487202ea8
Adwaita-derived (GNOME Project, LGPL-3-or-CC-BY-SA), recoloured per light/dark skin and laid out in IDM's toolbar positions per docs/03-gui-spec.md §1. Licence and per-file provenance recorded in gui/resources/icons/LICENSE. New IconTheme mirrors ThemeManager's colorSchemeChanged hook so a live light/dark switch swaps every action's glyph, not just the QSS. Toolbar/menu actions, the tray icon and the window icon all now use it instead of the SP_ArrowDown placeholder. Install-path notes for PKG filed as R5 in gui/docs/pkg-qa-requests-m1.md. Also rebases lane/gui onto main (13 commits, no gui/ conflicts). Verified: full build clean, all 13 gui ctest targets pass (including gui_no_download_logic), unhappy-path DoD gate PASS against mockd (--drop-connection). Co-Authored-By: Claude Sonnet 5 <[email protected]>
254 lines
14 KiB
Markdown
254 lines
14 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).
|
||
|
||
---
|
||
|
||
## R5 — real icon set has landed; stop shipping the placeholder, here's the install path
|
||
|
||
`gui/resources/icons/.gitkeep` is gone. There's a real, licensed icon set now (Adwaita,
|
||
GNOME Project, LGPL-3-or-CC-BY-SA — full text and per-file provenance in
|
||
`gui/resources/icons/LICENSE`), embedded into `velox-gui` via `qt_add_resources` in
|
||
`gui/CMakeLists.txt` (`:/icons/{light,dark}/<name>.svg` for toolbar/menu actions,
|
||
`:/icons/app/velox.svg` for the window and tray icon). Nothing needs installing
|
||
separately for the binary itself — the SVGs are baked into the Qt resource system at
|
||
build time, not read from disk at runtime.
|
||
|
||
What that means for whatever `.deb` postinst / desktop-entry work PKG is doing:
|
||
|
||
* **Stop bundling a placeholder icon file in the test `.deb`.** There's no longer a gap
|
||
to fill — `velox-gui`'s own binary already carries its icon.
|
||
* **App icon for the `.desktop` file / desktop menu entry**, if PKG is generating one:
|
||
the source SVG is `gui/resources/icons/app/velox.svg` (full colour, unmodified Adwaita
|
||
`folder-download` glyph, same in light and dark). If the packaging pipeline wants a
|
||
filesystem-installed icon (standard XDG icon-theme layout, e.g.
|
||
`/usr/share/icons/hicolor/scalable/apps/velox.svg`, referenced from the `.desktop`
|
||
file's `Icon=velox` key) rather than relying on the binary's embedded window icon for
|
||
the taskbar/launcher, that single file is the one to install — no rasterization needed,
|
||
it's already a clean scalable SVG. GUI has no packaging-stage code to add this itself
|
||
(CLAUDE.md §1: `packaging/` is PKG's lane), so filing the path here rather than reaching
|
||
across.
|
||
* Licence compliance for the `.deb`: `gui/resources/icons/LICENSE` should ship somewhere
|
||
discoverable from the package (e.g. copied into `debian/copyright` or
|
||
`/usr/share/doc/velox/copyright` alongside whatever other third-party attributions PKG
|
||
already tracks) — it documents the Adwaita provenance and dual LGPL-3/CC-BY-SA license
|
||
per file, which needs to be preserved wherever the binary that embeds these SVGs ships.
|