Skip to content

B49/B50: repoint FastFlowLM at ROCm org; fix msi asset matcher + broken FLM detection - #42

Open
agr77one wants to merge 1 commit into
mainfrom
fix/flm-rocm-org-msi-asset
Open

B49/B50: repoint FastFlowLM at ROCm org; fix msi asset matcher + broken FLM detection#42
agr77one wants to merge 1 commit into
mainfrom
fix/flm-rocm-org-msi-asset

Conversation

@agr77one

@agr77one agr77one commented Aug 25, 2026

Copy link
Copy Markdown
Owner

Summary

Upstream FastFlowLM now lives at ROCm/FastFlowLM and ships flm-setup.msi (latest v1.0.2). GitHub 301-redirects the old org path, so nothing was outright broken — but this points at the canonical location rather than leaning on a redirect indefinitely, and fixes two real defects found along the way.

B49 — .exe-only release-asset matcher

check_flm_update() selected release assets by .exe suffix only, so every check since FLM v1.0.1 returned asset_url="". Latent rather than user-visible: the dashboard's "Download update…" button uses release_url, and nothing in the repo consumes asset_url. Now prefers .msi with .exe retained as a fallback so an older/rolled-back release still resolves.

Live-verified against the real API: 0.9.45 → 1.0.2, asset_url.../v1.0.2/flm-setup.msi.

B50 — FLM detection was broken for both installer formats

This resolves the UNVERIFIED caveat shipped with B47 in 2.5.0 — as broken, and worse than flagged.

NeedsFLM() / FlmUninstallCmd() matched the uninstall subkey name against "flm version ". The real subkey names are:

Format Subkey DisplayName Hive
Inno (legacy) flm_is1 flm version 0.9.45 32-bit
MSI (v1.0.1+) {D0F67DEE-…} flm 64-bit

So it matched neither"flm version 0.9.45" is only the DisplayName, never the subkey. The path fallback probed PF\FastFlowLM\flm.exe, but FLM actually installs to PF\flm\. Both signals failed, so NeedsFLM() reported "absent" on a machine with FLM installed and running — meaning the installer redundantly chain-installs FLM on every run, and never drops .flm_installed_by_us, so the uninstall chain never fires.

Fix: match DisplayName across both HKLM64 and HKLM32 (this installer runs 64-bit per ArchitecturesInstallIn64BitMode, so a plain HKLM never saw the legacy 32-bit entry), probe the real install path first, and derive a quiet uninstall from UninstallString when QuietUninstallString is empty (the MSI publishes it empty).

Verified by simulating the new logic against a real registry carrying both entries:

  • NeedsFLM()False (detected; previously True)
  • FlmUninstallCmd()MsiExec.exe /X{D0F67DEE-…} /quiet /norestart

Self-correction

This branch's first draft of B49 also claimed vendor/.gitignore lacking flm/*.msi made a 26 MB installer commitable. That was wrong — caught by the pre-merge review pass. Root .gitignore's vendor/* already excludes the subtree, and git never descends into an excluded directory, so every rule inside vendor/.gitignore is unreachable (git check-ignore -v resolves to .gitignore:36; git add is refused). The added line is kept as dead-but-consistent with its equally-dead neighbours, and SPEC.md now records the correction rather than the false claim.

Test plan

  • ruff check . clean; full suite green (539 tests, +3 new)
  • New regression tests cover .msi selection, .exe fallback, and the ROCm repoint
  • Detection logic simulated against a real dual-entry registry
  • installer.iss checked for the B48 brace-constant-in-Pascal-comment hazard (0) and begin/end balance (23/23)
  • CI green — iscc compilation is only covered here (no local Inno Setup)

🤖 Generated with Claude Code

Upstream FastFlowLM now lives at ROCm/FastFlowLM and ships flm-setup.msi
(latest v1.0.2). GitHub 301-redirects the old org path, so nothing was
broken -- but point at the canonical location rather than lean on a
redirect indefinitely.

B49: check_flm_update() matched release assets by .exe suffix only, so every
check since FLM v1.0.1 returned asset_url="" (latent -- the dashboard button
uses release_url, so no consumer noticed). Prefer .msi, keep .exe as a
fallback for older/rolled-back releases. Live-verified 0.9.45 -> 1.0.2 now
resolves the real asset URL.

B50: resolves B47's UNVERIFIED flag as BROKEN, and worse than flagged.
NeedsFLM()/FlmUninstallCmd() matched the uninstall SUBKEY NAME against
"flm version ", but the real subkey names are flm_is1 (Inno -- "flm version
0.9.45" is only its DisplayName) or a product-code GUID (MSI), so it matched
NEITHER installer format. The path fallback probed PF\FastFlowLM\flm.exe
while FLM actually installs to PF\flm\. Both signals failed, so NeedsFLM()
reported "absent" on a machine with FLM installed and running -- meaning the
installer redundantly chain-installs FLM on every run and never drops
.flm_installed_by_us, so the uninstall chain never fires.

Now matches DisplayName across both HKLM64 and HKLM32 (this installer runs
64-bit, so a plain HKLM never saw the legacy 32-bit entry), probes the real
install path first, and derives a quiet uninstall from UninstallString when
QuietUninstallString is empty (the MSI publishes it empty). Verified by
simulating the new logic against a real registry carrying BOTH entries:
NeedsFLM -> False, cmd -> MsiExec.exe /X{GUID} /quiet /norestart.

Also corrects a factually wrong claim in this branch's first draft of B49
(review-caught): vendor/.gitignore lacking flm/*.msi did NOT make the
installer commitable -- root .gitignore's "vendor/*" already excludes the
subtree and git never descends into an excluded directory, so every rule in
vendor/.gitignore is unreachable. The added line is kept as dead-but-
consistent with its equally-dead neighbours; the record now says so.

iscc compilation is covered by CI (no local Inno Setup).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 077a41861e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread installer/installer.iss
Comment on lines +272 to +275
if FindFlmUninstallKey(HKLM64, FoundKey) then
begin
Result := False;
Exit;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve pre-existing FastFlowLM installations

When FastFlowLM is already installed, this new registry lookup makes NeedsFLM() return false, so the [Files] and [Run] entries correctly skip installation; however, CurStepChanged(ssPostInstall) calls NeedsFLM() again and creates .flm_installed_by_us whenever it returns false. Consequently, installing Flowkey alongside a user-managed FLM now marks that runtime as Flowkey-owned, and [UninstallRun] later removes it when Flowkey is uninstalled. Capture whether FLM was absent before the install and create the marker only when this setup actually installed it successfully.

Useful? React with 👍 / 👎.

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.

2 participants