Skip to content
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

Merged
merged 1 commit into from Mar 6, 2024

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Mar 5, 2024

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?).

@MikeMcQuaid MikeMcQuaid requested a review from Bo98 March 5, 2024 16:51
@Bo98
Copy link
Member

Bo98 commented Mar 5, 2024

Looks ok, though there's a mild concern that installed_caskfile might now never be updated and thus brew uninstall may not work correctly.

@MikeMcQuaid MikeMcQuaid changed the title cask: read installed_version from Info.plist when sensible. cask: read bundle version from Info.plist when sensible. Mar 5, 2024
@MikeMcQuaid
Copy link
Member Author

@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.

@Bo98
Copy link
Member

Bo98 commented Mar 5, 2024

I agree with that approach - seems simplest overall and keeps existing --greedy-auto-updates behaviour untouched.

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.

@MikeMcQuaid MikeMcQuaid requested a review from a team March 5, 2024 17:27
@bevanjkay
Copy link
Member

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.

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 <me@reitermark.us>
@MikeMcQuaid
Copy link
Member Author

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.

I don't think any of those would be affected by the current iteration of the PR. The earlier iteration, as @Bo98 mentioned, having installed_caskfile act differently could be detrimental.

I still think there's room to be smarter here (particularly around --greedy) to avoid repeatedly redownloading the same content.

@MikeMcQuaid MikeMcQuaid merged commit 45cd50b into master Mar 6, 2024
26 checks passed
@MikeMcQuaid MikeMcQuaid deleted the cask_installed_version_plist branch March 6, 2024 16:09
@MikeMcQuaid
Copy link
Member Author

Thanks for review all!

@apainintheneck
Copy link
Contributor

Do we want to include this information in the JSON API? Currently, we're not parsing it in the Cask::CaskLoader::FromAPILoader but we could. I'm not sure how important that is though. I'm also sort of wondering if we want it to be included in the JSON API v3 as well.

@Bo98
Copy link
Member

Bo98 commented Mar 8, 2024

Nah, it's something purely based on local app install state rather than anything in the cask itself.

@apainintheneck
Copy link
Contributor

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.

@github-actions github-actions bot added the outdated PR was locked due to age label Apr 7, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants