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: upgrade to 0.60.1 #16699

Closed
wants to merge 1 commit into from
Closed

brew-cask: upgrade to 0.60.1 #16699

wants to merge 1 commit into from

Conversation

DomT4
Copy link
Member

@DomT4 DomT4 commented Jan 10, 2016

Over at Homebrew we've accidentally caused an edge-case problem for some Caskroom users. We recently merged Homebrew/legacy-homebrew@915ac06d which is much stricter on empty installations, failing during install rather than just shouting about it in the audit.

We'd prefer not to revert the change there because it's quite a positive change that's much more obvious about installation problems, given installations shouldn't be empty. There's also the likelihood that if we revert, wait 3 months to add the check back, someone out there will wait 4 months before upgrade and we'll run into this again.

However, this causes some issues with Cask because when Mike rewrote the formula he left def install an empty hole, which was fine at the time. Anyone who hasn't updated between December 9th 2015 and January 9th 2016 will now run into a fatal error on upgrading the Cask formula, as seen in: Homebrew/legacy-homebrew#47929

I was wondering if the Cask would be kind enough to tag one very last bug fix release, or even retag the existing tag with a similar fix to this. You may want to do this yourself rather than using this PR; this was an easy way to both explain the problem and propose a fix without sticking a diff blob in the middle of the text.

Obviously, if tested by Travis here this build will likely fail because there is no 0.60.1 tag. Apologies for the accidental mess here. It's one of those things we'll probably get better at if/when we can bring the two projects closer together and do more unified testing.

CC @MikeMcQuaid for thoughts.

@MikeMcQuaid
Copy link
Member

👍 from me.

@adidalal
Copy link
Contributor

Pinging @caskroom/maintainers

@adidalal adidalal added enhancement awaiting maintainer feedback Issue needs response from a maintainer. core Issue with Homebrew itself rather than with a specific cask. labels Jan 11, 2016

depends_on :ruby => "2.0"

def install
(buildpath/"UPGRADE").write <<-EOS.undent
You should uninstall this formula. Only `brew tap Caskroom/cask`
Copy link
Member

Choose a reason for hiding this comment

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

“should” → “must”? We want people to really do it, so I wouldn’t object to the more forceful language.

In addition, “Only brew tap Caskroom/cask is required now to stay up-to-date.” could be something like “It is no longer needed to stay up to date, as homebrew takes care of that automatically”. By saying “only brew tap Caskroom/cask is required now to stay up-to-date”, I wonder if some people will regularly try that command to update homebrew-cask. Making clear they must uninstall the formula and need do nothing more is likely a plus.

This because we still receive so many bug reports regarding this latest change (despite plastering information about it all over the place), having a tone of “you must uninstall this because it really isn’t needed, and here’s why” will likely be beneficial.

Copy link
Member

Choose a reason for hiding this comment

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

👍 to must.

Copy link
Member

Choose a reason for hiding this comment

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

But I'd feel free to pull as-is and edit locally ( @DomT4 definitely won't mind) if it lets you get it in quicker and save you work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. This was more of a suggested fix than a "PR" per se. You can use it if you want to and I'm happy to push changes as desired, but am entirely understanding of the fact it's probably easier for you to push the necessary tweaks on your end and close this PR afterwards and not going to object if you prefer that path.

@vitorgalvao
Copy link
Member

@jawshooah You commented (and I agree) about having a round number for the last release (0.60.0) and I concur it makes sense. Shall we make this 0.70.0?

@MikeMcQuaid
Copy link
Member

@vitorgalvao Up to you but given this is just a patch around a Homebrew bug in the interests of semver I'd leave it as a .1.

@jawshooah
Copy link
Contributor

I don't care much about the version tag; not like we've been sticking to semver anyway (plenty of breaking changes in the project's history, not a single major version bump). @vitorgalvao, any objection to me pulling this down, tagging and pushing?

@DomT4
Copy link
Member Author

DomT4 commented Jan 11, 2016

Let me know if you want me to make any changes.

@vitorgalvao
Copy link
Member

@vitorgalvao, any objection to me pulling this down, tagging and pushing?

No objection at all.

@adidalal
Copy link
Contributor

Other than the suggested wording changes, it LGTM

Over at Homebrew we've accidentally caused an edge-case problem for some
Caskroom users. We recently merged Homebrew/legacy-homebrew@915ac06d
which is much stricter on empty installations, failing during install rather than
just shouting about it in the audit.

We'd prefer not to revert the change there because it's quite a positive change
that's much more obvious about installation problems, given installations
shouldn't be empty. There's also the likelihood that if we revert, wait 3 months
to add the check back, someone out there will wait 4 months before upgrade and we'll
run into this again.

However, this causes some issues with Cask because when Mike rewrote the formula
he left `def install` an empty hole, which was fine at the time.
Anyone who hasn't updated between December 9th 2015 and January 9th 2016 will now
run into a fatal error on upgrading the Cask formula, as seen in: Homebrew/legacy-homebrew#47929

I was wondering if the Cask would be kind enough to tag one very last bug fix release,
or even retag the existing tag and formula with a similar fix to this.
If you're agreed and prefer to do this yourself you can ignore the PR but wanted
to explain the background.
@DomT4
Copy link
Member Author

DomT4 commented Jan 11, 2016

Made some wording tweaks.

@jawshooah
Copy link
Contributor

I'm going to keep the wording the same between install and caveats if it's all the same to you guys.

That is, I'll leave out the mention of brew tap Caskroom/cask, since it isn't really necessary as Homebrew does it for you.

@DomT4
Copy link
Member Author

DomT4 commented Jan 11, 2016

Feel free to make whatever changes you want to to anything I've done here.

@jawshooah
Copy link
Contributor

Closed in b4c92f3.

@jawshooah jawshooah closed this Jan 11, 2016
@DomT4 DomT4 deleted the 0.60.1 branch January 11, 2016 17:50
@DomT4
Copy link
Member Author

DomT4 commented Jan 11, 2016

Thanks!

@adidalal adidalal removed the awaiting maintainer feedback Issue needs response from a maintainer. label Apr 12, 2016
@miccal miccal removed core Issue with Homebrew itself rather than with a specific cask. enhancement labels Dec 23, 2016
@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants