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

Returning Windows file attribute information via os.stat() #65918

Closed
benhoyt mannequin opened this issue Jun 11, 2014 · 17 comments
Closed

Returning Windows file attribute information via os.stat() #65918

benhoyt mannequin opened this issue Jun 11, 2014 · 17 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@benhoyt
Copy link
Mannequin

benhoyt mannequin commented Jun 11, 2014

BPO 21719
Nosy @loewis, @vstinner, @benhoyt, @ethanfurman, @JimJJewett, @zware
Files
  • issue21719.patch
  • issue21719-2.patch
  • issue21719-3.patch
  • issue21719-4.patch
  • 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 = 'https://github.com/zware'
    closed_at = <Date 2014-06-19.15:02:20.309>
    created_at = <Date 2014-06-11.13:22:41.541>
    labels = ['type-feature', 'library']
    title = 'Returning Windows file attribute information via os.stat()'
    updated_at = <Date 2014-06-20.09:18:58.030>
    user = 'https://github.com/benhoyt'

    bugs.python.org fields:

    activity = <Date 2014-06-20.09:18:58.030>
    actor = 'vstinner'
    assignee = 'zach.ware'
    closed = True
    closed_date = <Date 2014-06-19.15:02:20.309>
    closer = 'zach.ware'
    components = ['Library (Lib)']
    creation = <Date 2014-06-11.13:22:41.541>
    creator = 'benhoyt'
    dependencies = []
    files = ['35615', '35621', '35622', '35632']
    hgrepos = []
    issue_num = 21719
    keywords = ['patch']
    message_count = 17.0
    messages = ['220266', '220272', '220274', '220275', '220416', '220439', '220464', '220467', '220497', '220498', '220499', '220563', '220989', '220990', '221062', '221064', '221081']
    nosy_count = 6.0
    nosy_names = ['loewis', 'vstinner', 'benhoyt', 'ethan.furman', 'Jim.Jewett', 'zach.ware']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue21719'
    versions = ['Python 3.5']

    @benhoyt
    Copy link
    Mannequin Author

    benhoyt mannequin commented Jun 11, 2014

    I asked recently on python-dev [1] 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 [2].

    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,
    [3].

    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 [4] [5] [6]), 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. :-)

    [1] https://mail.python.org/pipermail/python-dev/2014-June/134990.html
    [2] https://github.com/python/cpython/blob/master/Modules/posixmodule.c#L1462
    [3] http://stackoverflow.com/a/6365265
    [4] https://mail.python.org/pipermail/python-dev/2014-June/134993.html
    [5] https://mail.python.org/pipermail/python-dev/2014-June/135006.html
    [6] https://mail.python.org/pipermail/python-dev/2014-June/135007.html

    @benhoyt benhoyt mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jun 11, 2014
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jun 11, 2014

    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).

    @benhoyt
    Copy link
    Mannequin Author

    benhoyt mannequin commented Jun 11, 2014

    Fair call -- "st_file_attributes" sounds good to me, and matches the prefix of the FILE_ATTRIBUTES_* constants better too.

    @vstinner
    Copy link
    Member

    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.

    @benhoyt
    Copy link
    Mannequin Author

    benhoyt mannequin commented Jun 13, 2014

    I've got a patch for this. Need to finish the docs and add tests, and then I'll post here.

    @benhoyt
    Copy link
    Mannequin Author

    benhoyt mannequin commented Jun 13, 2014

    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

    @jimjjewett
    Copy link
    Mannequin

    jimjjewett mannequin commented Jun 13, 2014

    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.

    @zware
    Copy link
    Member

    zware commented Jun 13, 2014

    Not sure what you were going for on the version, Jim; you added 3.4, but this is a new feature for 3.5.

    @benhoyt
    Copy link
    Mannequin Author

    benhoyt mannequin commented Jun 13, 2014

    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.

    @zware
    Copy link
    Member

    zware commented Jun 13, 2014

    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).

    @benhoyt
    Copy link
    Mannequin Author

    benhoyt mannequin commented Jun 13, 2014

    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.

    @benhoyt
    Copy link
    Mannequin Author

    benhoyt mannequin commented Jun 14, 2014

    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

    @zware
    Copy link
    Member

    zware commented Jun 19, 2014

    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!

    @zware zware closed this as completed Jun 19, 2014
    @zware zware self-assigned this Jun 19, 2014
    @benhoyt
    Copy link
    Mannequin Author

    benhoyt mannequin commented Jun 19, 2014

    Great, thanks for committing!

    @benhoyt
    Copy link
    Mannequin Author

    benhoyt mannequin commented Jun 20, 2014

    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.

    @zware
    Copy link
    Member

    zware commented Jun 20, 2014

    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.

    @vstinner
    Copy link
    Member

    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 bpo-21813 to enhance the documentation of os.stat_result.

    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants