pip icon indicating copy to clipboard operation
pip copied to clipboard

Display windows path in its original case

Open w1redch4d opened this issue 3 years ago • 13 comments

Fixes #6823

w1redch4d avatar Jul 04 '22 07:07 w1redch4d

Another deeper problem is that abspath() is not semantically the same as resolve()—the latter follows symbolic links. So we need to not use resolve, and review callers of this function to ensure they are passing in a correct value.

uranusjr avatar Jul 04 '22 07:07 uranusjr

Another deeper problem is that abspath() is not semantically the same as resolve()—the latter follows symbolic links. So we need to not use resolve, and review callers of this function to ensure they are passing in a correct value.

yep it does follow the symlinks but it also requires the path to exist tho, so i dont think that might be a issue?

If the path doesn’t exist and strict is True, FileNotFoundError is raised.

w1redch4d avatar Jul 04 '22 09:07 w1redch4d

The issue is that after following symlinks path may no longer start with os.getcwd(), making the check return a different result.

uranusjr avatar Jul 20 '22 02:07 uranusjr

so should we take this approach instead?:

if WINDOWS:
        path = os.path._getfinalpathname(path).lstrip(r"\\?")
else:
        path = os.path.normcase(os.path.abspath(path))
if path.startswith(os.getcwd() + os.path.sep):
        path = "." + path[len(os.getcwd()) :]
return path

w1redch4d avatar Jul 27 '22 17:07 w1redch4d

I definitely prefer to not use private functions.

uranusjr avatar Jul 28 '22 08:07 uranusjr

Agreed. This seems like way too much effort for what is essentially a cosmetic issue, and it's going to result in a maintenance burden because future maintainers won't know why we've chosen to use such a complex and fragile approach.

Can I suggest that we take a step back and understand what we're trying to achieve with display_path? As far as I can see, all we're trying to do is to express the path as relative to the CWD, if we can. There's a standard function for that in pathlib:

original = Path(path)
try:
    # Let's see if we can describe that relative to the CWD
    simplified = original.relative_to(Path.getcwd())
except ValueError:
    # Nope, so we assume what we have is good enough
    return path

return str(simplified)

If that doesn't do what we want, then we can look at when it fails, and understand what we want in that case and extend the code appropriately. Ideally with comments explaining why we need each additional bit of complexity. But honestly, I wouldn't bother - I'd reframe display_path as doing precisely what relative_to does, on the basis that pathlib is the reference implementation on "how to manipulate paths in a cross-platform way".

pfmoore avatar Jul 28 '22 09:07 pfmoore

return str(simplified)

That should be os.fsdecode instead of str but... same idea. :)

pradyunsg avatar Jul 28 '22 17:07 pradyunsg

@pfmoore if you look at the original issue, we gotta use resolve() with pathlib in that case, but i dont think thats the way pip should be due to reasons previously mentioned by @uranusjr , so i wonder if there is a way to do it without resolve() from pathlib or private function from os?

w1redch4d avatar Jul 30 '22 20:07 w1redch4d

Sorry, I don't see why we have to use resolve(). Can you be more specific about where that's necessary? If the problem is that display_path is being called with a value that's not in the original case, then the fix is to work out when we lost the original case, and fix the problem there, not try to recover the original case later after it's already been lost...

This is what @uranusjr said here, though, so I'm confused why you are still insisting we need to use resolve().

pfmoore avatar Jul 30 '22 21:07 pfmoore

gotcha

w1redch4d avatar Jul 30 '22 21:07 w1redch4d

Sorry, I don't see why we have to use resolve(). Can you be more specific about where that's necessary? If the problem is that display_path is being called with a value that's not in the original case, then the fix is to work out when we lost the original case, and fix the problem there, not try to recover the original case later after it's already been lost...

This is what @uranusjr said here, though, so I'm confused why you are still insisting we need to use resolve().

just to confirm , may i know how i might recover the original case of the path, from where it begins? like the path by default in python is lowercase, so any nudges on this would be helpful, for example, say this is the part from where we get the path of the dependecy:

@property
    def location(self) -> Optional[str]:
        return self._dist.location

@property
    def installed_location(self) -> Optional[str]:
        egg_link = egg_link_path_from_location(self.raw_name)
        if egg_link:
            location = egg_link
        elif self.location:
            location = self.location
        else:
            return None
        return normalize_path(location)

so what should i do to retrieve the original casing, as u can see the path sensitivity is not getting "lost", its just lowercase by default

w1redch4d avatar Jul 30 '22 23:07 w1redch4d

If you want to retain the casing, you shouldn't use normalize_path as that deliberately calls os.path.normcase.

And yes, I know that changing installed_location to return paths whose case is not normalised will cause other problems, as code will have to check properly[^1] whether paths match rather than just doing a string comparison. I'm not suggesting that it's easier to maintain the original case throughout - far from it, it's likely a lot of work. But I am saying that I think that if we want to fix this issue, we should do so properly, and not start introducing complex hacks involving internal functions just to recover the original form after we threw it away earlier.

I'll also point out that code like if path.startswith(os.getcwd() + os.path.sep) is exactly the sort of naive string comparison that results in paths getting normalised in the first place, to "make it work". Which is why I think it's far better to use path-specific methods like relative_to(), which provide the correct semantics directly.

And finally, I'll point out that if you think this is all too much work for a small cosmetic issue, then I agree with you. I'd frankly just leave the display using normalised case and not worry about it[^2].

[^1]: By which I mean taking case sensitivity into account [^2]: Hoping that at some point, someone will be interested enough to convert pip to use pathlib everywhere, at which point this issue will get resolved naturally.

pfmoore avatar Jul 31 '22 10:07 pfmoore

makes sense, ima see if i can convert all of them to pathlib, in case i have some free time

w1redch4d avatar Jul 31 '22 14:07 w1redch4d

closing it, since i am having a hard time making time for this migration, hope someone else will convert it someday :"c

w1redch4d avatar Aug 18 '22 05:08 w1redch4d