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

Handle broken symlinks on install, install --force #21893

Merged
merged 1 commit into from
Jun 12, 2016
Merged

Handle broken symlinks on install, install --force #21893

merged 1 commit into from
Jun 12, 2016

Conversation

claui
Copy link
Contributor

@claui claui commented Jun 12, 2016

While testing PR #21857 earlier today, I have found that brew cask install fails when it encounters a broken symlink, no matter if with or without --force:

image

Users might run into this in the wake of PR #21857, e. g. trying to move their Caskrooms around in an attempt to get brew cask list back working, or to merge their Caskrooms. One could argue that users are not supposed to do this, but we all know they will do it anyway. The issue will also bite unsuspecting users once PR #21858 is merged. Any legacy (pre-#13966) symlink pointing inside a custom Caskroom location is likely going to break.

I feel that while install needs to fail gracefully, install --force should do exactly what it says on the tin and sweep broken symlinks out of the way.

This PR fixes both the install and install --force case.

Note that the uninstall and zap cases have similar issues; those remain unfixed for the time being. Those cases have additional issues anyway, require a bit more effort to be fixed, and thus deserve a PR on their own.

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.
@claui claui added bug Issue describing a reproducible bug. awaiting maintainer feedback Issue needs response from a maintainer. core Issue with Homebrew itself rather than with a specific cask. labels Jun 12, 2016
@jawshooah jawshooah merged commit eac934f into Homebrew:master Jun 12, 2016
@jawshooah
Copy link
Contributor

Great work once again, @claui 😄

@jawshooah jawshooah removed the awaiting maintainer feedback Issue needs response from a maintainer. label Jun 12, 2016
@claui
Copy link
Contributor Author

claui commented Jun 12, 2016

Thanks @jawshooah 😊

@claui claui deleted the consider-broken-symlinks branch June 12, 2016 18:22
chizmw pushed a commit to chizmw/homebrew-cask that referenced this pull request Jun 15, 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 Homebrew#21857, and certainly _will_ be bitten by it once PR Homebrew#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.
@miccal miccal removed bug Issue describing a reproducible bug. core Issue with Homebrew itself rather than with a specific cask. 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.

3 participants