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

allow shutil.disk_usage to take a file path on Windows also #76738

Closed
eryksun opened this issue Jan 15, 2018 · 11 comments
Closed

allow shutil.disk_usage to take a file path on Windows also #76738

eryksun opened this issue Jan 15, 2018 · 11 comments
Labels
3.8 only security fixes OS-windows stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@eryksun
Copy link
Contributor

eryksun commented Jan 15, 2018

BPO 32557
Nosy @terryjreedy, @pfmoore, @vstinner, @giampaolo, @tjguk, @zware, @eryksun, @zooba, @miss-islington, @jopamer
PRs
  • bpo-32557: allow shutil.disk_usage to take a file path on Windows also #9372
  • 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 = <Date 2018-09-25.15:25:35.556>
    created_at = <Date 2018-01-15.15:26:52.477>
    labels = ['3.8', 'type-feature', 'library', 'OS-windows']
    title = 'allow shutil.disk_usage to take a file path on Windows also'
    updated_at = <Date 2018-12-11.10:48:12.254>
    user = 'https://github.com/eryksun'

    bugs.python.org fields:

    activity = <Date 2018-12-11.10:48:12.254>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-09-25.15:25:35.556>
    closer = 'steve.dower'
    components = ['Library (Lib)', 'Windows']
    creation = <Date 2018-01-15.15:26:52.477>
    creator = 'eryksun'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32557
    keywords = ['patch']
    message_count = 11.0
    messages = ['309992', '310286', '319371', '325585', '325613', '325655', '325787', '325818', '325906', '326359', '331603']
    nosy_count = 10.0
    nosy_names = ['terry.reedy', 'paul.moore', 'vstinner', 'giampaolo.rodola', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'miss-islington', 'jopamer']
    pr_nums = ['9372']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue32557'
    versions = ['Python 3.8']

    @eryksun
    Copy link
    Contributor Author

    eryksun commented Jan 15, 2018

    bpo-26330 was resolved by documenting that shutil.disk_usage requires a directory. However, the shutil module is in a position to harmonize cross-platform behavior in ways that aren't normally possible or recommended in the low-level os module. To that end, the Windows implementation could retry calling nt._getdiskusage on the resolved parent directory in case of NotADirectoryError. For example:

        def disk_usage(path):
            try:
                total, free = nt._getdiskusage(path)
            except NotADirectoryError:
                path = os.path.dirname(nt._getfinalpathname(path))
                total, free = nt._getdiskusage(path)
            used = total - free
            return _ntuple_diskusage(total, used, free)

    Alternatively, this could be addressed in the implementation of nt._getdiskusage itself.

    @eryksun eryksun added 3.8 only security fixes stdlib Python modules in the Lib dir OS-windows type-feature A feature request or enhancement labels Jan 15, 2018
    @terryjreedy
    Copy link
    Member

    Seems sensible to me.

    @terryjreedy terryjreedy changed the title allow shutil.disk_usage to take a file path allow shutil.disk_usage to take a file path on Windows also Jan 19, 2018
    @giampaolo
    Copy link
    Contributor

    +1

    @jopamer
    Copy link
    Mannequin

    jopamer mannequin commented Sep 17, 2018

    Hi!

    I decided to try fixing this one as a way to get acquainted with the code base. I went ahead and updated the backing NT C function, but please let me know if you'd prefer I update disk_usage as proposed.

    Thanks!

    @zooba
    Copy link
    Member

    zooba commented Sep 18, 2018

    I took a quick look at the patch and the main issue I see is the use of the MAX_PATH constant. We encourage people to disable this limit where possible, so using dynamic allocation where we need to reduce the path will be a better way to avoid this function breaking.

    There should be other examples of this pattern in the file, but I don't think it'll matter here if we always copy the string. Better to avoid it, of course, but that complicates things.

    @jopamer
    Copy link
    Mannequin

    jopamer mannequin commented Sep 18, 2018

    Got it - thanks! That sounds good to me, so I'll take a look at how other functions are working around MAX_PATH and update the diff to also avoid the copy when possible.

    @jopamer
    Copy link
    Mannequin

    jopamer mannequin commented Sep 19, 2018

    Just to loop back, I updated the PR to avoid MAX_PATH and only allocate in the "not a directory" case. Thanks for getting back to me so quickly!

    One question, though, is that it *does* seem like MAX_PATH is still referenced in several places in posixmodule.c. Is the ultimate goal to remove those references as well?

    @zooba
    Copy link
    Member

    zooba commented Sep 19, 2018

    (Excuse the GitHub syntax - was about to post it there, but it got long enough to belong here)

    Regarding the _dirnameW discussion, fixing _dirname would be ideal, but that is bloating out your PR quite a bit :)

    The "right" function to use there is PathCchRemoveFileSpec, which unfortunately does not exist on Windows 7.

    The fallback is PathRemoveFileSpec which only supports up to MAX_PATH (but may do something relatively sensible for longer paths, such as trimming out the last directory separator before that limit - I haven't tested it).

    Supporting both is tricky - there's a similar example in [PC/getpathp.c](https://github.com/python/cpython/blob/main/PC/getpathp.c) for PathCchCombineEx that could be replicated for dirname. But I'd be okay with taking this PR without that fix and filing a new bug for _dirnameW.

    @jopamer
    Copy link
    Mannequin

    jopamer mannequin commented Sep 20, 2018

    Awesome - thanks, Steve - this is all super helpful! If you're cool with it I'd like to stick to using _dirnameW for now, and then follow up with another set of PRs for the fixes you've recommended.

    @miss-islington
    Copy link
    Contributor

    New changeset c8c0249 by Miss Islington (bot) (Joe Pamer) in branch 'master':
    bpo-32557: allow shutil.disk_usage to take a file path on Windows also (GH-9372)
    c8c0249

    @zooba zooba closed this as completed Sep 25, 2018
    @vstinner
    Copy link
    Member

    The new test is unstable: see bpo-35458.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes OS-windows stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants