uninstall: remove symlinks created by flit install --symlink

[sbidoul]

Fixes #6892

This PR adds a failing test and a fix for uninstalling in a scenario that is very close to what flit install --symlink does.

The fix has two parts:

• avoid a redundant path normalization in is_local that was defeating careful head normalization in req_uninstall.py.
• avoid accidentally changing permission of the symlink target by considering symlinks as files instead of directories in StashedUninstallPathSet

[sbidoul]

@cjerdonek thanks for the review. Simplification done.

[cjerdonek]

@sbidoul Thanks for the updates. Re: your comment here on the original issue:

But it reveals another issue: when uninstalling such symlinks, pip ends up modifying the permissions on the target directory to 0200...

Can you point out whether / how this PR addresses that? Is a code comment warranted if this was an issue that went unnoticed in the past?

[sbidoul]

@cjerdonek I just extracted the part concerning stash/commit/rollback of symlinks in a separate commit and added extensive tests.

There is also an integration test for the mode change:

pip/tests/functional/test_uninstall.py

Line 479 in f8d5825

assert symlink_target.stat().st_mode == st_mode

[cjerdonek]

Sorry, I didn't mean to suggest this PR should (or needed) to address that comment. I was just asking because I wasn't sure. Does the second fix have to go with the first fix? In other words, would it cause problems if only the first part was done? If not, I think it would be better to do the second part as a separate issue and PR because it seems like a distinct issue and would make it easier to understand and review.

[sbidoul]

I think both should be addressed together. Before this PR, uninstalling was merely leaving symlinks behind in the virtualenv. With the first part of this PR, the symlinks get removed but the stash mechanism corrupts the symlink targets. So I think only merging the first part would make the situation worse and it's best to fix the stashing mechanism at the same time.

Is the separate commit sufficient to facilitate review or do you prefer a separate PR and merge them together?

[sbidoul]

Also, the test for part 2 needs improvements in tests/lib/path.py that are in part one. So splitting the PR is somewhat more complicated.

[cjerdonek]

So I think only merging the first part would make the situation worse and it's best to fix the stashing mechanism at the same time.

Okay, this is good to know then. Before you had proposed only the first part without mentioning that more work needed to be done.

Is the separate commit sufficient to facilitate review or do you prefer a separate PR and merge them together?

If both parts need to be committed together, then it should be one PR.

[chrahunt]

Can we just normalize the head of path here and below?

[sbidoul]

We could, but I think the callers need to do it too anyway, so for the use cases we have it would be redundant.

[chrahunt]

Better redundant and give a consistent interface with no gotchas IMO, but I won't block this on this point.

[chrahunt]

LGTM.

[chrahunt]

Thanks!