-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
Comments
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. |
Seems sensible to me. |
+1 |
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! |
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. |
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. |
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? |
(Excuse the GitHub syntax - was about to post it there, but it got long enough to belong here) Regarding the 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 Supporting both is tricky - there's a similar example in |
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. |
The new test is unstable: see bpo-35458. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: