Skip to content

fix(build): qtkeychain's libsecret check finds the backend, and the touchpoint manifest is regenerated after #6267 and #6273. Principle VIII. - #6275

Merged
ten9876 merged 2 commits into
mainfrom
fix/qtkeychain-libsecret-check
Oct 8, 2026
Merged

ten9876 merged 2 commits into
mainfrom
fix/qtkeychain-libsecret-check

Conversation

@ten9876

@ten9876 ten9876 commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

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: LibSecretKeyring loads secret-1 through QLibrary. Ubuntu links with --as-needed by default, so the built library has no NEEDED libsecret-1 entry, 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 in setup-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 with LIBSECRET_SUPPORT=ON.

Evidence

  • setup-qtkeychain.sh built with LDFLAGS=-Wl,--as-needed, reproducing Ubuntu's linking:
    • NEEDED libsecret: 0, so the old check would fail;
    • the script prints Verified: qtkeychain has its libsecret backend.
  • A LIBSECRET_SUPPORT=OFF build (qtkeychain 0.16.0) has no secret_password_lookup, so the new check fails it as it should.
  • The script header no longer claims linuxdeploy bundles libsecret. It is loaded from the user's system, which every Secret Service desktop provides.

Also: stale touchpoint manifest

#6267 and #6273 each regenerated docs/architecture/aetherd-touchpoints.md without the other's change. Together, core/ThemeManager.h has 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

… 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>
@ten9876
ten9876 requested review from a team as code owners October 8, 2026 07:58
…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>
@ten9876 ten9876 changed the title fix(build): check that qtkeychain has its libsecret backend, not that it links libsecret. Principle VIII. fix(build): qtkeychain's libsecret check finds the backend, and the touchpoint manifest is regenerated after #6267 and #6273. Principle VIII. Oct 8, 2026
@aethersdr-agent

Copy link
Copy Markdown
Contributor

Static checks: already fixed by your second commit

Good news — there's nothing to fix here. CI reported on e64082c4, the first of this PR's two commits, and the commit that fixes it was already on the branch.

Failing run Static checks → aetherd touchpoint manifest on e64082c4
Fixed by ab26d1d1 ("regenerate the touchpoint manifest after #6267 and #6273 merged together")
Current head Static checks ✅ passes on ab26d1d1

Verified locally

Reproduced at e64082c4 against tools/gen_touchpoint_manifest.py:

::error::aetherd-touchpoints.md is stale — run `python tools/gen_touchpoint_manifest.py`

…and at ab26d1d1: manifest up to date.

The drift is not from your change. Two checks confirm it:

  • git diff --stat 8603a1a1 e64082c4 touches only .github/docker/Dockerfile and scripts/setup/setup-qtkeychain.sh — no src/, so nothing in that commit could move an includer count.
  • The same check fails on 8603a1a1 (main) itself, exactly as your PR body describes.

The whole delta is the one line you already committed in docs/architecture/aetherd-touchpoints.md:

-| `core/ThemeManager.h` | 168 | ui-support — Qt token-based theming singleton …
+| `core/ThemeManager.h` | 169 | ui-support — Qt token-based theming singleton …

Tag unchanged (ui-support); only the count moves — as you noted.

Two notes

  • build / check-windows / check-macos on e64082c4 show as cancelled, not failed — superseded when you pushed ab26d1d1. They have not reported on the new head yet, so they're still worth a look before merge; the cancellation isn't a signal either way.
  • Because main is currently stale, this check is red for every PR branched off 8603a1a1, not just yours. Merging this clears it repo-wide.

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

@ten9876
ten9876 merged commit 14ef3e7 into main Oct 8, 2026
8 checks passed
@ten9876
ten9876 deleted the fix/qtkeychain-libsecret-check branch October 8, 2026 08:23

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 → job build, failed step "Build and push" (run 37746996…/37746195605)
  • check-system-libs-linux → failed step "Install the release Qt and qtkeychain" (run 37746195603), which is setup-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), then resolve("secret_password_lookup") et al. at lines 289-299. libsecret is dlopen'd, never symbol-referenced, so even though qtkeychain/CMakeLists.txt:62 appends ${LIBSECRET_LIBRARIES} to the link line, Ubuntu's --as-needed drops the NEEDED entry. The old readelf check could never pass. The PR's diagnosis is exactly right.
  • The replacement check is discriminating, not vacuous — that was my main worry, because libsecret.cpp is in qtkeychain_SOURCES unconditionally (qtkeychain/CMakeLists.txt:66). But every line of it, including the resolve("secret_password_lookup") literals, sits inside #if defined(HAVE_LIBSECRET) (lines 1, 9, 171, 287-301), and HAVE_LIBSECRET is defined only under if(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 1 and VERSION 0.16.0 (CMakeLists.txt:4, qtkeychain/CMakeLists.txt:97-99), so the installed files are libqt6keychain.so → .so.1 → .so.0.16.0. .so.0.* matches today only because it happens to match the version's leading 0., not the soname — a bump to 1.x breaks it. It fails closed (an unmatched glob stays literal and grep errors 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 -q is 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:58 is pkg_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 being OFF, 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.yml and nothing stages libsecret (line 116 is the -dev build dep), and with no NEEDED entry 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 no libsecret-1.so.0, LibSecretKeyring::isAvailable() returns false (keychain_unix.cpp:173-178) and detection falls through to Backend_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 near README.md:151-152, since the existing libsecret entries at README.md:183/191/202 read 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.cpp is compiled unconditionally (qtkeychain/CMakeLists.txt:66), so the string might be present with LIBSECRET_SUPPORT=OFF and the check would pass vacuously. It isn't — the whole translation unit body and all eight resolve("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 given SOVERSION 1/VERSION 0.16.0; the script's ls …/libqt6keychain.so.* | head -1 sorts .so.0.16.0 ahead of .so.1, and grep follows 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 check exit 1-ed before the stamp was written (line 198), so no half-verified tree exists. STAMP_CONTENT is unchanged by this PR and the CI caches key on hashFiles('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 under uname -s = Linux (lines 140-147), so grep -a (GNU-ism concerns, nonexistent .so on Darwin) never executes there. check-macos is 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.yml triggers only on push to branches: [main] for the Dockerfile paths, so the image layer runs post-merge. system-libs-canary.yml's paths: are src/, third_party/, tests/, CMakeLists.txt — not scripts/setup/, so even merging won't retrigger it before the 04:00 UTC cron. The Linux build/Static checks jobs run inside the :latest container with qtkeychain prebuilt, and since the #6273 image publish failed, :latest is 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant