Created on 2014-06-11 13:22 by benhoyt, last changed 2014-06-20 09:18 by haypo. This issue is now closed.
|issue21719.patch||benhoyt, 2014-06-13 13:19||review|
|issue21719-2.patch||benhoyt, 2014-06-13 20:36||review|
|issue21719-3.patch||benhoyt, 2014-06-13 20:52||review|
|issue21719-4.patch||benhoyt, 2014-06-14 15:12||review|
|msg220266 - (view)||Author: Ben Hoyt (benhoyt) *||Date: 2014-06-11 13:22|
I asked recently on python-dev  about adding a "st_winattrs" attribute to stat result objects on Windows, to return the full set of Windows file attribute bits, such as "hidden" or "compressed" status. Copying from that thread for a bit more context here: Python's os.stat() simply discards most of the file attribute information fetched via the Win32 system calls. On Windows, os.stat() calls CreateFile to open the file and get the dwFileAttributes value, but it throws it all away except the FILE_ATTRIBUTE_DIRECTORY and FILE_ATTRIBUTE_READONLY bits. See CPython source at . Given that os.stat() returns extended, platform-specific file attributes on Linux and OS X platforms (for example, st_blocks, st_rsize, etc), it seems that Windows is something of a second-class citizen here. There are several questions on StackOverflow about how to get this information on Windows, and one has to resort to ctypes. For example, . To solve this problem, I think we should add a "st_winattrs" attribute to the object returned by os.stat() on Windows. And we should add the relevant Win32 FILE_ATTRIBUTE_* constants to the "stat" module. Then, similarly to existing code like hasattr(st, 'st_blocks') on Linux, you could write a cross-platform function to determine if a file was hidden, something like so: def is_hidden(path): if startswith(os.path.basename(path), '.'): return True st = os.stat(path) return getattr(st, 'st_winattrs', 0) & FILE_ATTRIBUTE_HIDDEN != 0 There was general support for this idea on python-dev (see   ), so I'd love to see this in Python 3.5. Basically we need to add a "st_winattrs" integer attribute to the win32_stat struct in posixmodule.c and add the FILE_ATTRIBUTE_* constants to _stat.c, as well as adding Windows-specific tests and documentation. I've never compiled CPython on Windows, but I aim to provide a patch for this at some stage soonish. Feedback and other patches welcome in the meantime. :-)  https://mail.python.org/pipermail/python-dev/2014-June/134990.html  https://github.com/python/cpython/blob/master/Modules/posixmodule.c#L1462  http://stackoverflow.com/a/6365265  https://mail.python.org/pipermail/python-dev/2014-June/134993.html  https://mail.python.org/pipermail/python-dev/2014-June/135006.html  https://mail.python.org/pipermail/python-dev/2014-June/135007.html
|msg220272 - (view)||Author: Martin v. Löwis (loewis) *||Date: 2014-06-11 14:18|
Instead of the somewhat cryptic name "winattrs", I suggest to call it st_file_attributes, as it is called in the Windows API (actually, GetFileAttributesEx calls it dwFileAttributes, but st_file_attributes could be considered a Pythonic spelling of that).
|msg220274 - (view)||Author: Ben Hoyt (benhoyt) *||Date: 2014-06-11 14:20|
Fair call -- "st_file_attributes" sounds good to me, and matches the prefix of the FILE_ATTRIBUTES_* constants better too.
|msg220275 - (view)||Author: STINNER Victor (haypo) *||Date: 2014-06-11 14:35|
> Instead of the somewhat cryptic name "winattrs", I suggest to call it st_file_attributes (...) On Windows, os.stat() calls GetFileInformationByHandle() which fills a BY_HANDLE_FILE_INFORMATION structure, and this structure has a dwFileAttributes attribute. The os.stat() calls also GetFileType(). http://msdn.microsoft.com/en-us/library/windows/desktop/aa364952%28v=vs.85%29.aspx So I agree that os.stat().st_file_attributes is less surprising for Windows developers and it is more homogenous with FILE_ATTRIBUTE_xxx constants.
|msg220416 - (view)||Author: Ben Hoyt (benhoyt) *||Date: 2014-06-13 03:47|
I've got a patch for this. Need to finish the docs and add tests, and then I'll post here.
|msg220439 - (view)||Author: Ben Hoyt (benhoyt) *||Date: 2014-06-13 13:19|
Full patch to add this to Python 3.5 attached: * code changes to posixmodule.c and _stat.c * tests added in test_os.py and test_stat.py * docs added to os.rst and stat.rst
|msg220464 - (view)||Author: Jim Jewett (Jim.Jewett)||Date: 2014-06-13 16:22|
Added version 3.5; is 3.4 somehow still correct? Set stage to patch review as there are still comments to deal with, but it looks pretty close to commit review.
|msg220467 - (view)||Author: Zachary Ware (zach.ware) *||Date: 2014-06-13 16:58|
Not sure what you were going for on the version, Jim; you added 3.4, but this is a new feature for 3.5.
|msg220497 - (view)||Author: Ben Hoyt (benhoyt) *||Date: 2014-06-13 20:36|
Updated patch attached based on code reviews. I'm replying to the code review here as when I tried to reply on bugs.python.org/review I got a Python exception, "AttributeError: NoneType has no attribute something or other". FYI, it seems Django is set up in debug mode, because it was the full colourful traceback with local vars etc. I didn't save this file, sorry! Zach Ware ========= > Either way, these should probably be defined in stat.py as well (sorry; I should > have said 'as well as' instead of 'instead of' when I mentioned _stat.c in the > email thread). I don't think so -- I thought the purpose of moving them to _stat.c was so you could use the windows.h constants rather than hard-coded integers? But if they're in stat.py, why do they need to be in _stat.c as well? http://bugs.python.org/review/21719/diff/12149/Modules/_stat.c#newcode563 > Modules/_stat.c:563: // For some reason FILE_ATTRIBUTE_INTEGRITY_STREAM and > PEP 7 bans C++ style comments. Good point. stoneleaf ========= > 'covert' should be 'convert' - can you fix this as part of your patch? ;) Fixed in latest patch. > Instead of spelling out FILE_ATTRIBUTE_ each time, can we abbreviate that part > to FA_ ? I disagree. I realize FILE_ATTRIBUTE_* is longish, but it's what it's called in the Windows API, so will make these constants much more findable and obvious to Windows programmers (consistent with the Win32 documentation). > On non-Windows machines 'st_file_attributes' will be 0? No, it won't be present at all (like the other non-cross-platform attributes). > There's no attribute for the value 8? Correct -- see the Win32 API documentation and loewis's comment. haypo ===== > It would be nice to document these options here :) I don't think we should duplicate the Win32 documentation. Ours will just be less thorough and potentially get out of date. I agree we should add a link to the Win32 docs though. > Instead of == 0, you can use assertFalse. Could, but I don't like this when I'm doing bit/integer comparisons. Explicit is better. > The purpose of the _stat module is to only provide data from the OS, to not > hardcode constants. So -1 to provide these constants on all platforms. Agreed. See above. > If the constant is only available on new Visual Studio version, we may hardcode > the constant to avoid the dependency on a specific Visual Studio version. For > example, in unicodeobject.c I used: > > #ifndef WC_ERR_INVALID_CHARS > # define WC_ERR_INVALID_CHARS 0x0080 > #endif That's a good way of doing it. I'll use this approach. > Modules/posixmodule.c:1437: unsigned long st_file_attributes; > Should we use a DWORD here? I don't think so. None of the other values in win32_stat (e.g., st_dev) are defined using DWORD or Win32-style types. See also loewis's comment. loewis ====== > We shouldn't replicate Microsoft's documentation (if for no other reason than > copyright). Instead, linking to > > http://msdn.microsoft.com/en-us/library/windows/desktop/gg258117.aspx > > might be helpful (as long as Microsoft preserves this URL). Agreed. I'll make this change.
|msg220498 - (view)||Author: Zachary Ware (zach.ware) *||Date: 2014-06-13 20:44|
Ben Hoyt wrote: > I'm replying to the code review here as when I tried to reply on > bugs.python.org/review I got a Python exception, "AttributeError: > NoneType has no attribute something or other". Generally when you get that you can just hit back and try again, it will work by the 4th try ;) > I don't think so -- I thought the purpose of moving them to _stat.c > was so you could use the windows.h constants rather than hard-coded > integers? But if they're in stat.py, why do they need to be in > _stat.c as well? The idea is that _stat.c will use system-provided values wherever possible, but stat.py should be as accurate as we can make it to provide a backup for when _stat isn't around (either when it's just not built, which isn't an issue on Windows, or in another Python implementation).
|msg220499 - (view)||Author: Ben Hoyt (benhoyt) *||Date: 2014-06-13 20:52|
> The idea is that _stat.c will use system-provided values wherever > possible, but stat.py should be as accurate as we can make it to > provide a backup for when _stat isn't around (either when it's just > not built, which isn't an issue on Windows, or in another Python > implementation). Fair call. I've added the constants to stat.py as well. Updated patch attached.
|msg220563 - (view)||Author: Ben Hoyt (benhoyt) *||Date: 2014-06-14 15:12|
Uploading a (hopefully final! :-) patch to fix Zach Ware's points from the code review: 1) use stat.FILE_ATTRIBUTE_DIRECTORY constant in test_os.py 2) break line length in stat.rst doc source
|msg220989 - (view)||Author: Zachary Ware (zach.ware) *||Date: 2014-06-19 15:02|
Committed as 706fab0213db (with the wrong issue number), with just a couple of comment tweaks (mostly to shorten a couple more lines) and some committer drudge-work. Thanks for your contribution, Ben!
|msg220990 - (view)||Author: Ben Hoyt (benhoyt) *||Date: 2014-06-19 15:06|
Great, thanks for committing!
|msg221062 - (view)||Author: Ben Hoyt (benhoyt) *||Date: 2014-06-20 02:14|
BTW, thanks for the mention in "What's New in Python 3.5": https://docs.python.org/3.5/whatsnew/3.5.html#os Can I make one small suggestion for a tweak there? A link to the docs for os.stat() would be good. So maybe instead of mentioning "os.stat_result" -- which isn't really a documented class -- say "The return value of :func:`os.stat` now has..." where os.stat is a link to https://docs.python.org/3.5/library/os.html#os.stat with the documentation for that.
|msg221064 - (view)||Author: Zachary Ware (zach.ware) *||Date: 2014-06-20 03:17|
If you want to make a patch to that effect, I'll commit it. At this point though, the whatsnew doc is just a stub and the whole document will be beaten into better shape closer to release time.
|msg221081 - (view)||Author: STINNER Victor (haypo) *||Date: 2014-06-20 09:18|
> Can I make one small suggestion for a tweak there? A link to the docs for os.stat() would be good. I created the issue #21813 to enhance the documentation of os.stat_result.
|2014-06-20 09:18:58||haypo||set||messages: + msg221081|
|2014-06-20 03:17:54||zach.ware||set||messages: + msg221064|
|2014-06-20 02:14:59||benhoyt||set||messages: + msg221062|
|2014-06-19 15:06:11||benhoyt||set||messages: + msg220990|
|2014-06-19 15:02:20||zach.ware||set||status: open -> closed|
messages: + msg220989
stage: patch review -> resolved
messages: + msg220563
messages: + msg220499
|2014-06-13 20:44:23||zach.ware||set||messages: + msg220498|
messages: + msg220497
versions: - Python 3.4
+ Python 3.4|
nosy: + Jim.Jewett
messages: + msg220464
stage: patch review
keywords: + patch
messages: + msg220439
|2014-06-13 03:47:51||benhoyt||set||messages: + msg220416|
messages: + msg220275
|2014-06-11 14:20:02||benhoyt||set||messages: + msg220274|
messages: + msg220272