Conversation
bundle_version initial implementation
There was a problem hiding this comment.
Pull request overview
Adds initial bundle_version support for cask app artifacts to enable “metadata-only” upgrades when an installed app bundle already matches the incoming cask’s version + bundle_version, avoiding unnecessary downloads/reinstalls. This also extends API serialization/loading and introduces an online audit to validate declared bundle_version values against extracted artifacts.
Changes:
- Add
bundle_versionoption toappartifacts, including API JSON serialization/loading and artifact matching logic. - Implement metadata-only upgrade path (and skip prefetch/fetch where applicable) when installed bundle version matches.
- Add
brew audit --onlinechecks and unit tests covering metadata-only upgrade and bundle version auditing.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Library/Homebrew/cask/artifact/abstract_artifact.rb | Adds default “metadata-upgrade” capability hooks to artifacts. |
| Library/Homebrew/cask/artifact/app.rb | Implements bundle_version option + matching against installed Info.plist. |
| Library/Homebrew/cask/upgrade.rb | Adds metadata-only upgrade flow and fetch filtering for upgrades. |
| Library/Homebrew/cmd/upgrade.rb | Skips prefetching downloads for casks eligible for metadata-only upgrades. |
| Library/Homebrew/cask/installer.rb | Adds write_metadata helper used by metadata-only upgrades. |
| Library/Homebrew/cask/audit.rb | Adds online audit for declared bundle_version vs extracted app bundle version. |
| Library/Homebrew/test/cask/upgrade_spec.rb | Tests metadata-only upgrade behavior and prefetch skipping. |
| Library/Homebrew/test/cask/audit_spec.rb | Tests online audit behavior for bundle version matching/mismatch. |
| Library/Homebrew/test/cask/artifact/app_spec.rb | Tests bundle_version parsing/storage and matching logic on App artifacts. |
| Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb | Tests loading bundle_version from JSON and internal JSON API artifacts. |
| Library/Homebrew/test/api/cask/cask_struct_generator_spec.rb | Updates API artifact processing expectations for bundle_version. |
| Library/Homebrew/test/api/cask_spec.rb | Updates expected cask source checksum due to fixture changes. |
| Library/Homebrew/test/support/fixtures/cask/Casks/everything.rb | Adds bundle_version to fixture cask’s app stanza. |
| Library/Homebrew/test/support/fixtures/cask/everything.json | Updates API fixture artifacts to include bundle_version and checksum. |
| Library/Homebrew/test/support/fixtures/cask/everything.internal.json | Updates internal API fixture artifacts to include bundle_version and checksum. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| add_error "Unable to determine bundle version for App '#{artifact.source.basename}'" \ | ||
| "bundle version should be removed from the artifact if it cannot be determined reliably." |
There was a problem hiding this comment.
The error message concatenation here is missing punctuation/whitespace: the two adjacent string literals join into "...App 'X'bundle version...". Add a separating space/newline (and ideally a period) so the audit output is readable.
| add_error "Unable to determine bundle version for App '#{artifact.source.basename}'" \ | |
| "bundle version should be removed from the artifact if it cannot be determined reliably." | |
| add_error "Unable to determine bundle version for App '#{artifact.source.basename}'." \ | |
| " Bundle version should be removed from the artifact if it cannot be determined reliably." |
| fetch_upgradable_casks = fetch_required_casks(upgradable_casks.map(&:last)) | ||
|
|
||
| created_download_queue = T.let(false, T::Boolean) | ||
| download_queue ||= if !dry_run && !skip_prefetch | ||
| created_download_queue = true | ||
| Homebrew::DownloadQueue.new(pour: true) | ||
| end | ||
|
|
||
| if !dry_run && !skip_prefetch | ||
| if !dry_run && !skip_prefetch && fetch_upgradable_casks.present? |
There was a problem hiding this comment.
fetch_required_casks/metadata_upgrade_available? relies on artifact target paths, which depend on cask.config (e.g., custom --appdir). At this point the new_cask configs haven’t been merged with the installed cask’s config yet (that happens later in upgrade_cask), so this prefetch filtering can be wrong and trigger unnecessary download prefetches. Consider merging each new_cask’s config with the corresponding installed cask’s config before calling metadata_upgrade_available? in the prefetch phase.
|
I think we may also need to add an audit to not allow the use of |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks for the PR @bevanjkay but I think this needs a different approach.
I think we may also need to add an audit to not allow the use of bundle_version when the download url is unversioned, because the download of a specific version is not guaranteed in that case.
This is revealing because I think, unfortunately, it makes the DSL pointless in that case. The "problem to be solved" also applies when the download URL is unversioned, unless I've missed something.
Basically: we want a way to have brew upgrade --greedy be the default but avoid upgrading things where the bundle version indicates it's already up-to-date. We may still want to update e.g. the caskroom version in that case so we don't keep repeatedly needing to check the same data.
We have a bundle_version attribute in our existing cask JSON API that sniffs this bundle version. That's what's already been used by other tools to do this sort of comparison.
Hope that helps, let me know what you think as I may be totally wrong about some of the above!
| "Everything.app" | ||
| "Everything.app", | ||
| { | ||
| "bundle_version": "1234" |
There was a problem hiding this comment.
there's already a bundle_version parameter in this file that's "bundle_version": null. we should use that instead.
There was a problem hiding this comment.
The implementation in the PR is a bit deeper as it can work across multiple app stanzas. bundle_version may not always be global to the entire cask, which is why it is a parameter on the artifact instead.
I didn't look too deeply but the API value for the existing bundle_version seems to always return nil, so I assumed it is legacy.
| rename "Foo.app", "Bar.app" | ||
|
|
||
| app "Everything.app" | ||
| app "Everything.app", bundle_version: "1234" |
There was a problem hiding this comment.
I'm not sure this is the right approach for this DSL/API, sorry.
This seems like we'll just need to adjust all tooling and user behaviour to ensure this is bumped every time the version is?
Instead, I think we should aim to essentially have some sort of "this is how you convert from version to bundle version when they differ" or "the bundle and app version are the same" DSL(s).
There was a problem hiding this comment.
I'll look into this some more, but my concern here is how often the cask "version" may diverge from the "bundle_version".
I'll get some real data and come back around.
There was a problem hiding this comment.
Thanks @bevanjkay! Yeh, I think data will help here a lot.
There was a problem hiding this comment.
Okay, the results from a preliminary analysis;
To be eligible as a target;
- I had to have the cask installed
- It has to only have a single
appartifact
This resulted in 99 eligible casks.
I then expanded the comparison to check the following;
- Does
versionmatch eitherCFBundleVersionorCFBundleShortVersionString - Does
version.csv.any?match eitherCFBundleVersionorCFBundleShortVersionString- Some mild concern matching this naively, because some may only increment the secondary version string for additional builds.
The match rate was 82.8% - which I was actually somewhat surprised by.
In summary, I think that you're right here, using a more fundamental (or naive) matching system is probably the way to go for now as it will work in many cases without any lift. But I can't see this being good enough to make --greedy the default in the short->medium term.
Unversioned downloads are always a worse UX because there's no guarantee the version in the cask will match |
This seems better than nothing if we can 👍🏻 |
brew lgtm(style, typechecking and tests) with your changes locally?I used Codex to help plan out and implement the initial version of
bundle_version. I have reviewed and tested the functionality, but anticipate that there will be some edge-cases.This PR introduces an initial implementation of a
bundle_versionparameter for cask artifacts. In this first iteration, it only applies toappartifacts, which is the primary target for this feature.The goal is to support metadata-only upgrades when an app has already been updated outside Homebrew, for example by an in-app updater. If a newer cask is available in Homebrew, and the installed app bundle already matches the incoming cask’s version and
bundle_version, Homebrew updates its installed cask metadata without fetching or reinstalling the app.Note that this only applies when the version matches, so
brew upgrade <cask>would still run an installation when the installed version is greater than the Homebrew version. A future possible enhancement (out of scope here) could be to handle cases where the installed app is never than the incoming cask version.I’ve kept this PR scoped to the MVP. For broader adoption, we would also want
brew bumpandbrew bump-cask-prsupport to detect and updatebundle_versionautomatically. I intend to work on this as a follow-up, unless there is preference to include it here.Included in this PR;
bundle_versionsupport forappartifactsbundle_versionbrew audit --onlinefunctionality to check thebundle_versionvalues against the artifactFollow-up work:
bundle_versiondetection and update support inbrew bumpandbrew bump-cask-pr