Implement brew cask upgrade
#3396
Conversation
What/where exactly is step 7? Anyways, here is what I had in mind to handle this case: Currently we have a |
|
Here:
|
|
|
|
/cc for review @reitermarkus |
There was a problem hiding this comment.
Currently we have a
#installand#uninstallmethod for every artifact class. In#uninstallthe artifact is usually completely removed. To handle a failed upgrade, we should instead make#uninstalldo the exact opposite of#install, i.e. if#installmoves a file from A to B,#uninstallshould move B back to A.
This part hasn't been addressed yet. Probably wasn't clear because I said #uninstall when I meant #uninstall_phase (which is called in #uninstall_artifacts).
| end | ||
|
|
||
| def run | ||
| outdated_casks = casks(alternative: -> { Hbc.installed }).find_all { |cask| cask.outdated?(greedy?) } |
There was a problem hiding this comment.
Use select instead of find_all.
| outdated_casks = casks(alternative: -> { Hbc.installed }).find_all { |cask| cask.outdated?(greedy?) } | ||
|
|
||
| if outdated_casks.empty? | ||
| oh1 "No packages to upgrade" |
There was a problem hiding this comment.
I think it should just return here.
| @@ -28,6 +28,7 @@ def initialize(cask, command: SystemCommand, force: false, skip_cask_deps: false | |||
| @verbose = verbose | |||
| @require_sha = require_sha | |||
| @reinstall = false | |||
| @upgrade = upgrade | |||
| end | |||
|
|
|||
| attr_predicate :binaries?, :force?, :skip_cask_deps?, :require_sha?, :verbose? | |||
There was a problem hiding this comment.
Add :upgrade? here and use upgrade? instead of @upgrade in other methods.
|
@reitermarkus, I'm afraid I don't yet understand your intended So far, I came up with this flow:
Since I reuse the |
This is what I want to avoid, because often the old version is not available anymore. We already had it installed before, so we can reinstall from the previous local version. So you are right that we don't want to call So how do we make it possible to reinstall from the previous local version? Make
We don't want this. Instead, we want the A more general overview of the flow:
|
Now the artifacts get re-staged, and upon an uninstall/finalize_upgrade they are deleted by purge_versioned_files instead
|
I see now! Thanks for the explanation-- |
|
I've fixed all the remaining issues:
|
|
Apologies in advance if I am misunderstanding this. Currently this uninstalls Edit: Also moves |
|
@commitay, yes, you're right. I did it that way because I wanted to just |
|
@commitay, now the flow looks like this: amalia@Sakura:~$ brew cask upgrade
==> Upgrading 2 outdated packages, with result:
libreoffice 5.4.3, libreoffice-language-pack 5.4.3
==> Satisfying dependencies
==> Downloading https://download.documentfoundation.org/libreoffice/stable/5.4.3
######################################################################## 100,0%
==> Verifying checksum for Cask libreoffice
==> Starting upgrade for Cask libreoffice
==> Moving App 'LibreOffice.app' to '/usr/local/Caskroom/libreoffice/5.4.2/Libre
==> Unlinking Binary '/usr/local/bin/regmerge'.
==> Unlinking Binary '/usr/local/bin/regview'.
==> Unlinking Binary '/usr/local/bin/senddoc'.
==> Unlinking Binary '/usr/local/bin/ui-previewer'.
==> Unlinking Binary '/usr/local/bin/uno'.
==> Unlinking Binary '/usr/local/bin/urelibs'.
==> Unlinking Binary '/usr/local/bin/uri-encode'.
==> Unlinking Binary '/usr/local/bin/xpdfimport'.
==> Unlinking Binary '/usr/local/bin/soffice'.
==> Unlinking Binary '/usr/local/bin/unopkg'.
==> Unlinking Binary '/usr/local/bin/gengal'.
==> Moving App 'LibreOffice.app' to '/Applications/LibreOffice.app'.
==> Linking Binary 'regmerge' to '/usr/local/bin/regmerge'.
==> Linking Binary 'regview' to '/usr/local/bin/regview'.
==> Linking Binary 'senddoc' to '/usr/local/bin/senddoc'.
==> Linking Binary 'ui-previewer' to '/usr/local/bin/ui-previewer'.
==> Linking Binary 'uno' to '/usr/local/bin/uno'.
==> Linking Binary 'urelibs' to '/usr/local/bin/urelibs'.
==> Linking Binary 'uri-encode' to '/usr/local/bin/uri-encode'.
==> Linking Binary 'xpdfimport' to '/usr/local/bin/xpdfimport'.
==> Linking Binary 'soffice.wrapper.sh' to '/usr/local/bin/soffice'.
==> Linking Binary 'unopkg' to '/usr/local/bin/unopkg'.
==> Linking Binary 'gengal' to '/usr/local/bin/gengal'.
==> Purging files for version 5.4.2 of Cask libreoffice
🍺 libreoffice was successfully upgraded!
==> Satisfying dependencies
All Cask dependencies satisfied.
==> Downloading http://download.documentfoundation.org/libreoffice/stable/5.4.3/
######################################################################## 100,0%
==> Verifying checksum for Cask libreoffice-language-pack
==> Starting upgrade for Cask libreoffice-language-pack
==> Purging files for version 5.4.2 of Cask libreoffice-language-pack
🍺 libreoffice-language-pack was successfully upgraded! |
|
Is there anything I can do wrt the |
|
I've open PRs with fixes for their dependent Casks, thanks anyway for the offer! |
|
I added two more tests to check that failed upgrades are properly reverted (if necessary). |
| end | ||
|
|
||
| unless source.exist? | ||
| return if skip |
There was a problem hiding this comment.
I think instead of this, the move(target, source, **options) direction should be completely separate, e.g. move_back or something.
Also, the Utils.path_occupied?(target) part above is not needed for the move_back case, because this will (by which I mean should) never happen.
There was a problem hiding this comment.
You're right here. However, we will still need this var because uninstall tests expect delete's behaviour (skip if artifact not found).
| odebug "Started upgrade process for Cask #{old_cask}" | ||
| raise CaskNotInstalledError, old_cask unless old_cask.installed? || force? | ||
|
|
||
| unless old_cask.installed_caskfile.nil? |
There was a problem hiding this comment.
We should error here if there is no old cask file.
| option "--greedy", :greedy, false | ||
| option "--quiet", :quiet, false | ||
| option "--force", :force, false | ||
| option "--force-update", :force_update, false |
There was a problem hiding this comment.
This flag isn't used anywhere.
| # If successful, wipe the old Cask from staging | ||
| old_cask_installer.finalize_upgrade | ||
| rescue CaskError => e | ||
| opoo e.message |
There was a problem hiding this comment.
Instead outputting the error message here, re-raise the exception at the end. Otherwise it will exit with 0 when it fails.
| @@ -83,7 +84,7 @@ def install | |||
| odebug "Hbc::Installer#install" | |||
|
|
|||
| if @cask.installed? && !force? && !@reinstall | |||
| raise CaskAlreadyInstalledError, @cask | |||
| raise CaskAlreadyInstalledError, @cask unless upgrade? | |||
There was a problem hiding this comment.
Add !upgrade? to the if above instead, or move force? || @reinstall down here.
| purge_versioned_files | ||
| purge_caskroom_path if force? | ||
| end | ||
|
|
||
| def uninstall_artifacts | ||
| def start_upgrade | ||
| return unless upgrade? |
There was a problem hiding this comment.
Remove this, this shouldn't be called anyway if this isn't an upgrade. Same goes for the methods below.
| @@ -420,10 +443,10 @@ def purge_versioned_files | |||
| end | |||
| end | |||
| @cask.metadata_versioned_path.rmdir_if_possible | |||
| @cask.metadata_master_container_path.rmdir_if_possible | |||
| @cask.metadata_master_container_path.rmdir_if_possible unless upgrade? | |||
There was a problem hiding this comment.
Is this check here necessary, since this is not rm_rf but only rmdir?
There was a problem hiding this comment.
Yes. Line 445 clears the Cask's versioned metadata (which is what we need for finalize_upgrade), but the lines below delete the whole metadata folder (which we need for uninstall), and then purges the whole Cask's folder (which will break everything).
(edited for clarity and completeness)
- Split move into a move_back (and clarify when it is used) - Remove unused flags - Raise error if installed Caskfile not found - Error out if an upgrade fails - Remove some defensive programming checks
|
Thank you for your great work here, @amyspark! 👍 🎉 |
|
great to have this natively supported now however what about upgrading auto-updating casks? --greedy calls all "latest" casks which is pretty verbose. I like a package manager because it provides version information and can act based on that but now the version information of the auto-updatable casks is rendered useless. (I currently use thanks for your efforts! |
I agree - I'd like to have the ability to upgrade greedily...but without "latest". |
We can’t have it every way. Apps will auto-update independently of what we do, and we’re not going to cripple that (nor could we even if we wanted). So versions are going to be auto of sync anyway for casks that auto-update, no way around that.
Maybe from an
And what about the casks that are |
|
Hello, how can i test that feature (brew cask upgrade)? |
|
Yes - those are all points that have been used in the past to argue against having an |
Just use brew upgrade for everything that has Version Info so as soon as I run my brew command everything is ready to go. (Or at least as ready as it can be) |
@vitorgalvao Out of interest, is there any metadata that indicates if autoupdate is something a Cask does. If so, perhaps that could be used here? |
|
@MikeMcQuaid Yes. It’s a simple stanza we add to casks. I guess that could work: if I’m now running a script to determine how many casks we have in that situation. I’ll wager they’re not many. For reference, we have (across all official repos):
|
|
I would actually be in favour of also upgrading Casks with |
|
@reitermarkus I think it's a reasonable assumption that people are opening the Casks they've installed, though (at least for a default). |
|
@mike But you do not open them on a regular basis, yet if you open them you still want them up to date, the main reason to use a package manager |
The verdict is in. From 5272 casks, only 48 are latest auto_updates casks
I don’t think it’s worth losing sleep over this, then. Especially since ideally the number of latest casks will continue to decrease, and if they have auto_updates there’s a better chance we can get find appcasts for them (meaning they’ll stop being latest).
|
|
@vitorgalvao Seems reasonable to me, thanks for checking ❤️ |
|
@koutsenko this feature will presumably be in the next stable release of homebrew, that's why we don't have it yet by default. I tested it by creating a new branch within |
|
I just pulled master to try this out and ran into a small issue. On my system I have the
Could this be a side-effect of the upgrade process for |
|
@ev0rtex, this seems to be specific to |
Brew Cask has had a proper upgrade command since Homebrew/brew#3396
brew testswith your changes locally?This pull request fixes Homebrew/homebrew-cask#29301 by implementing the
brew cask upgradecommand.This is an initial version; I've added tests that cover a correct upgrade flow, but I would be especially interested if anyone could check that step 7 (if install fails, revert) works correctly.
Please review, and thanks for the comments!