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

cmd/update-report: consistency across formulae and casks #8395

Merged
merged 1 commit into from Aug 20, 2020

Conversation

miccal
Copy link
Member

@miccal miccal commented Aug 19, 2020

  • 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 style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Currently, running brew update will list any new, updated or deleted formulae and any updated or deleted casks, but not new casks:

==> New Formulae
a shiny new formula
==> Updated Formulae
an updated formula
==> Deleted Formulae
a deleted formula
==> Updated Casks
an updated cask
==> Deleted Casks
a deleted cask

In the spirit of consistency, this change prints a similar message for new casks, so now:

==> New Formulae
a shiny new formula
==> Updated Formulae
an updated formula
==> Deleted Formulae
a deleted formula
==> New Casks
a shiny new cask
==> Updated Casks
an updated cask
==> Deleted Casks
a deleted cask

I have tested this change locally with fictitious new, updated and deleted formulae and casks in my personal tap, and everything works as I imagined -- hopefully I have not missed anything.

Thank you.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the spirit of consistency it might also be nice to only list "Updated Casks" that have had a version change.

@miccal
Copy link
Member Author

miccal commented Aug 19, 2020

In the spirit of consistency it might also be nice to only list "Updated Casks" that have had a version change.

Please help me understand how this works with "Updated Formulae" -- given a formula, let's use zsh as an example, this will be listed as "updated" if and only if the first url stanza, namely:

url "https://downloads.sourceforge.net/project/zsh/zsh/5.8/zsh-5.8.tar.xz"

is changed?

So if this single stanza is unchanged, then no matter what else is changed in the formula's .rb file (including the url stanza's for the head block and/or the resource block), it will not be listed as "updated"?

Is this correct?

@MikeMcQuaid
Copy link
Member

if and only if the first url stanza, namely:

url "https://downloads.sourceforge.net/project/zsh/zsh/5.8/zsh-5.8.tar.xz"

is changed?

If url or revision are changed: yes.

(including the url stanza's for the head block and/or the resource block), it will not be listed as "updated"?

Yes.

@miccal
Copy link
Member Author

miccal commented Aug 19, 2020

If url or revision are changed: yes.

@MikeMcQuaid thank you for the clarification. This makes sense -- in a formula, a change to either the url or revision stanza in a formula's .rb file basically implies that a new bottle has been created, and hence the formula needs an update.

However, the situations where a change in a cask's .rb file may or may not require an actual update via brew is not so straight-forward. Let me illustrate this with a few examples off the top of my head:

  • For a cask that has had a version change, then an update is required, and this will be delivered via brew upgrade provided the cask does not have auto_updates true (or if the --greedy option is used for a cask that does have auto_updates true).

  • For a cask that has not had a version change but has had a sha256 change, this may indicate that an update is required, for example if the .app was changed in-place to fix a bug but without a version increment; or it may indicate that an update is not required, for example the url was simply changed to point to the same .app housed inside a .dmg instead of in a .zip.

  • For a cask that has not had a version or sha256 change, this may indicate that an update may be required because the install method was changed to add a binary stanza; or it may indicate that an update is not required because some other stanza was trivially changed.

This is all the more complicated by those casks that have auto_updates true, where an update via brew is the choice of the user (using the --greedy option, for example); and for those casks that have version :no_check but perhaps had the url updated (so no update is required) or the install method was changed to add a binary (in which case an update will be required to use this new-added functionality).

The point I am trying to make with these examples is this -- for formulae, monitoring changes to the url or revision stanzas in the formula's .rb file is all that is really required to indicate if an update is required.

However, for casks the sheer number of cases that do require/may require/may not require an update makes it much more complicated to know what specific change in the cask's .rb file to monitor.

In summary, I think the current status quo is the way to go; that is, showing all casks that have been altered in any way via brew update.

I also personally prefer it this way, as I can then review exactly what has been changed in the cask's .rb file and decide if I should update it via brew. Of course, other user's may approach cask updates differently, complicating things even further!

@MikeMcQuaid
Copy link
Member

  • For a cask that has not had a version change but has had a sha256 change, this may indicate that an update is required, for example if the .app was changed in-place to fix a bug but without a version increment; or it may indicate that an update is not required, for example the url was simply changed to point to the same .app housed inside a .dmg instead of in a .zip.
  • For a cask that has not had a version or sha256 change, this may indicate that an update may be required because the install method was changed to add a binary stanza; or it may indicate that an update is not required because some other stanza was trivially changed.

I think "an update may be required" should be covered by metadata e.g. a revision rather than the git diff of a file.

This is all the more complicated by those casks that have auto_updates true, where an update via brew is the choice of the user (using the --greedy option, for example); and for those casks that have version :no_check but perhaps had the url updated (so no update is required) or the install method was changed to add a binary (in which case an update will be required to use this new-added functionality).

In these cases: we're not going to show "Updated" when upstream has updated so it seems confusing to do that. Regardless, there could be a different behaviour for these cases if desired.

However, for casks the sheer number of cases that do require/may require/may not require an update makes it much more complicated to know what specific change in the cask's .rb file to monitor.

I don't agree that they are different. A formula can be changed and not revisioned but the actual output of installing that formula has changed (and it may have a different bottle).

In summary, I think the current status quo is the way to go; that is, showing all casks that have been altered in any way via brew update.

It is inconsistent with how formulae are being handled, though, and I still don't see a good reason for this inconsistency.

What is the "action item" from your perspective for seeing "Updated casks" here? Run brew cask outdated (and see nothing is outdated sometimes)? Do nothing? Why print this output at all?

@miccal
Copy link
Member Author

miccal commented Aug 19, 2020

I think "an update may be required" should be covered by metadata e.g. a revision rather than the git diff of a file.

I agree, in theory -- having a cask stanza similar to the revision one in formula would help, but I fear that maintaining it in terms of reviews and checking if a "revision-bump" is necessary in a cask PR would be problematic.

In these cases: we're not going to show "Updated" when upstream has updated so it seems confusing to do that. Regardless, there could be a different behaviour for these cases if desired.

Yep, complicated :)

I don't agree that they are different. A formula can be changed and not revisioned but the actual output of installing that formula has changed (and it may have a different bottle).

Sorry, I took away from your earlier reply that unless there was a change in either url or revision in a formula it would not be listed as an upgraded formula?

It is inconsistent with how formulae are being handled, though, and I still don't see a good reason for this inconsistency.

True, but in some ways casks are a bit more "tricky" than formula in the sense that there are a lot more stanzas to deal with that dictate both their install method and how they are "run". Again, complicated :)

What is the "action item" from your perspective for seeing "Updated casks" here? Run brew cask outdated (and see nothing is outdated sometimes)? Do nothing? Why print this output at all?

I personally use brew to handle all the updates of things I have installed by cask; that is, I run brew update && brew outdated --greedy && brew upgrade --dry-run and inspect the output -- that will tell me what to do next, be it a basic brew upgrade, a brew upgrade {an installed cask with auto_updates true} and/or brew cask cat {for an installed cask on the list of brew update but not on the list of brew outdated} or a peek at the PR on GitHub to see what was changed -- I am a little OCD, though :)

Please also note that I am not trying to be difficult or argumentative here @MikeMcQuaid.

@MikeMcQuaid
Copy link
Member

Sorry, I took away from your earlier reply that unless there was a change in either url or revision in a formula it would not be listed as an upgraded formula?

Correct, it would not. That does not imply that the output of the formula hasn't been "updated", though.

True, but in some ways casks are a bit more "tricky" than formula in the sense that there are a lot more stanzas to deal with that dictate both their install method and how they are "run". Again, complicated :)

Well, the complication in this case is: how many of these changing stanzas would result in an action from a user?

I personally use brew to handle all the updates of things I have installed by cask; that is, I run brew update && brew outdated --greedy && brew upgrade --dry-run and inspect the output -- that will tell me what to do next, be it a basic brew upgrade, a brew upgrade {an installed cask with auto_updates true} and/or brew cask cat {for an installed cask on the list of brew update but not on the list of brew outdated} or a peek at the PR on GitHub to see what was changed -- I am a little OCD, though :)

Yeh, I'd argue the typical user case for this output is "should I run brew upgrade" which is the case I'm proposing optimising better for (on the cask side).

Please also note that I am not trying to be difficult or argumentative here @MikeMcQuaid.

No, don't worry, I don't think you are being at all (me, on the other hand 😂).

@miccal
Copy link
Member Author

miccal commented Aug 19, 2020

Correct, it would not. That does not imply that the output of the formula hasn't been "updated", though.

Ah, I see, thank you.

Well, the complication in this case is: how many of these changing stanzas would result in an action from a user?

Good point -- I suppose for the vast majority of users they would only be interested in a version change, as is the case for formula.

Yeh, I'd argue the typical user case for this output is "should I run brew upgrade" which is the case I'm proposing optimising better for (on the cask side).

I see your point, thank you.

No, don't worry, I don't think you are being at all (me, on the other hand 😂).

You are not, so don't you worry either!

I will have to think about how best to implement this and show a cask as "updated" only if the version changes.

@MikeMcQuaid while I investigate this further would it be ok if you could merge this PR and my other one -- my other PR will require some updates to things in the cask repositories that I would like to deal with first.

Thank you.

@MikeMcQuaid
Copy link
Member

Good point -- I suppose for the vast majority of users they would only be interested in a version change, as is the case for formula.

Agreed 👍🏻

@MikeMcQuaid while I investigate this further would it be ok if you could merge this PR and my other one -- my other PR will require some updates to things in the cask repositories that I would like to deal with first.

Sure thing 👍🏻 Thanks for making it!

@MikeMcQuaid MikeMcQuaid merged commit 8041649 into Homebrew:master Aug 20, 2020
@miccal miccal deleted the cask_new branch August 20, 2020 12:11
@miccal
Copy link
Member Author

miccal commented Aug 20, 2020

Thanks @MikeMcQuaid.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 16, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 16, 2020
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

4 participants