Let self-managed pipx uninstall itself on windows again

[guahki]

• I have added a news fragment under changelog.d/ (if the patch affects the end users)

Summary of changes

Change deletion of directories to ignore errors when safe_rm is true, such that locked binaries on windows can be moved to the trash afterwards. Fixes #1203.

Comment

In #1203 I proposed the following solutions:

1. Revert the deleting of the folder to use the os.system call on windows
2. Ignore errors in shutil.rmtree using ignore_errors=True (I didn't test this yet, but it should work imho)
3. Catch the specific PermissionError and ignore it (I didn't test this yet, but it should work imho)

I implemented solution 2 (see #1203 (comment) for reasoning). To be more precise, I use ignore_errors=safe_rm, so only ignoring errors, when the left-over directory is afterwards moved to the trash.

Furthermore, I removed the surrounding try-except-block, as this (after the introduction of ignore_errors) has no effect imho:

1. either safe_rm is True, then errors are ignored and thus no exception should ever be raised (and potentially catched)
2. or safe_rm is False, in which case the except FileNotFoundError really could make a difference. However, the if not path.is_dir(): in the first line of the function should guard against this case. This is further backed by the fact, that the try-except-block was introduced far before the safe-guard if (2aea78e 5 years ago vs. 6e1907b 3 years ago). If there is any concern about this evaluation, please let me know.

Test plan

Tested by installing pipx into pipx on windows and then running

pipx uninstall pipx

which failed before and now works

[Gitznik]

Can you verify this behavior in a test please?

[guahki]

I can easily add some basic tests, trying to delete a non-existing folder and an existing one.

But this would not really test the "gist" of this function, the safe_rm feature, which moves files to the "trash" on windows, if they are locked (e.g. due to being a running executable). I'm not sure, if this is even testable and tbh I'm not willing to spend lot's of time finding a solution, how to test this. At least, I added a comment (as also suggested in the last paragraph of #1203 (comment)).

I understand, that adding new tests with each PR improves the tests and test coverage. But to request a test from me for a "bugfix" like this (obviously no test was requested, when this was broken in #1168; and most likely the test would not have found the issue, due to the facts described above) seems a little hard for me.

So: I can assist with providing "basic" tests, which prove nothing (or very little) and most likely do not catch newly introduced bugs, if you want me to. But I would see it as timewaste tbh. If you want real tests, I have to ask for assistance from somebody with the time to think about, how the locked-file scenario can be tested properly.

[Gitznik]

I understand, that adding new tests with each PR improves the tests and test coverage. But to request a test from me for a "bugfix" like this (obviously no test was requested, when this was broken in #1168; and most likely the test would not have found the issue, due to the facts described above) seems a little hard for me.

The point is not necessarily adding a test with each PR, it's adding a test with every bugfix to make sure we're not regressing. The point that no test was added in #1168 is not an argument against writing tests, it is a lesson that we should've done it.

I appreciate this might not be easy to test. I will try and look into it a little, but as I'm not on windows it's a little cumbersome for me to play around with it. I appreciate any windows user that is willing to spend some brain power on this as well.

Should we not be able to find a way to test this, IMO we can merge it, but we should do our due diligence first trying to find a meaningful test.

[guahki]

The point is not necessarily adding a test with each PR, it's adding a test with every bugfix to make sure we're not regressing. The point that no test was added in #1168 is not an argument against writing tests, it is a lesson that we should've done it.

Yes, I get that.

I appreciate this might not be easy to test. I will try and look into it a little, but as I'm not on windows it's a little cumbersome for me to play around with it. I appreciate any windows user that is willing to spend some brain power on this as well.

Would be very much thankful for any help here, as I unfortunately most likely will not soon have the time to develop a sensible test strategy and write sensible testcases for this one.

Should we not be able to find a way to test this, IMO we can merge it, but we should do our due diligence first trying to find a meaningful test.

👍

[Gitznik]

I tried a little bit but had a hard time reproducing the behavior in a test as well.

Manually, @guahki verified the behavior on windows, I did on Linux. I think this is ready for review.

Not reasonable to test

[guahki]

I tried a little bit but had a hard time reproducing the behavior in a test as well.

Thanks for taking the time and trying! I think one would have to have a running executable and try to delete that. Not an easy task.

Manually, @guahki verified the behavior on windows, I did on Linux. I think this is ready for review.

Thank you for all the moderation and help. Please let me know, if there is anything to do for me on this PR.