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

Fix make uninstall #231

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix make uninstall #231

wants to merge 1 commit into from

Conversation

ntadej
Copy link

@ntadej ntadej commented Oct 21, 2016

The double quotes in make uninstall prevented rm from working properly at least on OS X

The double quotes in make uninstall prevented `rm` from working properly at least on OS X
@benjamin-weiss
Copy link
Contributor

Why do you want to uninstall it? Metropolis is awesome. 😄

@matze
Copy link
Owner

matze commented Oct 21, 2016

Besides the reason given by @benjamin-weiss, we should look for another solution because of more severe implications: The quotes were introduced because passing directory name with spaces in them might delete unrelated directories.

@ntadej
Copy link
Author

ntadej commented Oct 21, 2016

@benjamin-weiss I made a personal fork with some changed colors and defaults, so I renamed it and wanted to test if makefile still works 😊

@matze The current uninstall does not work. I can try iterating over the files and appending path to each one, but I'm not very fluent in make.

@rchurchley
Copy link
Contributor

What if we just add quotes to the problematic directory strings? That is, instead of adding quotes to each command in make uninstall, we define

DESTDIR ?= "$(shell kpsewhich -var-value=TEXMFHOME)"

instead of

DESTDIR ?= $(shell kpsewhich -var-value=TEXMFHOME)

I think this should properly escape the things that need to be escaped without breaking the uninstall script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants