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

Change default Caskroom path #21857

Merged
merged 2 commits into from
Jun 12, 2016
Merged

Change default Caskroom path #21857

merged 2 commits into from
Jun 12, 2016

Conversation

vitorgalvao
Copy link
Member

@vitorgalvao vitorgalvao commented Jun 11, 2016

Changes to the core


Closes #21603.

Pinging @caskroom/maintainers. I’d like to merge this as soon as possible, so it still kind of fits inside the breaking changes of #13966. Separated it in two commits, core and docs.

@vitorgalvao vitorgalvao added awaiting maintainer feedback Issue needs response from a maintainer. core Issue with Homebrew itself rather than with a specific cask. labels Jun 11, 2016
@vitorgalvao vitorgalvao changed the title Caskroom Change default Caskroom path Jun 11, 2016
@phinze
Copy link
Contributor

phinze commented Jun 11, 2016

👏 Hooray! Over three years since #188 where we moved into /opt to work around Spotlight's inability to index Homebrew's prefix, and we are coming back home! 😄 💞

@MikeMcQuaid
Copy link
Member

@vitorgalvao 👍

@phinze Interesting to hear that was the original motivation and 🆒 that it's now resolved!

@jawshooah
Copy link
Contributor

All for this 👍

@fanquake
Copy link
Contributor

👍

@vitorgalvao
Copy link
Member Author

Seems like we’re all in agreement and everything is correct. Thank you all. Merging.

@vitorgalvao vitorgalvao merged commit 8318ffe into Homebrew:master Jun 12, 2016
@vitorgalvao vitorgalvao deleted the caskroom branch June 12, 2016 09:09
@claui
Copy link
Contributor

claui commented Jun 12, 2016

Ahhhh, I was just in the middle of typing my feedback :D

@vitorgalvao
Copy link
Member Author

@claui It’s still welcome! I did see your thumbs up before merging, so I hope there wasn’t a mistake that needed fixing.

Also, on a tangential note to everyone, what about we get rid of the --caskroom flag?

@claui
Copy link
Contributor

claui commented Jun 12, 2016

Looking forward for this to be merged. Reality has just caught up with me. ☺️

A few minor nitpicks:

  • In USAGE.md, there are a few places (1 2) that still refer to /opt.
  • We might want to update the caveat in zulu.

and, regarding UX:

  • How do we let users know that it is not a bug that on their first brew update after rolling out this PR, they will experience this:
    $ brew cask list
    Warning: nothing to list
  • How are users supposed to know that their /opt/homebrew-cask/Caskroom, which potentially contains several gigabytes of (unmoved) apps, is deserted and can be safely deleted?

@claui
Copy link
Contributor

claui commented Jun 12, 2016

I did see your thumbs up before merging

@vitorgalvao I couldn’t help but I was just too excited about it 😊

@vitorgalvao
Copy link
Member Author

How do we let users know that it is not a bug that on their first brew update after rolling out this PR, they will experience this

Well, brew cask list is severely broken anyway and that even sometimes translates to users not being able to use uninstall (usually the fix is just to delete the app manually). My continuous advice to users has always been that they cannot trust list anyway, as it stands. Not that it makes it OK, but it’s a bit more expected.

How are users supposed to know that their /opt/homebrew-cask/Caskroom, which potentially contains several gigabytes of (unmoved) apps, is deserted and can be safely deleted?

If they have unmoved apps, then /opt/homebrew-cask/Caskroom is still a necessity for them (and they haven’t updated those apps to the new system). Those links will still function.

This is still relatively close to #13966 to be considered part of its breaking changes (as mentioned in #13201). Fortunately, those are mostly done for now, and the small breaks we’re now discussing (#21372 and #21858) should affect just a handful of people.

You are correct, thought, this is still a change that might warrant a bigger warning. Will set one in the main README.

@claui
Copy link
Contributor

claui commented Jun 12, 2016

Well, brew cask list is severely broken anyway […]. Not that it makes it OK, but it’s a bit more expected.

Fair enough.

If they have unmoved apps, then /opt/homebrew-cask/Caskroom is still a necessity for them […].

You are correct. Personally, I’d like to avoid getting confused with multiple Caskrooms in my file system, especially while testing things and working on the core.

As a stopgap measure, I have now decided to mv /opt/homebrew-cask/Caskroom /usr/local/.
Like you correctly foreshadowed, this broke a few symlinked apps I happened to have left; I’m going to reinstall --force those now.

That said, I completely agree with you in that not many people are probably going to be affected.

@vitorgalvao
Copy link
Member Author

Done and done.

@espinielli
Copy link
Contributor

I was one of those surprised not to see my many installed casks anymore...luckily I found this issue...
Should I really re-install all?! I have hundreds of apps in /opt/homebrew-cask/Caskroom/!!!
Any suggestion?
Thanks

@dantreacy
Copy link

With regard to @claui's comment about UX and the "nothing to list" after the first brew upgrade.

To be honest possibly scaring the shit out of the user making them think they may have lost all their apps (granted I was far more worried as I'd just run a cleanup, not just an update, thought perhaps cleanup has gone mad/rouge/insane and wiped everything) is probably not the best course of action.

I understand @vitorgalvao's comments regarding brew cask list, but a lot of people use it frequently (in fact one of the most common methods I've seen for mass updating casks uses it) and haven't had any problems with it at all and just as many happily use the wonderful tool you're developing for us without ever visiting github and reading the repo's readme..

If ever there was a time to be informing users actually within the program to be "ignoring" disastrous sounding "error messages" this would be it. :)

@carlosefr
Copy link

I've got a script to check for updated casks - the reason why I use brew cask vs. upstream installers - which suddenly started reporting there's no casks installed. I'm baffled to find this was an intentional and silent change of the default Caskroom directory.

Calling this an "UX issue" is an understatement. Leaving users scratching their heads trying to figure out out what happened and wasting time looking for how to fix it is not proper procedure. I can think of a few options that would/will avoid this:

  1. Warn the user that his installed casks will become orphan.
  2. Keep using the old /opt/homebrew-cask/Caskroom when it already exists (maybe warning that it's deprecated).
  3. Merge the two locations (always install to the new location but search the old one for installed casks);
  4. Move all casks and replace the old location by a symlink to the new location.

Option 1. is the very minimum I'd expect but, aside from this thread, I can't even find it explicitly mentioned anywhere...

jawshooah pushed a commit that referenced this pull request Jun 12, 2016
This commit fixes an error occurring with both `install` and `install --force` where either would fail when encountering a broken symlink.

Users _might_ run into that issue in the wake of PR #21857, and certainly _will_ be bitten by it once PR #21858 is merged.

This commit changes the `install` case so it handles the condition gracefully. It also fixes `install --force` so before moving an app, it removes any broken symlinks which might stand in the way.
@vitorgalvao
Copy link
Member Author

vitorgalvao commented Jun 12, 2016

Locking this issue. Please direct any further comments after #21894 (comment).

That comment addresses the change, why it happened, what will be done about it and a workaround for the meantime (it actually points to the comments that detail those). Apologies for the disturbance.

@Homebrew Homebrew locked and limited conversation to collaborators Jun 12, 2016
@adidalal adidalal removed the awaiting maintainer feedback Issue needs response from a maintainer. label Jul 2, 2016
@miccal miccal removed the core Issue with Homebrew itself rather than with a specific cask. label Dec 23, 2016
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.