ci(windows): build and bundle qtkeychain for SmartLink credential persistence. Principle XI.#3634
ci(windows): build and bundle qtkeychain for SmartLink credential persistence. Principle XI.#3634NF0T wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Thanks for this, @NF0T — clean, well-scoped, and it correctly closes a real silent-no-op gap on Windows. The implementation faithfully follows the hidapi/FFTW3/DeepFilterNet3 setup-script pattern, the find_package PREPEND is properly WIN32-guarded so Linux/macOS are untouched, error handling in the PowerShell script is solid ($ErrorActionPreference = "Stop" plus per-step $LASTEXITCODE checks), and the windeployqt pre-stage workaround is both correct and well-commented. All six checks (build, check-windows, check-macos, CodeQL, analyze) are green, and the deploy-block copies are Test-Path-guarded so they also work on a cache hit. No application code changed, as advertised.
One substantive, non-blocking note:
Cache key doesn't capture the Qt version (windows-installer.yml:46-49). The key is qtkeychain-${{ runner.os }}-${{ hashFiles('scripts/setup/setup-qtkeychain.ps1') }} — identical to the opus/fftw/hidapi/deepfilter pattern, so this matches convention. But qtkeychain is the one dependency here that links against Qt and whose ABI must match the app's Qt exactly (the PR body calls this out). The workflow pins Qt at 6.8.* (line 24), a floating minor. A future bump of that line to e.g. 6.9.* would not invalidate this cache, so CI could silently reuse a qtkeychain built against the old Qt minor against a newer Qt — exactly the ABI mismatch the build-from-source approach exists to avoid. (Within 6.8.x patch bumps this is ABI-stable, so the practical risk is low and only bites on a minor bump.)
Cheap hardening: fold the Qt version into the key, e.g. key: qtkeychain-${{ runner.os }}-6.8-${{ hashFiles('scripts/setup/setup-qtkeychain.ps1') }}, or hash the workflow file alongside the script. Your call whether it's worth it now vs. a comment for whoever next bumps Qt.
Minor / optional: the tarball download has no SHA-256 verification — consistent with the other setup scripts, so not a regression, just noting it as future defense-in-depth for the third-party fetch.
Nothing here blocks merge.
🤖 aethersdr-agent · cost: $5.2686 · model: claude-opus-4-8
There was a problem hiding this comment.
Reviewed the full diff against the existing windows-installer.yml and the established hidapi/FFTW3/DeepFilterNet3 dependency pattern. This is clean, well-scoped, and the explanation matches what the code actually does. Nice work, @NF0T — thanks for closing the silent Windows credential-persistence gap.
What checks out
- Scope is tight: the three files are exactly the build pipeline, no application code, consistent with the "
SmartLinkClient.cppis already correct" claim. - The setup script mirrors the other
setup-*.ps1scripts:$ErrorActionPreference = "Stop", per-step$LASTEXITCODEguards on tar/configure/build/install, an idempotency guard onQt6KeychainConfig.cmake, and absolute-path normalization. Error handling at the download/extract/build boundaries is solid. - The cache step matches the sibling deps (path +
runner.os+ script hash), and thecmake --installstaging produces the layoutfind_package(Qt6Keychain QUIET)resolves against — with thelist(PREPEND CMAKE_PREFIX_PATH ...)correctlyWIN32-gated so Linux/macOS are untouched. - The DLL is copied into the
deploy/dir inside the "Deploy Qt" step, so it's captured by all three downstream artifacts (portable ZIP viaCompress-Archive deploy\*, Inno, and MSIX-DeployDir deploy) — the PR claim is accurate.
One thing worth a note (non-blocking)
The cache key (qtkeychain-${{ runner.os }}-${{ hashFiles('setup-qtkeychain.ps1') }}) is copied verbatim from the other deps, but qtkeychain differs from them in one way: Opus/FFTW3/hidapi/DeepFilterNet3 are Qt-independent C libraries, whereas qt6keychain.dll links Qt6Core and was built against whatever Qt install-qt-action resolved 6.8.* to in the run that populated the cache. On a cache hit in a later run with a newer 6.8 patch, the stale-built DLL is reused against newer Qt.
In practice this is low-risk — Qt 6 guarantees forward binary compatibility across the 6.x series and qtkeychain only touches Qt6Core, and the version is floated forward (never pinned back), so the cached build is always equal-or-older than the runtime Qt. So no action strictly required. If you want belt-and-suspenders, you could fold the resolved Qt version into the key (it's the one dep where the Qt ABI is actually a cache input), but it's fine to leave as-is to stay consistent with the other entries.
The workflow_dispatch validation on your fork (qtkeychain built against 6.8.3, HAVE_KEYCHAIN defined, DLL in artifact) is exactly the end-to-end evidence this needs. LGTM.
🤖 aethersdr-agent · cost: $2.3721 · model: claude-opus-4-8
jensenpat
left a comment
There was a problem hiding this comment.
Downloads Qtkeychain from upstream github and builds during CI run. Thanks Ryan
12da828 to
335f92e
Compare
…sistence. Principle XI. Add qtkeychain 0.16.0 to the Windows release pipeline so HAVE_KEYCHAIN is defined and SmartLink credential persistence (Auth0 refresh token in Windows Credential Manager) is active in all shipped Windows artifacts. - scripts/setup/setup-qtkeychain.ps1: download, build, and cmake-install qtkeychain against QT_ROOT_DIR; stages cmake config files, import lib, and DLL to third_party/qtkeychain/ for find_package resolution - CMakeLists.txt: prepend third_party/qtkeychain to CMAKE_PREFIX_PATH on WIN32 before find_package(Qt6Keychain QUIET) - windows-installer.yml: cache + setup steps (after Qt install, before Configure); qt6keychain.dll pre-staged to Qt bin dir before windeployqt (windeployqt looks for Qt6* DLLs in the Qt installation to resolve transitive deps); explicit qt6keychain.dll copy in deploy block Validated via workflow_dispatch on NF0T/AetherSDR (run 27658397042): Qt6Keychain found in configure output, full build succeeded, DLL present in artifact. Closes aethersdr#3600 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
335f92e to
6c598a8
Compare
Closes #3600
Problem
SmartLink credential persistence — saving the Auth0 refresh token to the OS credential store so users aren't prompted for their password on every launch — is fully implemented in application code behind
#ifdef HAVE_KEYCHAIN.HAVE_KEYCHAINis defined only when CMake findsQt6Keychainat configure time.On Windows CI,
find_package(Qt6Keychain QUIET)has always returned not-found: nothing in the Windows build pipeline installs or builds qtkeychain. The feature silently compiled out of every Windows release artifact (installer, portable ZIP, MSIX), making credential persistence a no-op for all Windows users.macOS Apple Silicon already has it via
brew install qtkeychain. Linux AppImage and macOS Intel are a separate gap tracked in a follow-on issue.Root cause
find_package(Qt6Keychain QUIET)fails silently →HAVE_KEYCHAINundefined →saveCredentials()andtryAutoLogin()inSmartLinkClient.cppcompile as empty no-ops.Implementation
Follows the established pattern used by hidapi, FFTW3, and DeepFilterNet3: a setup script downloads and builds from source, CI caches the result, CMake finds it via a staged
third_party/directory.scripts/setup/setup-qtkeychain.ps1(new)QT_ROOT_DIR(set byinstall-qt-action) so the qtkeychain ABI matches the application's Qt exactly-DBUILD_TRANSLATIONS=OFFto avoid aQt6LinguistToolsdependency on unused locale filescmake --installto stage the full package layout — headers, import lib, DLL, and cmake config files — tothird_party/qtkeychain/; the cmake config files are whatfind_package(Qt6Keychain)actually resolves againstCMakeLists.txtWIN32, prepends${CMAKE_SOURCE_DIR}/third_party/qtkeychaintoCMAKE_PREFIX_PATHimmediately before the existingfind_package(Qt6Keychain QUIET)call.github/workflows/windows-installer.ymlthird_party/qtkeychain, keyed on the script hash (invalidates on version bump)qt6keychain.dllpre-staged to the Qt bin directory beforewindeployqtruns —windeployqttreats anyQt6*DLL as an official Qt module and looks for it in the Qt installation to scan transitive dependencies; without this it aborts with exit code 1qt6keychain.dllcopy in the deploy block covers all three artifacts (portable ZIP, Inno installer, MSIX)No application code changes
The credential save/load paths in
SmartLinkClient.cppare already correct and already work on macOS Apple Silicon. This PR only makes the Windows build pipeline supply the dependency those paths require.Testing
Validated end-to-end via
workflow_dispatchonNF0T/AetherSDR(run 27658397042):Setup qtkeychainstep completed — qtkeychain 0.16.0 built against Qt 6.8.3 MSVC 2022Qt6Keychain found — SmartLink credential persistence enabledHAVE_KEYCHAINdefinedqt6keychain.dllpresent in artifactPrinciple XI (Fixes Are Demonstrated).
🤖 Generated with Claude Code