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.

Sign up for GitHub

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

Jump to bottom

brew-cask: move to using tap cmd directory. #15381

Merged
merged 3 commits into from
Dec 9, 2015
Merged

brew-cask: move to using tap cmd directory. #15381

merged 3 commits into from
Dec 9, 2015

Conversation

MikeMcQuaid
Copy link
Member

This provides a few benefits:

  • faster brew cask execution times as another Ruby process is not needed. Cask can instead be loaded in-process with Homebrew. This will also make it easier to use some of Homebrew's core code and ease moving code from Cask into Homebrew core.
  • Users do not need to brew upgrade Cask any more: it's done automatically on any brew update or git pull of the Cask tap.

I appreciate this PR will probably not be merged as-is while we work out how to handle e.g. the Formula and I answer other questions.

CC @vitorgalvao @DomT4 @xu-cheng

@xu-cheng
Copy link
Member

Probably want to update brew-cask formula to an dummy one, i.e. bumping the version number but installing nothing.

@MikeMcQuaid
Copy link
Member Author

Probably want to update brew-cask formula to an dummy one, i.e. bumping the version number but installing nothing.

I was thinking the same 👍

@adidalal adidalal added enhancement core Issue with Homebrew itself rather than with a specific cask. labels Nov 24, 2015
@jawshooah
Copy link
Contributor

One question about @xu-cheng's suggestion: what about existing installations which symlink to the brew-cask script? Could the upgrade remove them?

@jawshooah
Copy link
Contributor

Also, the failing test is due to the cmd directory not being listed here.

@xu-cheng
Copy link
Member

what about existing installations which symlink to the brew-cask script? Could the upgrade remove them?

The symlink will be removed. The only residual file will be an empty /usr/local/Cellar/brew-cask/version/ directory.

@MikeMcQuaid
Copy link
Member Author

Also, the failing test is due to the cmd directory not being listed here.

@jawshooah Yeh, will fix up the CI and do more testing.

@vitorgalvao
Copy link
Member

I should probably mention I’ve already seen this as well. I like the idea and have nothing to add.

I’ll ask that you please always @mention @jawshooah in these situations as well, since he’s taking care of most of the actual implementation and revisions to the core.

@DomT4
Copy link
Member

DomT4 commented Nov 26, 2015

Don't have much to comment beyond to say the idea here is great 👍.

@MikeMcQuaid
Copy link
Member Author

Sure thing @vitorgalvao, thanks. I just mentioned you in this case as you commented on the Homebrew issue that provoked this.

@MikeMcQuaid
Copy link
Member Author

This is green now. The only outstanding issue I noticed from the formula is how we want to handle the manpage. CC @xu-cheng for ideas.

@vitorgalvao vitorgalvao mentioned this pull request Dec 2, 2015
Closed
@MikeMcQuaid
Copy link
Member Author

Can one of the Homebrew Cask maintainers let me know what needs to be changed (except the man fix) to make this suitable for merging?

@jawshooah
Copy link
Contributor

LGTM. Not sure how manpages are usually done for external brew commands, but we can open a separate issue for that. @vitorgalvao, any objections to merging this now?

@vitorgalvao
Copy link
Member

No objections at all. I was also waiting on the mentioned manpage ideas, but I agree that can be a different issue.

@jawshooah
Copy link
Contributor

With this merged, the version number will no longer be relevant. We'll need to bump it once to prompt users to upgrade, but after that it will have no further use. How do you want to handle that?

@vitorgalvao
Copy link
Member

Ah, yes, hadn’t start thinking about that transition yet. Any suggestions?

@jawshooah
Copy link
Contributor

HBC_VERSION doesn't appear to be used anywhere apart from generating the output for brew cask --version and brew cask doctor, so we may as well keep it around. Homebrew's version hasn't been updated since 2013, but they haven't removed it. We should definitely include the git revision in the output of those commands, though.

We can get rid of releasing.md and the bump_version, generate_changelog, and get_release_tag developer scripts, as they won't be needed anymore.

Our final release notes should emphasize this change, and that there will be no more releases in the future.

@adidalal
Copy link
Contributor

adidalal commented Dec 7, 2015

The no more releases is independent from Cask DSL changes, correct? Because PRs like #15643 (any any other additional features) will still require a bump in the cask versioning.

@jawshooah
Copy link
Contributor

@adidalal DSL versioning is completely independent from Cask's own version.

@jawshooah
Copy link
Contributor

It's important to note, however, that we aren't currently performing any meaningful validation of the DSL version. I started working on it a while ago, but haven't gotten around to finishing up.

@xu-cheng
Copy link
Member

xu-cheng commented Dec 7, 2015

About manpage, a possible workaround is to auto install it whenever invoking brew cask command.

About version, you may want to create a develop branch for core code development.

@MikeMcQuaid
Copy link
Member Author

We can get rid of releasing.md and the bump_version, generate_changelog, and get_release_tag developer scripts, as they won't be needed anymore.

I'll remove these.

About manpage, a possible workaround is to auto install it whenever invoking brew cask command.

@xu-cheng as this case seems relatively common I think I'll add an optional man directory to the tap root which is autosymlinked. I think that should probably block this being merged.

@MikeMcQuaid MikeMcQuaid mentioned this pull request Dec 8, 2015
Closed
@MikeMcQuaid
Copy link
Member Author

Opened a Homebrew PR for linking manpages in Homebrew/legacy-homebrew#46795.

@MikeMcQuaid
Copy link
Member Author

@jawshooah I think I've done everything you suggested and made sure brew cask doctor was clean for me.

jawshooah
jawshooah reviewed Dec 8, 2015
View reviewed changes
lib/hbc/version.rb
module Hbc
def self.full_version
@full_version ||= begin
revision, commit = Hbc.default_tappath.cd do
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have Homebrew's Pathname extensions, so this line breaks the build.

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, worked for me locally because this is loaded in the same process as Homebrew. I'll fix it up regardless.

@MikeMcQuaid
Copy link
Member Author

@jawshooah Comments addressed (and I only just "got" your username)

@MikeMcQuaid MikeMcQuaid deleted the cask-cmd branch December 9, 2015 20:36
@MikeMcQuaid
Copy link
Member Author

@vitorgalvao Thanks for the kind words, great project and @jawshooah thanks for the merge!

@DomT4
Copy link
Member

DomT4 commented Dec 9, 2015

Seeing some issues locally:

Updated 1 tap (caskroom/cask).
Error: Could not link caskroom/cask manpages to:
  /usr/local/share/man/man1/brew-cask.1

Please delete these files and run `brew tap --repair`.
No changes to formulae.
==> Upgrading 1 outdated package, with result:
caskroom/cask/brew-cask 0.60.0
==> Upgrading caskroom/cask/brew-cask
rm /usr/local/bin/brew-cask
rm /usr/local/share/man/man1/brew-cask.1
==> Cloning https://github.com/caskroom/homebrew-cask.git
git clone --depth 1 --branch v0.60.0 https://github.com/caskroom/homebrew-cask.git /usr/local/var/homebrew/cache/brew-cask--git
Cloning into '/usr/local/var/homebrew/cache/brew-cask--git'...
warning: Could not find remote branch v0.60.0 to clone.
fatal: Remote branch v0.60.0 not found in upstream origin
Error: Failed to download resource "brew-cask"
Failure while executing: git clone --depth 1 --branch v0.60.0 https://github.com/caskroom/homebrew-cask.git /usr/local/var/homebrew/cache/brew-cask--git

@DomT4
Copy link
Member

DomT4 commented Dec 9, 2015

Might be resolved by f2c2f85. Double checking now.

@DomT4
Copy link
Member

DomT4 commented Dec 9, 2015

👍. Got the manpage link failure still but otherwise all issues resolved. Probably fairly unlikely anyone who wasn't watching this issue tried to update in the last 7 minutes, heh.

This was referenced Dec 9, 2015
Closed
Closed
@vitorgalvao vitorgalvao mentioned this pull request Dec 9, 2015
Closed
@fanquake
Copy link
Contributor

👍

MikeMcQuaid pushed a commit to Homebrew/legacy-homebrew that referenced this pull request Dec 10, 2015
@jawshooah @MikeMcQuaid
Do not brew install brew-cask
60199cc 
Refs Homebrew/homebrew-cask#15381

Closes #46845.
reitermarkus added a commit to reitermarkus/dotfiles that referenced this pull request Dec 10, 2015
@reitermarkus
brew-cask is now an external command.
c2aa9a2 
Homebrew/homebrew-cask#15381
jawshooah added a commit that referenced this pull request Dec 11, 2015
@jawshooah
Merge pull request #15777 from proinsias/usage_brew_cask
6cb924b 
Update USAGE.md to remove brew-cask install #15381
@jawshooah jawshooah mentioned this pull request Dec 12, 2015
Closed
ryotarai added a commit to ryotarai/itamae-plugin-resource-cask that referenced this pull request Dec 14, 2015
@ryotarai
brew-cask is no longer available since Homebrew/homebrew-cask#15381.
1f5d470
@ryotarai ryotarai mentioned this pull request Dec 14, 2015
Merged
k0kubun added a commit to itamae-plugins/itamae-plugin-resource-cask that referenced this pull request Dec 14, 2015
@k0kubun
Merge pull request #2 from ryotarai/brew-cask-cmd
5330cd2 
brew-cask is no longer available since Homebrew/homebrew-cask#15381.
@Amorymeltzer Amorymeltzer mentioned this pull request Dec 15, 2015
Closed
@zmwangx zmwangx mentioned this pull request Dec 17, 2015
Closed
@Amorymeltzer Amorymeltzer mentioned this pull request Dec 20, 2015
Merged
Amorymeltzer added a commit to Amorymeltzer/homebrew-cask that referenced this pull request Dec 20, 2015
@Amorymeltzer
Update links/paths in manpage, hacking.md, usage.md to reflect struct…
e766cba 
…ure of brew-cask

- Old github.com links were invalid
- Old Cellar structure was invalid
- Man page regenerated, including content from Homebrew#15643

Refs Homebrew#15381
@swrobel
Copy link
Contributor

swrobel commented Dec 23, 2015

@MikeMcQuaid this is awesome! 👏 Merry christmas to us all! 🎅

@naffan2014
Copy link

不错的想法！ 虽然我不怎么懂吧

@vitorgalvao
Copy link
Member

@naffan2014 English only, please. Google Translate returns “Good idea! Although I do not understand it”.

In essence, brew update will also now upgrade homebrew-cask automatically.

@naffan2014
Copy link

lol. google is great . it translate exactly what i mean .haha

@hovancik
Copy link
Contributor

hovancik commented Jan 2, 2016

so i tried this today, but I am getting error:

$ brew uninstall --force brew-cask; brew update
Already up-to-date.
$ brew cask install Caskroom/cask/flash-player
Error: Unknown command: cask
$ brew cask install Caskroom/cask/flash-player --verbose
Error: Unknown command: cask
Error: Kernel.exit
$ brew update; brew cleanup; brew cask cleanup
Already up-to-date.
Warning: Skipping (old) keg-only: /usr/local/Cellar/openssl/1.0.2d_1
Error: Unknown command: cask
$ brew cask doctor
Error: Unknown command: cask

@adidalal
Copy link
Contributor

adidalal commented Jan 2, 2016

@hovancik and others: Please make a new issue so we can avoid adding unrelated comments to this (merged) PR.

As a pre-emptive solution, please try:

brew update; brew cleanup; brew cask cleanup
brew uninstall --force brew-cask; brew update

If you are still having problems, open an issue with the appropriate template

@Homebrew Homebrew locked and limited conversation to collaborators Jan 2, 2016
@miccal miccal removed core Issue with Homebrew itself rather than with a specific cask. enhancement labels Dec 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.