Only upgrade :latest casks when --greedy and the cask has been updated #13275
Conversation
Library/Homebrew/cask/cask.rb
Outdated
| @new_download_sha ||= Installer.new( | ||
| self, verify_download_integrity: false | ||
| ).download(quiet: true).instance_eval { |x| Digest::SHA256.file(x).hexdigest } |
There was a problem hiding this comment.
| @new_download_sha ||= Installer.new( | |
| self, verify_download_integrity: false | |
| ).download(quiet: true).instance_eval { |x| Digest::SHA256.file(x).hexdigest } | |
| @new_download_sha ||= Installer.new( | |
| self, verify_download_integrity: false | |
| ).download(quiet: true) | |
| .instance_eval { |x| Digest::SHA256.file(x).hexdigest } |
There was a problem hiding this comment.
I think I'll just line up .new, .download and .instance_eval vertically instead.
Library/Homebrew/cask/cask.rb
Outdated
| if greedy || greedy_latest || (greedy_auto_updates && auto_updates) | ||
| if latest_version.latest? | ||
| return outdated_download_sha? ? versions : [] | ||
| end | ||
| elsif auto_updates | ||
| return [] | ||
| end |
There was a problem hiding this comment.
| if greedy || greedy_latest || (greedy_auto_updates && auto_updates) | |
| if latest_version.latest? | |
| return outdated_download_sha? ? versions : [] | |
| end | |
| elsif auto_updates | |
| return [] | |
| end | |
| if greedy || greedy_latest || (greedy_auto_updates && auto_updates) | |
| if latest_version.latest? | |
| if outdated_download_sha? | |
| return versions | |
| else | |
| return [] | |
| end | |
| end | |
| elsif auto_updates | |
| return [] | |
| end |
There was a problem hiding this comment.
Unfortunately, this doesn't pass the linter. We could use a guard clause but that seems even uglier than the ternary operator imo. Which do you prefer?
# before
return outdated_download_sha? ? versions : []
# after
return versions if outdated_download_sha?
return []There was a problem hiding this comment.
after
return versions if outdated_download_sha?
return []
This seems nice!
Maybe? I'm not super familiar with I don't think adding any of those things would simplify the logic per se but it could help reduce unnecessary downloads and shas. As far as I can see, a lot of that work is already being done in the download method where it defaults to the cached download if the remote one hasn't been updated (now that I write this maybe something like With the current code the following things need to be checked when trying to verify if a
Would it be possible to just use the I think the next thing I'll do is look at how the downloader decides when to use the cached download. This could in theory be used to decide when casks need updating. |
|
Okay, I took a look around and I think that HTTP
So in conclusion, we'd still need to use hashing as a backup way to make sure we aren't reinstalling the same cask when upgrading unless we're fine with a 90% solution (that's just an estimate based on looking at casks in the default tap). It also would mean adding a method/class for retrieving and checking |
|
I might be wrong but it seems like at least the I believe the download still gets cached though so one solution would be to store that pathname in the cask object instead of the installer. Then, if a cask uses an installer to download a file twice during the same session, it will default to the cached download. Also, is HTTP |
I think it asks
Unsure! To be clear: was just spitballing enough and I'm happy for this to be merged as-is when you are 👍🏻 |
|
Sounds good. I think those were some good ideas and it makes sense to follow up on them. I'll keep poking around for now. |
A sha256 hash of the previous download is stored and compared with new downloads before updating :latest casks. This prevents unnecessary reinstalls when the cask hasn't been updated. Move download path to cask from installer to prevent unnecessary redownloads of casks.
081d853 to
b85f407
Compare
|
Okay, well after looking around I just don't think it's really worth it to pursue the HTTP Essentially, there are a few reasons for this.
That being said I did change one thing and you'll have to tell me what you think. Essentially, I decided to store the download path in the Cask object instead of the Installer. This means that the download will only happen once during the lifetime of a Cask object. It passes all of the tests but there might be a scenario I'm not thinking of where you'd need to download a Cask multiple times in one go. It looks like this now. def download(quiet: nil, timeout: nil)
# Store cask download path in cask to prevent multiple downloads in a row when checking if it's outdated
@cask.download ||= downloader.fetch(quiet: quiet, verify_download_integrity: @verify_download_integrity,
timeout: timeout)
endOther than that this PR should be ready. |
Thanks for investigating and the great write-up ❤️
Amazing work! |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?This PR is related to issue #11513 where
version :latestcasks would always be reinstalled when the--greedyor--greed-latestflags were passed tobrew upgraderegardless of whether or not the casks had been updated.Basically the idea is to store a sha256 hash of the original download file every time a
version :latestcask is installed. Then, before upgrading caskscask#outdated?is queried which internally compares the new and old sha ofversion :latestcasks to see if they should be updated. As far as I can see, the first download used to check the sha should be cached so it shouldn't need to be downloaded again before installing it.The sha is stored in a file called
LATEST_DOWNLOAD_SHA256in the.metadata/cask subdirectory. If there is no previous sha stored, it just automatically assumes the cask should be updated.Fixes #11513.