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

Add support for Path.absolute() #73874

Closed
DimitrisJim mannequin opened this issue Mar 1, 2017 · 21 comments
Closed

Add support for Path.absolute() #73874

DimitrisJim mannequin opened this issue Mar 1, 2017 · 21 comments
Labels
3.8 (EOL) end of life 3.9 only security fixes 3.10 only security fixes docs Documentation in the Doc dir stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@DimitrisJim
Copy link
Mannequin

DimitrisJim mannequin commented Mar 1, 2017

BPO 29688
Nosy @brettcannon, @pitrou, @methane, @serhiy-storchaka, @eryksun, @DimitrisJim, @barneygale
PRs
  • bpo-29688: Document Path.absolute #384
  • bpo-29688: document and test pathlib.Path.absolute(). #26153
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2017-03-01.17:37:28.703>
    labels = ['3.8', '3.9', '3.10', 'type-feature', 'library', 'docs']
    title = 'Add support for Path.absolute()'
    updated_at = <Date 2022-01-31.23:20:11.549>
    user = 'https://github.com/DimitrisJim'

    bugs.python.org fields:

    activity = <Date 2022-01-31.23:20:11.549>
    actor = 'eryksun'
    assignee = 'docs@python'
    closed = False
    closed_date = None
    closer = None
    components = ['Documentation', 'Library (Lib)']
    creation = <Date 2017-03-01.17:37:28.703>
    creator = 'Jim Fasarakis-Hilliard'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 29688
    keywords = ['patch']
    message_count = 20.0
    messages = ['288767', '288923', '288931', '289413', '289436', '289452', '289460', '289507', '289509', '289526', '358642', '393731', '395689', '412044', '412099', '412102', '412219', '412221', '412222', '412223']
    nosy_count = 9.0
    nosy_names = ['brett.cannon', 'pitrou', 'methane', 'docs@python', 'serhiy.storchaka', 'eryksun', 'Jim Fasarakis-Hilliard', '4-launchpad-kalvdans-no-ip-org', 'barneygale']
    pr_nums = ['384', '26153']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue29688'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    @DimitrisJim
    Copy link
    Mannequin Author

    DimitrisJim mannequin commented Mar 1, 2017

    Method absolute of Path objects lacked documentation, proposed PR adds relevant method to docs.

    @DimitrisJim DimitrisJim mannequin assigned docspython Mar 1, 2017
    @DimitrisJim DimitrisJim mannequin added 3.7 (EOL) end of life docs Documentation in the Doc dir labels Mar 1, 2017
    @brettcannon brettcannon assigned brettcannon and unassigned docspython Mar 2, 2017
    @brettcannon
    Copy link
    Member

    As brought up on the PR, it turns out Path.absolute() is extremely under-tested.

    Perhaps we should deprecate Path.absolute() instead of document it and properly test it (and the testing will be necessary to move forward with the documentation)? Path.resolve() handles absolute paths already while also resolving '.' and '..': https://docs.python.org/3/library/pathlib.html#pathlib.Path.resolve. It also works with non-existent paths so unless there's some performance issue I'm not aware of for resolving '.' and '..', then I say we deprecate Path.absolute().

    @brettcannon
    Copy link
    Member

    I've closed the PR on GitHub until we decide whether we just want to deprecate Path.absolute() in favour of Path.resolve().

    @brettcannon
    Copy link
    Member

    I'm still thinking about this but I'm still leaning towards deprecating pathlib.absolute().

    @eryksun
    Copy link
    Contributor

    eryksun commented Mar 11, 2017

    resolve() can't replace absolute(). They serve different purposes. Sometimes one wants an absolute path, but without resolving symbolic links.

    absolute() processes a path as a string, which will continue to be true if it's updated to call nt._getfullpathname (GetFullPathName) on Windows. OTOH, resolve() can outright fail on Windows. I can write up a list of examples (I can think of 5 or 6 unhandled error codes), but it's not directly relevant to this issue.

    @brettcannon
    Copy link
    Member

    I know it has it's uses (avoiding stat calls is one of them), but it is still undocumented, untested, and has two comments in it saying it needs work. Because of all that it might as well not exist since it doesn't meet our standards of quality.

    If someone wants to fix all those issues then we can properly document it as supported, but if no one is willing to then I don't think we should leave unsupported code lying around that people might discover through dir().

    And it doesn't serve a _different_ purpose compared to resolve(), it serves a _subset_ of resolve()'s purpose since resolve() calls absolute() unconditionally.

    @brettcannon brettcannon changed the title Document Path.absolute Add support for Path.absolute Mar 11, 2017
    @brettcannon brettcannon changed the title Add support for Path.absolute Add support for Path.absolute() Mar 11, 2017
    @eryksun
    Copy link
    Contributor

    eryksun commented Mar 12, 2017

    What's the rationale for not calling self._flavour.pathmod.abspath() to implement absolute()? For example:

        >>> p = pathlib.Path('C:/con')
        >>> p._flavour.pathmod.abspath(p)
        '\\\\.\\con'
        >>> p._from_parts((p._flavour.pathmod.abspath(p),), init=False)
        WindowsPath('//./con/')

    That's almost right except for an unrelated problem that pathlib shouldn't append a trailing slash for \\.\ local device paths. Doing so creates a different path, which may be invalid. \\.\con is a symbolic link to \Device\ConDrv\Console, and adding a trailing backslash after the "Console" filename is invalid. An example where the resulting path is valid but wrong is the volume device \\.\C:, which is a link to something like \Device\HarddiskVolume2. Appending a backslash refers to the root directory of the file system on the volume.

    @brettcannon
    Copy link
    Member

    "What's the rationale for not calling self._flavour.pathmod.abspath() to implement absolute()?" Beats me. :) It's just how Antoine wrote it and that's all I know.

    @serhiy-storchaka
    Copy link
    Member

    posixpath.abspath() collapses "<path>/<symlink>/.." to "<path>", this is not correct. Not sure about ntpath.abspath().

    @methane
    Copy link
    Member

    methane commented Mar 13, 2017

    posixpath.abspath() collapses "<path>/<symlink>/.." to "<path>", this is not correct. Not sure about ntpath.abspath().

    I feel this is reasonable limitation, and expected behavior for some cases.
    So I think adding note about this in document is enough.

    @brettcannon brettcannon removed their assignment Mar 13, 2017
    @brettcannon
    Copy link
    Member

    I have opened https://bugs.python.org/issue39090 to track updating the pathlib docs to have a section on getting the absolute path in various ways along with what the trade-offs are for each approach.

    @eryksun eryksun added stdlib Python modules in the Lib dir 3.8 (EOL) end of life 3.9 only security fixes 3.10 only security fixes and removed 3.7 (EOL) end of life labels Mar 13, 2021
    @eryksun eryksun added the type-feature A feature request or enhancement label Mar 13, 2021
    @barneygale
    Copy link
    Mannequin

    barneygale mannequin commented May 15, 2021

    New PR up here: #26153

    The correspondence between pathlib and os.path is as follows:

    • Path.resolve() is roughly os.path.realpath()
    • Path.absolute() is roughly os.path.abspath()

    Differences:

    • resolve() always raises RuntimeError on symlink loops, whereas realpath() either raises OSError or nothing depending on strict.
    • absolute() doesn't normalize the path, whereas abspath() does. Normalizing without resolving symlinks can change the meaning of the path, so pathlib does the better thing here.

    @barneygale
    Copy link
    Mannequin

    barneygale mannequin commented Jun 12, 2021

    Hi - please could a core dev review PR 26153? It adds documentation and tests for Path.absolute(). Thank you!

    @brettcannon
    Copy link
    Member

    New changeset 18cb2ef by Barney Gale in branch 'main':
    bpo-29688: document and test pathlib.Path.absolute() (GH-26153)
    18cb2ef

    @barneygale
    Copy link
    Mannequin

    barneygale mannequin commented Jan 29, 2022

    Now that GH 26153 is merged, I think this bug can be resolved.

    @eryksun
    Copy link
    Contributor

    eryksun commented Jan 29, 2022

    In Windows, paths that are relative to the current directory on a drive aren't resolved. The following should be resolved by the current code:

        >>> os.chdir('C:/Temp')
        >>> pathlib.Path('C:').absolute()
        WindowsPath('C:')

    But _from_parts() has bugs with drive-relative paths.

    Assuming the bugs are fixed, when a path has a drive, Path.absolute() should resolve against abspath(self.drive) instead of getcwd().

    @brettcannon
    Copy link
    Member

    @eryksun I'm not seeing what's wrong with your example. Would you mind pointing out what you expect the result to be?

    And are you saying on Windows you have to resolve the drive separately from the working directory and then concatenate them?

    @eryksun
    Copy link
    Contributor

    eryksun commented Jan 31, 2022

    I'm not seeing what's wrong with your example.

    "C:" or "C:spam\\eggs" are not absolute paths. They depend on the effective working directory on the drive. An absolute path should never depend on a working directory, which can change at random.

    WinAPI SetEnvironmentVariableW() allows applications to set environment variables with names that begin with "=". These names are effectively reserved for special use by the OS, at least as documented. In particular, names of the form "=X:", where "X" is a drive letter, are used to store the working directory on a drive. The C runtime _[w]chdir() function sets these per-drive environment variables, as does Python's os.chdir(). As environment variables, they can be inherited by child processes.

    When then Windows API resolves a file path to access a file, or in GetFullPathNameW(), a drive-relative path such as "X:" or "X:spam\\eggs" is resolved against either the current working directory (if it's on the drive) or the value of the "=X:" environment variable for the drive. If the latter isn't defined, it defaults to the root directory, e.g. "X:\\". If the current working directory is on the drive, the system updates the value of the "=X:" environment variable, if it exists.

    on Windows you have to resolve the drive separately from the
    working directory and then concatenate them?

    No, if self.drive is defined, then abspath(self.drive) should be called instead of getcwd(). In Windows, ntpath.abspath() calls WinAPI GetFullPathNameW(), which resolves the working directory on the drive.

    @barneygale
    Copy link
    Mannequin

    barneygale mannequin commented Jan 31, 2022

    @eryksun thanks for flagging, a couple thoughts:

    I'd imagine that bug is reproducible with Path('C:\\Temp', 'C:') already, right? If that's the case, should it logged as a separate issue?

    I'm planning to /experimentally/ throw away pathlib's internal
    path parsing logic and defer to posixpath / ntpath instead. I suspect this bug and others will be fixed by that change, but I need to see what the performance impact will be.

    @eryksun
    Copy link
    Contributor

    eryksun commented Jan 31, 2022

    I'd imagine that bug is reproducible with Path('C:\\Temp', 'C:')
    already, right? If that's the case, should it logged as a
    separate issue?

    Yes, it's a separate issue that affects the _from_parts() call in absolute().

    How about designing absolute() to create a new instance from an absolute path that's created by os.path? For example:

    join(abspath(self.drive) if self.drive else getcwd(), self)
    

    Of course use accessor functions.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @barneygale
    Copy link
    Contributor

    barneygale commented Jan 8, 2023

    This feature request was implemented in #26153. The bug Eryk mentioned is logged as #94909 (fixed) and #100809 (PR available). Resolving this issue!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 (EOL) end of life 3.9 only security fixes 3.10 only security fixes docs Documentation in the Doc dir stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants