-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cask: read bundle version from Info.plist
when sensible.
#16826
Conversation
Looks ok, though there's a mild concern that |
3ec8d1d
to
ee6b7ea
Compare
Info.plist
when sensible.Info.plist
when sensible.
@Bo98 Good point. I was on the fence between two approaches and you've nudged me into the other one: adding dedicated new fields to the JSON output for this use specifically. |
I agree with that approach - seems simplest overall and keeps existing In the long term, ideally we'd handle uninstalls better and then be able to merge the two. Maybe there's some opportunity to use GitHub Packages to store versioned info, but that is a much bigger task to tackle and not something for today. |
ee6b7ea
to
178b930
Compare
I'm still thinking on this, but there may be some other (rare) edge cases? Something else is changed in the cask file, like adding a binary, and it isn't updated for the user because it was "auto updated" and linking was then missed because we report the cask as being updated. |
91fd5e2
to
ff5dd33
Compare
If you're trying to use `brew info --json=v2` to get an installed version and figure out if it is outdated: you're going to have a bad time with `auto_updates` casks because `installed_version` alone is not enough to get the actually currently installed version of the app. Instead, in these cases, try to read from `Info.plist` if there is one and use that version. While we're here, add a `blank?` method to `Version` so we can use it for `present?` checks (making a `null?` `Version` object `blank?`). Co-authored-by: Markus Reiter <[email protected]>
ff5dd33
to
03e583e
Compare
I don't think any of those would be affected by the current iteration of the PR. The earlier iteration, as @Bo98 mentioned, having I still think there's room to be smarter here (particularly around |
Thanks for review all! |
Do we want to include this information in the JSON API? Currently, we're not parsing it in the |
Nah, it's something purely based on local app install state rather than anything in the cask itself. |
Ah, okay. That wasn't obvious to me at a glance. I guess it's just another example of something that makes sense as metadata for users but isn't needed in the API which is why v3 is necessary in the first place. |
If you're trying to use
brew info --json=v2
to get an installed version and figure out if it is outdated: you're going to have a bad time withauto_updates
casks becauseinstalled_version
alone is not enough to get the actually currently installed version of the app.Instead, in these cases, try to read from
Info.plist
if there is one and use that version.While we're here, add a
blank?
method toVersion
so we can use it forpresent?
checks (making anull?
Version
objectblank?
).