Install app_image by moving the AppImage, mirroring the app stanza - #23771
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The migration misses dangling legacy links and can prevent rollback during upgrades.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Changes AppImage installation to use the standard moved-artifact model, preventing self-updates from breaking user-facing links.
Changes:
- Moves AppImages into
appimagedirwith Caskroom backlinks. - Adds legacy-layout migration handling.
- Adds AppImage fixtures and lifecycle tests.
File summaries
| File | Description |
|---|---|
Library/Homebrew/cask/artifact/appimage.rb |
Implements moved installation and legacy cleanup. |
Library/Homebrew/test/cask/artifact/appimage_spec.rb |
Tests installation, updating, adoption, migration, and summaries. |
Library/Homebrew/test/support/fixtures/cask/Casks/with-appimage.rb |
Adds a test cask. |
Library/Homebrew/test/support/fixtures/cask/naked.AppImage |
Adds an AppImage test fixture. |
Review details
Suppressed comments (1)
Library/Homebrew/cask/artifact/appimage.rb:64
- Deleting the real
sourcehere makes the first upgrade from the legacy layout non-rollbackable.Installer#start_upgradecalls this withupgrade: trueand then backs up the staged directory; if the new installation fails,revert_upgraderestores that directory and callsinstall_artifacts, but the AppImage source is now gone and installation raises. Keep the source during upgrades so it is included in the backup; successful finalization will purge that backup.
Utils.gain_permissions_remove(source, command:)
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
What about those that declare a version? |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks! A few questions/thoughts.
Currently `app_image` installs the real AppImage into a versioned Caskroom directory and puts the user-facing entry at `config.appimagedir` (default `~/Applications`) as a *symlink* back to it, via `Artifact::Symlinked`. That symlink exposes the *versioned* Caskroom path as `$APPIMAGE` to the running application, which means that casks that use electron-updater for auto-updates will go through its `doInstall()` method which changes based on the `$APPIMAGE` filename: if it contains an `X.Y.Z` version, it writes a *new* versioned filename and `unlinkSync`s the old one; only a version-less basename results in an overwrite-in-place update [^1]. Therefore, because the exposed path to the AppImage is indeed versioned, electron-updater applies that rename logic, which results in a new `<App>-<newver>.AppImage` (inside the _older_ version's Caskroom dir!), the old file being deleted. Worst of all: the `appimagedir` symlink is left dangling until a manual `brew reinstall` is executed. This patch inverts the relationship, matching `Cask::Artifact::App` (i.e. the macOS way): `AppImage` now subclasses `Moved` so the real AppImage lives at the stable `appimagedir` target and the Caskroom holds the back-symlink. Most `app_image` casks already declare a version-less `target:` (e.g. `Obsidian.AppImage`, `Heroic.AppImage`), so as a result, `$APPIMAGE` becomes version-less and electron-updater overwrites them in place; i.e. the real file stays put across self-updates, and nothing user-facing breaks. If some other AppImage updater renames anyway, only the disposable Caskroom-side pointer is affected, not the user's file. `resolve_target` is unchanged, so `appimagedir` and custom targets behave exactly as before; the old `link` override becomes an `install_phase` override that makes the moved target executable (by overriding `install_phase`, which is `overridable`, rather than `Moved#post_move`, which is not). This introduces no new home-directory destination: `app_image` already writes to `config.appimagedir`, whose documented default `~/Applications` was explicitly proposed and accepted when the stanza landed [^2] [^3] [^4]; this patch only changes that entry from a symlink to the real file, plus a Caskroom backlink. Real artifacts under `$HOME` are already normal for `Moved` subclasses (e.g. `Font`, `Dictionary`, `ScreenSaver`, ...). Linux already extends `Moved` for `cp --reflink=auto` and no-ops the macOS-only altname xattr, so no new cross-platform machinery is required. This change affects all `app_image` casks, not only self-updaters; gating on `auto_updates` would result in two opposing placement/uninstall models for the same artifact class and rely on casks classifying their updater correctly. `app_image` has shipped since 5.1.12, so users already have AppImage installs with the old `Symlinked` layout. `brew upgrade` self-heals (it force-uninstalls then reinstalls), but a non-forced `brew uninstall`/`reinstall` would fail, because `Moved#move_back` only removes `source` when it is a symlink to `target`. Therefore, a small shim was added to `uninstall_phase`, to detect the old layout and reverse it directly. This mirrors how `migrator.rb` already tolerates state from an earlier migration era. This addresses AppImage updaters that replace the filename, specifically electron-updater. Other major updaters are not affected: Tauri's updater replaces `$APPIMAGE` in place and keeps the filename [^5], and AppImageUpdate/zsync2 updates in place, moving the old file aside as `.zs-old` [^6]. electron-updater is the one updater that renames by default, due to the version-in-basename heuristic that this patch neutralizes. A hypothetical updater that would rename the *target* to a different name would fall back to the inherited `Moved` behavior (it would report `Missing AppImage`). The Brew 6 `auto_updates` upgrade check [^7] cannot help here since `Cask#auto_updates_bundle_outdated?` requires an `Artifact::App` with an `Info.plist`, which an AppImage lacks. [^1]: https://github.com/electron-userland/electron-builder/blob/master/packages/electron-updater/src/AppImageUpdater.ts [^2]: Homebrew#20334 [^3]: Homebrew#20334 (comment) [^4]: Homebrew#20334 (comment) [^5]: tauri-apps/tauri#6279 [^6]: https://github.com/AppImageCommunity/AppImageUpdate [^7]: https://github.com/orgs/Homebrew/discussions/6916
This is partially addressed in the PR description where I mentioned a similar failure case:
An As a concrete example, if the example cask above (OpenWhispr) had a versioned
and with this PR:
I should point out that this is exceedingly rare, btw: out of all 100+ existing
That single exception is easily addressable by editing the cask file, but we could certainly add a check to the cask audit to ensure this doesn't recur. Should I fold such a check into this PR? (Again, even without fixing that cask, this PR already leads to a better outcome than what we currently have.) |
740a473 to
d154fcc
Compare
No, a new PR would be ideal, thanks. Please open PRs to:
🙇🏻 |
AppImages will soon require (via a new audit rule) a non-versioned target name to ensure that those that rely in electron-updater don't break installs (see Homebrew/brew#23771 for context). This fix prepares the bruno cask to pass the upcoming audit, and brings it to parity with other AppImage casks.
👉️ #23777
|
`Homebrew::DevCmd::Tests#setup_environment!` deletes every `HOMEBREW_*` config var not in its `allowed_test_env` allowlist, for test isolation. `HOMEBREW_NO_SANDBOX_LINUX` (and `HOMEBREW_SANDBOX_LINUX`) were not on that list, so the documented opt-out was stripped before `OS::Linux::DevCmd::Tests#check_test_environment!` read it: `sandbox_linux?` fell back to its default (true) and the Linux Landlock check ran regardless. That is why the same var reads false under `brew ruby` but effectively true under `brew tests`. On a dev host whose Ruby lacks Fiddle (Ruby 3.5/4.0 dropped it from the default gems; portable-ruby on CI still has it), the Landlock sandbox cannot initialise, so `brew tests` aborts with "Landlock requires Ruby's bundled Fiddle library" and the documented opt-out is the only escape hatch — which didn't work. This surfaced while running `brew lgtm` locally for Homebrew#23771. [^1] Add both sandbox toggles to `allowed_test_env` so the opt-out survives the cleanup and `check_test_environment!` can honor it. Default behavior is unchanged: without the toggle the sandbox is still enforced. Note: `HOMEBREW_SANDBOX_LINUX` / `HOMEBREW_NO_SANDBOX_LINUX` are `odeprecated`; maintainers may prefer removing the toggle entirely over restoring it. Worth confirming the intended direction in review. [^1]: Homebrew#23771
Today
app_imageinstalls the real AppImage into a versioned Caskroom directory and puts the user-facing entry atconfig.appimagedir(default~/Applications) as a symlink back to it (viaArtifact::Symlinked). For example:~/Applications/OpenWhispr.AppImage->/home/linuxbrew/.linuxbrew/Caskroom/openwhispr/1.8.3/OpenWhispr-1.8.3-linux-x86_64.AppImage.That symlink exposes the versioned Caskroom path as
$APPIMAGEto the running app. For casks that self-update with electron-updater,doInstall()branches on the$APPIMAGEfilename: if the basename contains anX.Y.Zversion, it writes a new versioned filename andunlinkSyncs the old one; conversely, version-less basenames are updated in place. Because the exposed path (the Caskroom one) is versioned,doInstall()'s rename branch is followed: a new<App>-<newver>.AppImagelands inside the older version's Caskroom dir (e.g. /home/linuxbrew/.linuxbrew/Caskroom/openwhispr/1.8.3/OpenWhispr-1.9.2-linux-x86_64.AppImage), the old file is deleted, and theappimagedirsymlink (at~/Applications) is left dangling until the user manually runsbrew reinstall.This patch inverts the relationship to match
Cask::Artifact::App(i.e. the macOS flow):AppImagenow subclassesMovedinstead ofSymlinked, so the real AppImage lives at the stable, unversionedappimagedirtarget, and the Caskroom holds the versioned back-symlink. Mostapp_imagecasks already declare a version-lesstarget:(e.g.Obsidian.AppImage,Heroic.AppImage), so$APPIMAGEbecomes version-less and electron-updater overwrites in place — the file stays put across self-updates and nothing user-facing breaks. (If some other AppImage updater renames anyway, only the disposable Caskroom-side pointer is affected, not the user's file.)Note: This change introduces no new home-directory destination:
app_imagealready writes toconfig.appimagedir, whose documented default~/Applicationswas explicitly proposed and accepted when the stanza landed (#20334). This only changes that entry from a symlink to the real file, plus a Caskroom backlink — the same modelAppand otherMovedartifacts (Font,Dictionary,ScreenSaver, ...) already use. Linux already extendsMoved, so no new cross-platform machinery is needed.The change applies to all
app_imagecasks, not only self-updaters: gating onauto_updateswould mean two opposing placement/uninstall models for the same artifact class.On backward compatibility:
app_imagehas shipped since 5.1.12, so users already have AppImages installed with the current symlink setup (appimagedir→$APPIMAGE). To account for this, a small, clearly-marked migration shim inuninstall_phasereverses the oldSymlinkedon-disk layout directly (a plainMoved#move_backwould otherwise error on a non-forced uninstall of a pre-inversion install).Validation
test/cask/artifact/appimage_spec.rbcovering placement + backlink, the executable bit, in-place self-update, adopt (with and withoutauto_updates), uninstall, migration from the oldSymlinkedlayout, and installed-summary output. It runs with the rest of the cask suite on macOS CI.brew benchmarkresults.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?Note:
brew lgtmpassesbrew typecheckandbrew style --changed --fix, but on my Linux machinebrew tests --changedcannot run due to missing the Fiddle library.That's probably a bug to be fixed separately; either way, CI will validate the changes.
I used GPT 5.6 Sol to perform the initial investigation and draft an implementation plan, and then Claude Opus 4.8 to implement and run the tests. I reviewed every change and validated the tests locally as described above.