Repository navigation
fix(build): qtkeychain's libsecret check finds the backend, and the touchpoint manifest is regenerated after #6267 and #6273. Principle VIII. - #6275
Conversation
… it links libsecret. Principle VIII. qtkeychain loads libsecret at runtime (QLibrary "secret-1"). Under Ubuntu's default --as-needed, the built library therefore has no NEEDED entry for it. The libsecret check added in #6273 rejected a correct build on main (8603a1a): Build CI Docker Image and System Libraries Canary both failed. The same check sits in setup-qtkeychain.sh, so Ubuntu builds following the README and the AppImage job would fail the same way. Both checks now look for the backend itself: the libsecret function names qtkeychain resolves, which are in the library only when it was built with libsecret. - A libsecret build linked with --as-needed (no NEEDED entry) passes. - A LIBSECRET_SUPPORT=OFF build fails. The script's header no longer claims linuxdeploy bundles libsecret. It is loaded from the user's system, which every Secret Service desktop provides. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…merged together. Principle VIII. Each PR regenerated the manifest without the other. Together, ThemeManager.h has one more GUI includer (168 to 169), which left Static checks red on main. The tag is unchanged (ui-support); only the count moves. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Static checks: already fixed by your second commitGood news — there's nothing to fix here. CI reported on
Verified locallyReproduced at …and at The drift is not from your change. Two checks confirm it:
The whole delta is the one line you already committed in -| `core/ThemeManager.h` | 168 | ui-support — Qt token-based theming singleton …
+| `core/ThemeManager.h` | 169 | ui-support — Qt token-based theming singleton …Tag unchanged ( Two notes
Nice catch spotting that #6267 and #6273 each regenerated the manifest blind to the other — that's a conflict-free merge where both sides edit the same row, so git won't flag it and it only surfaces as a red check afterwards. Thanks for fixing both that and the libsecret check in one pass. 🙏 🤖 Generated with Claude Code 🤖 aethersdr-agent · cost: $6.5550 · model: claude-opus-5 |
There was a problem hiding this comment.
Issue fit
No linked issue, and GOVERNANCE.md doesn't want one here: this is a clear-root-cause build fix for a regression this repo is carrying right now, so no RFC is needed. The two commits are both well-founded, and I verified each independently rather than taking the body's word for it:
1. The libsecret check. 8603a1a14 (#6273) introduced readelf -d … | grep -q libsecret-1 in both scripts/setup/setup-qtkeychain.sh and .github/docker/Dockerfile. On that merge commit, main is red in exactly the two lanes that execute those checks:
Build CI Docker Image→ jobbuild, failed step "Build and push" (run37746996…/37746195605)check-system-libs-linux→ failed step "Install the release Qt and qtkeychain" (run37746195603), which issetup-qt.sh→setup-qtkeychain.sh:166
get_ci_run_log gives me step names, not step text, so I can't quote the error — but I can confirm the mechanism from upstream source. I cloned the pinned commit (aa6da344, qtkeychain 0.16.0) and read it:
qtkeychain/libsecret.cpp:285—LibSecretKeyring::LibSecretKeyring() : QLibrary(QLatin1String("secret-1"), 0), thenresolve("secret_password_lookup")et al. at lines 289-299. libsecret is dlopen'd, never symbol-referenced, so even thoughqtkeychain/CMakeLists.txt:62appends${LIBSECRET_LIBRARIES}to the link line, Ubuntu's--as-neededdrops theNEEDEDentry. The oldreadelfcheck could never pass. The PR's diagnosis is exactly right.- The replacement check is discriminating, not vacuous — that was my main worry, because
libsecret.cppis inqtkeychain_SOURCESunconditionally (qtkeychain/CMakeLists.txt:66). But every line of it, including theresolve("secret_password_lookup")literals, sits inside#if defined(HAVE_LIBSECRET)(lines 1, 9, 171, 287-301), andHAVE_LIBSECRETis defined only underif(LIBSECRET_SUPPORT)(line 59). So the string is present iff the backend was compiled in. The check does what the comment says it does.
2. The manifest. Verified mechanically, not read: python3 tools/gen_touchpoint_manifest.py --check exits 1 ("stale") on my main checkout and 0 ("manifest up to date") on the PR head. ThemeManager.h 168 → 169 is the real regenerated value, and this is the known stale-after-merge case (#6267 and #6273 both touching the same include count).
Scope
| File | What it changes | Claimed by the title/body? | Verdict |
|---|---|---|---|
.github/docker/Dockerfile |
readelf → grep -aq secret_password_lookup; +3 comment lines |
Yes | In scope |
scripts/setup/setup-qtkeychain.sh |
same check swap in the sibling script; header comment corrected | Yes | In scope |
docs/architecture/aetherd-touchpoints.md |
one generated count 168 → 169 | Yes | In scope — generator-verified |
Everything in the diff is explained. Two commits, each one concern, both SIGNED. No new public surface, no settings keys, no UI, no CHANGELOG entry (correct). Both sibling copies of the duplicated check are fixed together — I grepped for a third and .github/docker/Dockerfile.tsan:139 builds with LIBSECRET_SUPPORT=OFF and carries no libsecret assertion, which is consistent, not a missed sibling. The deleted - lines are comment prose only; no guard, early return or confirmation was removed.
Blockers
None.
Nits (all non-blocking)
-
.github/docker/Dockerfile:156— the glob hardcodes the wrong version component:&& grep -aq secret_password_lookup "${QTKEYCHAIN_ROOT}"/lib/libqt6keychain.so.0.* \qtkeychain sets
SOVERSION 1andVERSION 0.16.0(CMakeLists.txt:4,qtkeychain/CMakeLists.txt:97-99), so the installed files arelibqt6keychain.so→.so.1→.so.0.16.0..so.0.*matches today only because it happens to match the version's leading0., not the soname — a bump to 1.x breaks it. It fails closed (an unmatched glob stays literal andgreperrors out), so this is cosmetic, but it adds a second thing to remember in a block whose own comment already says "BUMPING QTKEYCHAIN MEANS EDITING BOTH", and it diverges from the sibling script's.so.*. Aligning both on the loose glob is safe —grep -qis happy with the symlinks in the match list:&& grep -aq secret_password_lookup "${QTKEYCHAIN_ROOT}"/lib/libqt6keychain.so.* \ -
scripts/setup/setup-qtkeychain.sh:138-139(pre-existing, untouched, but adjacent to your hunk) — "qtkeychain's own CMake quietly builds without it when pkg-config can't find it" isn't true for 0.16.0:qtkeychain/CMakeLists.txt:58ispkg_check_modules(LIBSECRET REQUIRED libsecret-1), which hard-errors at configure. The pre-check is still worth keeping for the clearer message, and the post-build check still earns its place — what it actually catches is the flag beingOFF, which is precisely the #6273 bug. Worth a one-line correction while you're in the file. -
Runtime dependency is now undisclosed. You removed "linuxdeploy bundles libsecret for the AppImage", which was never true — I grepped
appimage.ymland nothing stages libsecret (line 116 is the-devbuild dep), and with noNEEDEDentry linuxdeploy has nothing to resolve. So the replacement text is the honest one. But "the library comes from the user's system, where every Secret Service desktop has it" (scripts/setup/setup-qtkeychain.sh:26) is an unverified claim, and the failure mode is silent: with nolibsecret-1.so.0,LibSecretKeyring::isAvailable()returns false (keychain_unix.cpp:173-178) and detection falls through toBackend_GnomeKeyring(keychain_unix.cpp:174-195) — the dlopen'd libgnome-keyring that RFC #6271 found absent, i.e. the pre-#6271 "Unknown error, session-only credentials" symptom returns with nothing pointing at the cause. I would not ask for bundling (glib/gio are excludelisted; bundling libsecret alone is worse). Just disclose it: a line in the PR body, and ideally an AppImage runtime note nearREADME.md:151-152, since the existinglibsecretentries atREADME.md:183/191/202read as build deps. -
.github/workflows/appimage.yml:253— stale comment left over from before #6271: "The pure Qt-D-Bus backend talks to KDE Wallet / GNOME Keyring at runtime." That's the belief #6273 disproved. Same class of fix as this PR's header edit.
What I tried to break
- "The grep proves the backend is compiled in." The attack:
libsecret.cppis compiled unconditionally (qtkeychain/CMakeLists.txt:66), so the string might be present withLIBSECRET_SUPPORT=OFFand the check would pass vacuously. It isn't — the whole translation unit body and all eightresolve("secret_password_*")literals are inside#if defined(HAVE_LIBSECRET), defined only under the option. Check survives. - Glob and file-selection.
.so.0.*resolves to the one real versioned file givenSOVERSION 1/VERSION 0.16.0; the script'sls …/libqt6keychain.so.* | head -1sorts.so.0.16.0ahead of.so.1, andgrepfollows the symlink either way. Both find a real file today. Fragility noted above as a nit, and it's fail-closed. - Stale-stamp bypass. The script early-exits on
third_party/qtkeychain/.build-stamp(line 83) before the verification, so I checked whether a tree built by the broken script could skip the new check. It can't: the old checkexit 1-ed before the stamp was written (line 198), so no half-verified tree exists.STAMP_CONTENTis unchanged by this PR and the CI caches key onhashFiles('scripts/setup/setup-qtkeychain.sh'), which this diff invalidates — so the new check will actually run where it's cached. - macOS. The gated block is
if [ "$LIBSECRET_FLAG" = "ON" ]and the flag is set only underuname -s = Linux(lines 140-147), sogrep -a(GNU-ism concerns, nonexistent.soon Darwin) never executes there.check-macosis green for exactly that reason — which also means it proves nothing about this fix. - What I could not check, and the real caveat: neither changed check was exercised by this PR's green CI.
docker-ci-image.ymltriggers only onpushtobranches: [main]for the Dockerfile paths, so the image layer runs post-merge.system-libs-canary.yml'spaths:aresrc/,third_party/,tests/,CMakeLists.txt— notscripts/setup/, so even merging won't retrigger it before the 04:00 UTC cron. The Linuxbuild/Static checksjobs run inside the:latestcontainer with qtkeychain prebuilt, and since the #6273 image publish failed,:latestis still the pre-#6273 image. AppImage runs only on a tag. All findings above are reasoned from the diff, the head checkout and upstream source — I ran no build and executed neither check.
Recommendation
Approve with nits. The diagnosis is correct and verified against upstream source rather than taken on faith, the replacement check is genuinely discriminating, both duplicated copies are fixed together, and the manifest commit is generator-verified and needed (main is stale right now). The verification caveat is real but bounded: the checks are fail-closed, so if the grep were wrong the image build would stay red exactly as it is today — this cannot make main worse, and it's the only path to making it green. Next step for the maintainer: merge, watch docker-ci-image.yml publish the qtkeychain layer (it auto-triggers on the Dockerfile path), then workflow_dispatch the system-libs canary rather than waiting for the cron. Nice catch on a bug that was only going to show up after the merge.
🤖 aethersdr-agent · cost: $6.6369 · model: claude-opus-5
Fixes main after #6273. On
8603a1a1, Build CI Docker Image and System Libraries Canary fail on the libsecret check that #6273 added.Cause
qtkeychain loads libsecret at runtime:
LibSecretKeyringloadssecret-1throughQLibrary. Ubuntu links with--as-neededby default, so the built library has noNEEDED libsecret-1entry, even though its libsecret backend is compiled in. The check looked for that entry (readelf -d … | grep libsecret-1), so it rejected a correct build. The same check is insetup-qtkeychain.sh, which is also used by Ubuntu builds that follow the README and by the AppImage job.Fix
Both checks now look for the backend itself: the libsecret function names qtkeychain resolves (
secret_password_lookup…), which are in the library only when it was built withLIBSECRET_SUPPORT=ON.Evidence
setup-qtkeychain.shbuilt withLDFLAGS=-Wl,--as-needed, reproducing Ubuntu's linking:NEEDED libsecret: 0, so the old check would fail;Verified: qtkeychain has its libsecret backend.LIBSECRET_SUPPORT=OFFbuild (qtkeychain 0.16.0) has nosecret_password_lookup, so the new check fails it as it should.Also: stale touchpoint manifest
#6267 and #6273 each regenerated
docs/architecture/aetherd-touchpoints.mdwithout the other's change. Together,core/ThemeManager.hhas one more GUI includer (168 to 169), which leaves Static checks red on main. The manifest is regenerated here (gen_touchpoint_manifest.py --check: up to date). The tag is unchanged (ui-support); only the count moves.The Dockerfile change takes effect when the CI image is next built: Build CI Docker Image runs on push to main and proves it there.
🤖 Generated with Claude Code