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

brew cask add upgrade/outdated/pin/unpin #1523

Closed
wants to merge 3 commits into from

Conversation

palxex
Copy link

@palxex palxex commented Nov 17, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

The long waited feature for brew cask to became a REAL package manager - at least for myself 😄
Currently using a naive method for version comparing - since I'm not familiar with existing homebrew development, I don't know whether homebrew already implement a more robust one. If so please mention me how to use it 😄

@MikeMcQuaid
Copy link
Member

Not a Homebrew Cask maintainer: I don't think upgrade or most things using versions make sense. Literally every application I have installed with Homebrew Cask does in-place (auto)updates. From that perspective they may well show as "outdated" only to actually install an older version (if there's been two updates and Homebrew Cask only has one).

@jawshooah jawshooah added the cask Homebrew Cask label Nov 17, 2016
@jawshooah
Copy link
Contributor

Don't have time to review this now, pinging @Homebrew/cask.

@palxex
Copy link
Author

palxex commented Nov 17, 2016

@MikeMcQuaid The problem is those doesn't have auto-upgrade, like wireshark/calibre/etc. I don't think add upgrade will broke them - and currently we can just install and then pin them. In future, maybe a small modify of the cask DSL is suitable for marking the auto-upgrade ones, and limit the brew cask upgrade only apply to those don't get the mark.

@MikeMcQuaid
Copy link
Member

Personally I think such a DSL and widespread use of it is important before these commands should be included.

@jawshooah
Copy link
Contributor

In future, maybe a small modify of the cask DSL is suitable for marking the auto-upgrade ones

Personally I think such a DSL and widespread use of it is important before these commands should be included

This already exists: auto_updates. Here are some examples.

@MikeMcQuaid
Copy link
Member

This already exists: auto_updates. Here are some examples.

@jawshooah Nice. No objections from me, then, if these commands properly handle this DSL 👍

I was going to whine about how most of my casks have this DSL flag missing but I'll submit a PR to add them instead 😉. Be the change(set) you want to see in the world!

@palxex
Copy link
Author

palxex commented Nov 17, 2016

@jawshooah Thanks very much. I'll include its process in next commit.
@MikeMcQuaid Agree, I'm not very familiar with cask's previous option.

Not sure why the CI failed, the failed brew style and brew tests --official-cmd-taps stage are all OK at my machine.

@MikeMcQuaid
Copy link
Member

@reitermarkus Could you look into the test failure? Pretty sure it's a regression from the test refactoring.

@vitorgalvao
Copy link
Member

I was going to whine about how most of my casks have this DSL flag missing but I'll submit a PR to add them instead 😉. Be the change(set) you want to see in the world!

Thank you! When an app can auto-update, though, it also means it has to have an appcast somewhere, and we try to have the least amount of casks possible with the former but not the latter. Our documentation on appcast covers at least the easiest (and most common) cases.


As for the PR itself, something worries me a bit. I think HBC’s upgrade has potential to be more complex than HB’s, and that’s why we were supposed to write the exact desired procedure before starting to work on it (big fault on my part by not having had the availability to do it yet).

Apart from it absolutely needing to respect auto_updates, I’m think about the following scenario (that does not apply to HB due to only accepting open-source apps):

You have a commercial app at version 3.2. The developer releases a paid upgrade: 4.0, the cask is updated, and you upgrade. However, you did not want to pay for version 4.0. Now you’re stuck with a version you do not want. Not only is that a bad experience for you as a user, now you’ll likely introduce that last version into caskroom/versions, further filling the repo.

pin does not solve this, as you’re not going to continuously review updates to casks and unpin/pin. However, we do have the .metadata dir, and perhaps we can leverage that. Of the top of my head, something like brew cask revert cask-name would brew cask uninstall cask-name (this is important, as the new version might have introduced new things to the system we need to remove), followed by installing the old cask we had before and pinning it. Then, if the user ever decides to pay for the upgrade, they can unpin and upgrade.

@palxex
Copy link
Author

palxex commented Nov 18, 2016

Made 2 more commits, consider auto-updates ( by default will no more upgrade them, but you can ignore it too ) , and can upgrade specified packages now. Add version info to pinned marker for future.
For the scenario @vitorgalvao described, seems versioning is the reasonable solution. Even with revert, Its hard to imagine that one cask file includes 5 version's uninstallation detail, for example.
To COMPLETELY solve the problem, it seems either the Cask DSL need be tweak again, to describe every corner case of app licensing model ( need_renew_since_version 3.0 ? ), or make a version specify DSL in pining ( brew cask pin package "<4.0" ? ) . Either way may take several months more to discuss. I think maybe its better to hold the discussion after the PR, if it could solve the 80% problem with some manual work ( pinning ) , AND wont block either way from implementing.

@vitorgalvao
Copy link
Member

vitorgalvao commented Nov 18, 2016

For the scenario @vitorgalvao described, seems versioning is the reasonable solution. Even with revert, Its hard to imagine that one cask file includes 5 version's uninstallation detail, for example.

I don’t understand your point at all. Why would we need multiple uninstallation instructions? We don’t. revert would only get you back one installation, to the one you had right before running upgrade. We already have a .metadata directory that saves your cask at the moment you installed it, and its purpose is exactly so things like uninstalling can act on the correct version you had when installing. That information is there for a reason, so lets use it when appropriate, like for this case.

To COMPLETELY solve the problem, it seems either the Cask DSL need be tweak again, to describe every corner case of app licensing model ( need_renew_since_version 3.0 ? ), or make a version specify DSL in pining ( brew cask pin package "<4.0" ? )

That would make the DSL needlessly complicated for everyone with no gain. It would annoy both users and maintainers. The point here is that users can upgrade or not when they want, not needlessly pin them without their input.

Either way may take several months more to discuss.

Yes, exactly, so things can be done the right way. There’s a reason this hasn’t been implemented sooner: it’s an important feature nobody wants to make a mess of.

I think maybe its better to hold the discussion after the PR, if it could solve the 80% problem with some manual work ( pinning ) , AND wont block either way from implementing.

That is absolutely the wrong decision (and I feel strongly on this). With all due respect, you’re not a maintainer of either project, and if your PR were merged you wouldn’t feel in your skin the breakage it caused for users, having to close a barrage of issues and deal with every user, while at the same time trying to figure out how to fix things and having to drop everything to contain the situation.

You’re suggesting every users gets bitten by this, complains, and then we tell them “oh, just pin”. Not only will they be annoyed, we’d be back at my descried issue, where now they have no easy way to go back to the previous install. So no, this is not solved by the manual work of pinning.

Admittedly, this is likely not going to suddenly break and be a mess for all users at the same time but if it is an issue we detect now, it is one we should fix now. Why? Because if it isn’t done now, there’s a good change it’ll be half-assed for a long time and that left us in big trouble in the past. We’d really rather not have half-done stuff laying around, just piling up issues and continually telling users about workarounds, getting verbal abuse from some of them in the interim.

@palxex
Copy link
Author

palxex commented Nov 18, 2016

So, OK, thats' the solution of lacking such a core feature several years after HBC releasing, and seems all upgrade-relative issues have been marked close in the HBC repo:) Feel free to close the PR or fork it, I've no time on continue maintaining it. Wish official HBC upgrade will be available before 2018.

@vitorgalvao
Copy link
Member

Feel free to close the PR or fork it, I've no time on continue maintaining it.

I (and everyone on this issue, I’d say) do very much appreciate you taking the time to work on this. However, you just made my point that this feature would be left half-baked as you would no longer be willing to follow through with it.

Wish official HBC upgrade will be available before 2018.

That kind of snark is unwarranted. Especially since no one was disrespectful to you in any way. And again, you just said you wouldn’t work anymore on this so you’re marking yourself as part of the problem. The irony is palpable.

This reminds of the puppy analogy. I’m sorry you feel that way, but to me that even signals you might not even touch this anymore if this PR caused other breaking changes, and that’s not a really nice way to contribute.

It’s particularly jarring to me since you just dismissed the whole contribution without giving anyone else the chance to chime in and possibly come up with a better solution that would require less work.

Either way, I again thank you (with all sincerity) for contributing and working on this.

@toonetown
Copy link
Contributor

I, personally, would like to continue this discussion and work. Where would be the best place to have the discussion about upgrade/outdated/pin/unpin?

One thing we could consider is adding a new DSL entry for "paid_upgrade" or something...maybe it takes an array of versions that require paid upgrades. When you run brew cask outdated and one of the casks has been upgraded and the new version is one of the paid upgrades, it lets the user know that...and then they can choose whether or not to upgrade it then. I think that this prevents (or at least lessens) the case where a user is upgraded without knowing that it is a paid upgrade.

In that case, brew cask upgrade (with no cask names) would only upgrade those that are "free" upgrades. While brew cask upgrade some-commercial-package would upgrade a non-free package (you have to explicitly specify it). Maybe even brew cask upgrade --paid some-commercial-package to make it even more explicit what the user is doing.

If you make it harder to "auto-update" to paid upgrades, I don't think that a revert mechanism would be needed. Nor do I think that we would end up polluting the repo with additional versions, etc. (though version pollution is still a thing).

Another idea which I just had is perhaps the "paid_upgrade" DSL contains some sort of value indicating at which commit that formula was updated. As an example, let's take "some-commercial-package" which currently has at version 4.2 (and there is a cask for it) and they just released a paid upgrade to 5.0. The value of paid_upgrade could look something like this:

paid_upgrade: { 5.0: "sha-sum-of-commit-of-5", 4.0: "sha-sum-of-commit-of-4" }

That's just an initial shot at a syntax - and we would probably need to leverage some kind of git commit hook to insert the sha sum of the commit in the correct place... However, this would give "us" (developers of brew cask utilities) as well as the user enough information to know when running the following commands:

> brew cask outdated
calibre
> brew cask outdated --show-auto
calibre
sourcetree (auto)
> brew cask outdated --show-paid
calibre
some-commercial-package (paid - 5.0)
> brew cask outdated --show-all
calibre
sourcetree (auto)
some-commercial-package (paid - 5.0)

That is, calibre (which is ALWAYS getting updated) is a "cask-managed" upgrade. sourcetree is flagged as auto_updates: true. And some-commercial-package has a version 5.0 upgrade available, but it's a paid upgrade.

The user could then run:

> brew cask upgrade
=== Upgrading calibre ===
...
Done!
> brew cask upgrade --auto
=== Upgrading sourcetree ===
...
Done!
> brew cask upgrade --paid some-commercial-package
=== Upgrading some-commercial-package ===
...
Done!

I would avoid the whole "pin/unpin" thing...it doesn't really make much sense to me from a cask perspective - but if it would be helpful, it could be included. Basically, it would just take the cask out of the list that is shown when running brew cask outdated.

And, if we really want to get fancy, we could provide a command for some-commercial-package to install the last version of 4.0 (let's say I have a 4.0 license, and am installing a new computer and want to install 4.0). All it would take is to check out the formula as of the commit prior to paid_upgrade { 5: "sha-sum-of-commit-of-5" }. The formula might not work, but it likely would...and then we wouldn't have to maintain all the different versions in the repo.

That last one is really just a "throw it out there" kind of thought. It probably would be way too fragile. In fact, I've not got my heart set on any of these proposals...I just thought I'd put them out there. To be honest, I've been just fine with my (dumb) scripts brewcask-outdated.rb and brewcask-upgrade.rb...and I've been using a brewcask-appcast.rb script for managing the automatically-updating ones as well.

Well - that was a long post to basically say "I'm willing" to help out getting an official "outdated/upgrade" command set for HBC...and I'd love to participate in the conversation that needs to happen in order to get it implemented.

@vitorgalvao
Copy link
Member

One thing we could consider is adding a new DSL entry for "paid_upgrade" or something...maybe it takes an array of versions that require paid upgrades.

That won’t work. A paid upgrade does not exist in a vacuum, it exists in relation to other versions so an array means nothing.

If you make it harder to "auto-update" to paid upgrades, I don't think that a revert mechanism would be needed.

That’s an interesting notion, and one that would indeed (as you mention) remove the need for revert/pin/unpin. If it can be well done, I could get behind that.

However, there’s a corner case that is yet to be addressed. Say we have a cask at 4.1 and a user installs it. We then update the cask to 4.2 but the user does not upgrade. Then 5.0 is released as a paid upgrade and the use still does not upgrade. Finally, 5.1 is released and they want to upgrade now. There comes the issue with referencing specific versions — it only makes sense to mention the cutoff for paid upgrades at the latest version of the previous one (4.2) or at the earliest of the current one (5.0). In this case, the user just bypassed that.

revert/pin/unpin would still take care of those, though.

And, if we really want to get fancy, we could provide a command for some-commercial-package to install the last version of 4.0 (let's say I have a 4.0 license, and am installing a new computer and want to install 4.0). All it would take is to check out the formula as of the commit prior to paid_upgrade { 5: "sha-sum-of-commit-of-5" }. The formula might not work, but it likely would...and then we wouldn't have to maintain all the different versions in the repo.

There was recently a discussion on why that is a bad idea. Especially this part:

The formula might not work, but it likely would

That idea to me reads as “endless support requests”. It’s hard enough to keep up with casks as they are, let alone introducing complexity we do not control.

@toonetown
Copy link
Contributor

toonetown commented Dec 29, 2016

All of your points make perfect sense. As I'm thinking about it more, it seems like if we were to (somehow) make automatic updates to paid versions more difficult and get rid of the pin/unpin/revert constructs, then we could probably get away with saying "we support the latest version - paid or not - and if you want to keep running an older version, it is up to you to either a) maintain your own tap, b) submit the old versions to homebrew-versions, or c) try installing directly from an old version as discussed in the linked issue".

It seems to me like anything more than supporting the "latest" version is beyond the scope of what homebrew and homebrew-cask are intended to do - as you mention, it is just "endless support requests". To say nothing of the fact that many of these companies that put out the software don't even support previous versions of their own software (you want support, you upgrade to the latest version).

I guess what I'm saying is I was completely overthinking this - and perhaps if we were to provide a brew cask upgrade command (which would be useful in most cases) that is at least flexible enough to not break paid (and, I would argue, auto) upgrades then maybe we define all the corner cases as "outside of the scope" of this.

I'll keep thinking on it...but maybe it really is just a matter of adding a paid_updates boolean value to the DSL (similar to auto_updates) - or changing auto_updates to be something like update_type with possible values of auto, paid, none, etc... (you could even treat auto and sparkle as different types...whatever makes sense).

Then, brew cask upgrade would only operate on certain types by default - requiring an additional command line argument to "force" the upgrade for paid and/or automatically upgrading software.

Doing that, we wouldn't have to even worry about the "user is running 4.1, never upgraded to 4.2 or 5.0, and now we are at 5.1" case. If software offers "paid" upgrades, it is up to the user to determine whether or not to actually apply that update. There is never really a way that we'll ever be able to understand or map how the paid upgrade works...there are too many different variables. For example, a company can automatically allow your 4.2 license to work on 5.0 if you purchased it within 3 months of the 5.0 release date...we'll never be able to map all those possible scenarios.

I would suggest keeping it simple...either software automatically updates or not (we already track that). Either they provide paid updates or not (we don't track that). brew cask outdated should show all casks that are currently installed that have an upgrade available...whether paid, auto, or not...and brew cask upgrade should be "hard" to apply to software that is paid (and probably software that auto updates).

That does, however, bring up another point...perhaps there should be a brew cask upgrade --reset or something (I have NO idea of what the flag should be...or perhaps it is even a different brew cask command entirely). For automatically-updating software, there should be a way to basically say "I have manually updated this within the app itself - update the .metadata information".

And along those same lines, there perhaps should be a brew cask upgrade --ignore <cask> command (or brew cask upgrade-ignore or whatever...I really don't know what would be the best way to name it) that says "I will always update this software automatically within the app - don't even show it to me again in brew cask outdated".

It seems like there should be some kind of documentation around the requirements of an outdated/upgrade/pin/unpin/revert/ignore feature. Is there already such documentation? If not, would it make more sense to continue the discussion here - or perhaps open a new issue with a proposal as a new issue? (I've come across and read through Homebrew/homebrew-cask#309, Homebrew/homebrew-cask#4678 and related - but I haven't found an actual "proposal") I'm willing to put together some kind of proposal and talk it out before jumping in on doing some sort of work on it.

I do think that this would be a helpful feature (helpful enough that I have been maintaining my own brewcask-outdated and brewcask-upgrade scripts that work for my own purposes)...but I also am aware of all the possible support issues that come up with such a feature (which is why my scripts are only good enough for my own purposes). I would just like to be able to help out on this feature as much as possible - but if it is decided that it has already been discussed and should not be done, then I am fine with just dropping it and leaving the current status of the feature as-is. I don't want to pull others away from more pressing and helpful tasks.

@vitorgalvao
Copy link
Member

It seems to me like anything more than supporting the "latest" version is beyond the scope of what homebrew and homebrew-cask are intended to do

That is indeed the case.

if we were to (somehow) make automatic updates to paid versions more difficult and get rid of the pin/unpin/revert constructs, then we could probably get away with saying "we support the latest version - paid or not - and if you want to keep running an older version, it is up to you to either a) maintain your own tap, b) submit the old versions to homebrew-versions, or c) try installing directly from an old version as discussed in the linked issue".

But this is an awful experience. Maybe you don’t want to pay to upgrade now. Making you remove the main cask (so it does not upgrade) and install (and perhaps specifically add) the old version in caskroom/versions just so you have it until you decide to pay for the upgrade (and go back to the main cask) is disrupting your workflow. It’s breaking what you have and then saying “now go fix it manually”.

We’re not going to do that. It makes much more sense to either say “we’re not going to upgrade you until you give us a flag saying you really want to” or “if we just did an update you weren’t ready for, give this revert flag and get back to what you had”. The key is to allow to delay the decision or go back on it, easily.

maybe it really is just a matter of adding a paid_updates boolean value to the DSL

Again, this can’t be done because a paid upgrade is not done in a vacuum. Not every upgrade is paid, and we don’t want to block on every minor one.

there are too many different variables. For example, a company can automatically allow your 4.2 license to work on 5.0 if you purchased it within 3 months of the 5.0 release date...we'll never be able to map all those possible scenarios.

True, but that is clearly an exception. I those cases, the user will just “yes, force the upgrade” or “no, do not revert the upgrade”.

We can continue to discuss here. After we get some kind of consensus, I’ll write a proposal (à lá Homebrew/homebrew-cask#13201).

@toonetown
Copy link
Contributor

We’re not going to do that. It makes much more sense to either say “we’re not going to upgrade you until you give us a flag saying you really want to” or “if we just did an update you weren’t ready for, give this revert flag and get back to what you had”. The key is to allow to delay the decision or go back on it, easily.

Yes - that is what I meant. I left off a "d) don't upgrade and just keep running the version you already are running" in my list. 😄

Again, this can’t be done because a paid upgrade is not done in a vacuum. Not every upgrade is paid, and we don’t want to block on every minor one.

I would argue that we would want to block on every minor one. The way I would envision it working is for "paid" software, we don't auto-upgrade anyone ever - not even to minor updates. The brew cask outdated command should list what that "latest" version is, but brew cask upgrade should not update paid software. We just offer the latest version of the software - it is up to the user to find out if their current license allows them to upgrade to that latest version (brew cask home would be helpful for them in determining it). I think we should provide the information to the user, and give them tools to do "common" tasks (i.e. list available updates, provide a mechanism for applying those updates, etc.), but we shouldn't try to be too "smart" about it.

I think that the interaction should be something like this (being super verbose here...maybe there is a better way to guide the user):

> brew cask upgrade some-paid-application
==> Upgrading some-paid-application
Warning: some-paid-application provides paid upgrades.
    The latest version is 5.1 (you have 4.2 installed).  Please check with the developer to
    determine if your license can be upgraded ('brew cask home').  You can then do one
    of the following:

        1) Update to the latest version: 'brew cask upgrade --force some-paid-application'
        2) Ignore this update: 'brew cask upgrade --ignore some-paid-application'
        3) Ignore all future updates: 'brew cask upgrade --ignore-all some-paid-application' 
        4) Replace your cask with one in caskroom/versions: 'brew cask replace some-paid-application4'
        ... etc ...

Perhaps something like 4) above could be determined dynamically - if a version exists in caskroom/versions for the major version that they have installed, we offer to "tweak" their .metadata directory so that the "versioned" cask is the one that is installed, and then we continue upgrades to that cask instead. (I'm really just throwing ideas out to see if anything "sticks"...)

@toonetown
Copy link
Contributor

You know what would be nice is a brew cask changelog command (or even a brew changelog command) that works like brew cask home but opens directly to the changelog (if a site provides it).

Understandably, it would require more maintenance...but it still would be pretty cool 😄

@DomT4
Copy link
Member

DomT4 commented Jan 15, 2017

I'll keep old internal politics out of my addition here, but I am utterly baffled why it was decided it was a good idea to merge #1808 before this PR really goes anywhere in terms of deciding whether it has a future or not.

I've discussed some of this in the past at ridiculous length so it's not like this has never been flagged up, and I'm not going to paste an essay here because I don't think there's much to gain by doing so, but I think when you have Casks like chromium which don't autoupdate and pile up security bugs in the way that browsers are oh so prone to doing, and then you work towards removing the linkapps function so people can no longer abuse formula to provide that automatic update function, you're essentially playing fast and loose with user security.

Obviously this needs to be handled delicately but I think merging #1808 before this is resolved was reckless, and beyond that not entirely factually true given formulae can be used to create relocatable app bundles, either through simply installing .apps (which I appreciate is a hack) or by building a relocatable .app by default as wireshark's qt5 option does (which is very much not a hack).

@MikeMcQuaid
Copy link
Member

I'll keep old internal politics out of my addition here, but I am utterly baffled why it was decided it was a good idea to merge #1808 before this PR really goes anywhere in terms of deciding whether it has a future or not.

A deprecation doesn't change anything except printing a message. We've been talking about brew linkapps not working well for years and it literally cannot work well.

I think when you have Casks like chromium which don't autoupdate and pile up security bugs in the way that browsers are oh so prone to doing, and then you work towards removing the linkapps function so people can no longer abuse formula to provide that automatic update function, you're essentially playing fast and loose with user security.

If you want to propose removing the Chromium cask for security reasons, that's another discussion.

I'd argue that it's effectively the expectation on macOS for .apps to be self-contained, not require external frameworks to be installed, not require Java or X11 and auto-update through Sparkle, the Mac App Store or an equivalent. If they do not do so, I consider that bad upstream packaging and that's not something that Homebrew is responsible for fixing any more than formulae that don't build with clang.

@MikeMcQuaid
Copy link
Member

All that said: I do think there's value for someone to pick up and progress with this work although it may be worth someone like @vitorgalvao sketching out maybe in a help wanted issue what that would need to look like for this to be accepted.

@vitorgalvao
Copy link
Member

although it may be worth someone like @vitorgalvao sketching out maybe in a help wanted issue what that would need to look like for this to be accepted.

I do really have to get on that, yes.

I’ve been thinking more about it lately, and maybe this can work even without pin/unpin, which weren’t even a consideration before this PR.

Reason being we have auto_updates (not yet taken into account in this PR) and I’m thinking: “are there any commercial apps that do not auto-update”? I’m betting those might very well be the minority if they exist (after all, if you ask for more money for upgrades, you want to auto-update so your users get notified).

That being the case, removing pin/unpin and adding support for auto_updates might indeed make this good enough to be accepted because we’d need more data to make a decision on if a revert (or whatever) is indeed needed and we can’t get that data without real-world usage.

@MikeMcQuaid
Copy link
Member

I’ve been thinking more about it lately, and maybe this can work even without pin/unpin, which weren’t even a consideration before this PR.

👍 to reducing scope

That being the case, removing pin/unpin and adding support for auto_updates might indeed make this good enough to be accepted because we’d need more data to make a decision on if a revert (or whatever) is indeed needed and we can’t get that data without real-world usage.

👍

@vitorgalvao
Copy link
Member

First draft of the outline, with the latest points of the discussion in mind: Homebrew/homebrew-cask#29301.

@MikeMcQuaid
Copy link
Member

Closing this in favour of that issue or a future PR. Thanks for the work anyway @palxex!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cask Homebrew Cask
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants