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

Add support for home-relative cask uninstall script paths #5677

Open
nbadal opened this Issue Feb 5, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@nbadal
Copy link

nbadal commented Feb 5, 2019

A detailed description of the proposed feature

Currently, 'uninstall script' assumes the path is relative to the staged path. This should be extended to allow absolute paths as well (similar to install script), depending on the formatting of the argument supplied.

Uninstall script currently:

executable_path = cask.staged_path.join(executable)

How this is approached in the install phase:

absolute_path = if path.absolute?
path
else
cask.staged_path.join(path)
end
if absolute_path.exist? && !absolute_path.executable?
FileUtils.chmod "+x", absolute_path
end
executable = if absolute_path.exist?
absolute_path
else
path
end

The motivation for the feature

Some applications create an uninstall script outside of the staged directory (ex: Fusion 360 creates ~/Applications/Remove Fusion 360.app/.../.../.../) which are currently inaccessible for new casks.

How the feature would be relevant to at least 90% of Homebrew users

It allows apps following this pattern to be added 😄

What alternatives to the feature have been considered

Workarounds are ugly at best: manually recreating the uninstall script via a bunch of deletes, or prepending ../../../../../ ad nauseum to the relative script path.

@nbadal nbadal closed this Feb 5, 2019

@nbadal nbadal reopened this Feb 5, 2019

@MikeMcQuaid MikeMcQuaid added the cask label Feb 5, 2019

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 5, 2019

CC @vitorgalvao @Homebrew/cask for thoughts on this.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Feb 5, 2019

Absolute paths are already supported, because Pathname#join replaces the path if the joined path is absolute, it's just that home-relative paths are not expanded before that happens.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 5, 2019

Cool, thanks @reitermarkus. My understanding is this can be closed but reopen if I'm wrong.

@MikeMcQuaid MikeMcQuaid closed this Feb 5, 2019

@nbadal

This comment has been minimized.

Copy link
Author

nbadal commented Feb 5, 2019

Should I open a new request for home-relative paths in that case? If I'm understanding, the last if absolute_path.exist? block is what allows for that in the installation snippet.

@MikeMcQuaid MikeMcQuaid reopened this Feb 5, 2019

@nbadal nbadal changed the title Add support for absolute cask uninstall script paths Add support for home-relative cask uninstall script paths Feb 5, 2019

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Feb 6, 2019

@nbadal, yes, please send a pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment