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 aggressive "rm -rf" in uninstall target #389

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nega0
Copy link
Contributor

@nega0 nega0 commented Feb 11, 2025

lcov's make uninstall wiped my ~/.local/share. This was... surprising.

The offending line is

lcov/Makefile

Line 190 in b1a5332

rm -rf `dirname $(SHARE_INST_DIR)`

SHARE_DIR_INST comes from:

SHARE_DIR := $(PREFIX)/share/lcov/
SHARE_INST_DIR := $(DESTDIR)$(SHARE_DIR)

So, make uninstall PREFIX=${HOME}/.local would give us:

rm -rf `dirname /home/nega/.local/share/lcov`

and, since dirname gives us the "last non-slash component and trailing slashes removed" we're just doing

rm -rf /home/nega/.local/share`

On the Good/Bad Scale, this ranks as a "Bad". I'm glad I didn't use the default PREFIX of /usr/local. Losing /usr/local/share would have been more than an "inconvenience".

This PR removes this gross hammer, by removing the examples and tests from the make install target. It also cleans up the manpages that make uninstall ignores.

Ideally, the uninstall target would be removed outright until a less fragile version is developed.

Signed-off-by: nega [email protected]


  • prefer to be explicit when constructing paths
  • fix aggressive "rm -rf" with a less aggressive "rm -rf"
  • remove examples and tests from the install target

This makes things more clear when looking at `find`, "shell", and `make`
syntax on one line (and removes double slashes in the output.)

Signed-off-by: nega <[email protected]>
`make uninstall` deletes `$(PREFIX)/share` instead of
`$(PREFIX)/share/lcov` with `rm -rf`. This is surprising. This patch
fixes that unconditional delete by deleting `$(PREFIX)/share/lcov`. This
is still not optimal, and wouldn't be necessary if the tests were not
installed.

This patch also cleans up the installed manpages.

Signed-off-by: nega <[email protected]>
This eliminates the need for `rm -rf` in the uninstall target.

Signed-off-by: nega <[email protected]>
@nega0
Copy link
Contributor Author

nega0 commented Feb 11, 2025

sigh. i guess the work flows will have to be reworked.

@nega0
Copy link
Contributor Author

nega0 commented Feb 11, 2025

i see... i think it's incorrect that lcov is trying to remove directories that it didn't install. obviously discussion is warrented.

@hartwork
Copy link
Contributor

Hi @nega0, I'm sorry to hear about your experience with LCOV's make uninstall.

On a detail level, I'd like to note that || true is likely not the most natural way to continue on error, there is prefix - for that. Let me demo that in a Bash terminal:

# make -f <(echo $'all:\n\t-false\n\techo hello' | tee /dev/stderr) ; echo $?
all:
        -false
        echo hello
false
make: [/dev/fd/63:2: all] Error 1 (ignored)
echo hello
hello
0

Maybe deleting the whole target would indeed be an option that should be considered or having its use-cost-ratio be evaluated.

@nega0
Copy link
Contributor Author

nega0 commented Feb 11, 2025

I just followed conventions already established in the file and introduced 16 months a go in 98f1779. You'll notice another convention I followed was the use of $(RM) -f .... By default GNU make sets RM to rm -f, making the added -f superfluous. It would be "more correct" to just use $(RM). The goal of this PR is to correct a dangerous operation, not to correct stylistic conventions.

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.

2 participants