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 pathlib.chown method #64978

Closed
vajrasky mannequin opened this issue Feb 26, 2014 · 31 comments
Closed

Add pathlib.chown method #64978

vajrasky mannequin opened this issue Feb 26, 2014 · 31 comments
Labels
3.14 new features, bugs and security fixes stdlib Python modules in the Lib dir topic-pathlib type-feature A feature request or enhancement

Comments

@vajrasky
Copy link
Mannequin

vajrasky mannequin commented Feb 26, 2014

BPO 20779
Nosy @pitrou, @vajrasky, @sedrubal, @eamanu, @y0urself
PRs
  • gh-64978: Add chown() to pathlib.Path #31212
  • Files
  • add_chown_to_pathlib.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 = None
    closed_at = None
    created_at = <Date 2014-02-26.09:51:50.192>
    labels = ['type-feature', 'library', '3.11']
    title = 'Add pathlib.chown method'
    updated_at = <Date 2022-02-08.11:17:25.947>
    user = 'https://github.com/vajrasky'

    bugs.python.org fields:

    activity = <Date 2022-02-08.11:17:25.947>
    actor = 'y0urself'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2014-02-26.09:51:50.192>
    creator = 'vajrasky'
    dependencies = []
    files = ['34227']
    hgrepos = []
    issue_num = 20779
    keywords = ['patch']
    message_count = 7.0
    messages = ['212245', '212378', '212411', '212425', '396629', '398695', '412787']
    nosy_count = 8.0
    nosy_names = ['pitrou', 'cvrebert', 'neologix', 'vajrasky', 'sedrubal', 'eamanu', 'Tom Cook', 'y0urself']
    pr_nums = ['31212']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue20779'
    versions = ['Python 3.11']

    Linked PRs

    @vajrasky
    Copy link
    Mannequin Author

    vajrasky mannequin commented Feb 26, 2014

    For pragmatic and philosophical reasons, I would argue that we should add chown to pathlib library.

    @vajrasky vajrasky mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Feb 26, 2014
    @pitrou
    Copy link
    Member

    pitrou commented Feb 27, 2014

    I don't know about philosophical reasons ;-), but I don't think chown is very often used. Do you know of a context where it is?

    @vajrasky
    Copy link
    Mannequin Author

    vajrasky mannequin commented Feb 28, 2014

    Pragmatic reasons:

    I use chown moderately often. Usually as root, I want to change the uid of the files after generating a bunch of files. But I almost never changed gid.

    I am in two mind whether I should add lchown as well or not (just like lchmod) and make the signature of pathlib.chown same as os.chown (with third parameter, follow_symbolic_links).

    If you close this as won't fix, I will not hold grudge against you, though. ;)

    @pitrou
    Copy link
    Member

    pitrou commented Feb 28, 2014

    Well, I don't know yet, but if we ever want chown() in pathlib, I think it should be higher-level and accept symbolic uids as well.

    @TomCook
    Copy link
    Mannequin

    TomCook mannequin commented Jun 28, 2021

    +1 this.

    I have a program that opens a UNIX socket as root for other processes to communicate with it. I need to set the permissions to 0o775 and set the owner gid to a specific group so that members of that group can communicate with the process. It's annoying to have to drop back to os.chown(str(path), ...) for this.

    @eamanu
    Copy link
    Mannequin

    eamanu mannequin commented Aug 1, 2021

    Hello!

    I also use it frequently, but looking this issue I can agree with pitrou that there aren't lot of people that use chown.

    If I'm not wrong, there's a trend to move os to pathlib, perhaps chown should be in pathlib too :)

    I can work on this.

    @eamanu eamanu mannequin added the 3.11 only security fixes label Aug 1, 2021
    @y0urself
    Copy link
    Mannequin

    y0urself mannequin commented Feb 7, 2022

    I would love to use chown for a pathlib Path, too.

    It may not be used often, but if anyone want's to change all the os references from a file, it would be cool to have it on pathlib!

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

    I'm -1 on this: I don't think pathlib should start accumulating further functionality from os unless its used to implement high-level path behaviour (like walking directory trees).

    @CAM-Gerlach
    Copy link
    Member

    CAM-Gerlach commented Jan 8, 2023

    To note, I can't confirm myself since I'm on a Windows machine at the moment, but os.chown is documented as natively supporting pathlib.Paths since Python 3.6. There's also the higher level shutil.chown which has a nicer API with more Pythonic argument handling and accepts user/group names as well as IDs, unlike the os version. As it simply passes path through to the underlying os.chown, it in turn supports pathlib.Paths as well.

    This seems to address the major concrete issue expressed by users here (e.g. "It's annoying to have to drop back to os.chown(str(path), ...)", "I would love to use chown for a pathlib Path, too."), as well as exposing a nicer higher-level interface without the need to add yet another chown in pathlib.

    (Sidenote: Perhaps, if there's interest, it's worth allowing passing through dir_fd or particularly follow_symlinks in shutil.chown to os.chown so the latter is a proper superset of the former, but that's an entirely separate issue of course and unrelated to pathlib.)

    @AlexWaygood
    Copy link
    Member

    I agree with @barneygale, @CAM-Gerlach, and @brettcannon (Brett voiced his opinions on the PR here: #31212 (comment)). There seems to be little point adding this method, if it's just a thin wrapper around os.chown().

    @AlexWaygood AlexWaygood closed this as not planned Won't fix, can't repro, duplicate, stale Jan 8, 2023
    @anki-code
    Copy link

    It will be super cool to have this method because there is awesome python-powered xonsh shell and Path objects are using in xonsh broadly. You can do this in the shell using path-strings:

    pip install -U 'xonsh[full]'
    xonsh
    
    cd /tmp && echo world > hello.txt 
    
    p'hello.txt'
    # Path('hello')
    p'hello.txt'.read_text()
    # 'world\n'
    
    for f in p'/tmp'.glob('hello*'):
        print(f, f.exists())
    # /tmp/hello.txt True

    It will be cool to run p'hello.txt'.chown('root') instead of running subprocess chown root hello.txt (it's slower) or do import os etc.

    I'm for reopening this issue and favor for enriching pathlib.

    @CAM-Gerlach CAM-Gerlach added 3.12 bugs and security fixes and removed 3.11 only security fixes labels Mar 18, 2023
    @barneygale
    Copy link
    Contributor

    Re-opening the issue for the sake of discussion.

    I'm ±0 on this idea.

    On the one hand, I don't want pathlib to accumulate simple wrappers of os functions, because there's a large number of them and it's hard to draw the line. The Zen of Python says "There should be one-- and preferably only one --obvious way to do it."

    On the other hand, the implementation of chown() in shutil (not os) feels high-level enough to belong in pathlib, as it deals with user and group names like pathlib's owner() and group(). I have a vague plan to move shutil things into pathlib, so it fits reasonably well. In the very distant future we might consider deprecating the shutil version, which makes me feel better about the intervening duplication.

    Would love to hear thoughts from others!

    @barneygale barneygale reopened this Mar 18, 2023
    @anki-code
    Copy link

    anki-code commented Mar 18, 2023

    I prefer shutil.chown() vs os.chown() and when wrote about xonsh's use cases I mean shutil's version. So +1

    @CAM-Gerlach
    Copy link
    Member

    Personally, FWIW, I'm +0.5 so long as the idea fits with the long-term plan and the implementation does not add more than minimal additional maintenance burden. It's a little unfortunate there are two different chown functions with different APIs, but if this could eventually lead to deprecating one of them, leading to the same net number, while adding the convinence of calling methods directly on Paths rather than importing from os, then it seems reasonable to consider.

    @barneygale
    Copy link
    Contributor

    Thanks both.

    IMO we should move the implementation from shutil to pathlib. The version in shutil becomes a thin wrapper:

    # shutil.py
    
    def chown(path, user=None, group=None):
        pathlib.Path(path).chown(user, group)

    However, we'd need to add support for bytes paths to pathlib, as they're accepted by shutil.chown(). There's no feature request logged for that yet. It's not easy, but it's worth doing I think.

    @brettcannon
    Copy link
    Member

    we'd need to add support for bytes paths to pathlib, as they're accepted by shutil.chown(). There's no feature request logged for that yet. It's not easy, but it's worth doing I think.

    FYI I disagree with that as we want people to move towards string-based paths as they are much easier to work with (just like we want people to use UTF-8 more).

    @barneygale
    Copy link
    Contributor

    barneygale commented Mar 22, 2023

    Hm. I wouldn't want to encourage users to use bytes if they don't need to, or to put the thought in their minds. If we were to add it, it would only get a brief mention in the docs.

    But POSIX doesn't specify a particular character encoding for paths, and portable POSIX applications can't assume one. Pathnames are byte strings that may be in no particular encoding. If we can find a way to support this in pathlib without compromising the usability for people who don't care, I'd like to pursue it. Maybe.

    @AlexWaygood
    Copy link
    Member

    Hm. I wouldn't want to encourage users to use bytes if they don't need to, or to put the thought in their minds. If we were to add it, it would only get a brief mention in the docs.

    I view pathlib refusing to handle bytes as a feature. If I have a function that accepts an arbitrary Path object x, I know for sure that x.parts[0] will be a str. That's one of the things that makes the pathlib API easy to fit in my brain, and easy to reason about.

    Even if we can add support for bytes paths without complicating the implementation or slowing things down, I think I'd still be against adding support for bytes paths in pathlib 😕 it would raise backwards-compatibility concerns and increase the cognitive load associated with the module, and I'm unsure that there's a strong use case for it.

    @barneygale
    Copy link
    Contributor

    barneygale commented Mar 22, 2023

    I find that quite convincing! Thanks.

    It sounds like we can't implement shutil.chown() using pathlib.Path.chown() then, as we'd be breaking support for bytes. It probably makes sense to do the opposite: call shutil.chown() from pathlib. This will make it slightly more complicated to move other shutil functionality to pathlib in a future where AbstractPath is a thing, but that's a problem for another day.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 22, 2023

    If users were allowed to pass bytes to pathlib, they would have to be converted to str internally anyway. I'd rather people do their own conversion if they need a special one.

    @barneygale
    Copy link
    Contributor

    If users were allowed to pass bytes to pathlib, they would have to be converted to str internally anyway. I'd rather people do their own conversion if they need a special one.

    I don't follow, sorry - why would they have to be converted to str? AFAIK the only reason to use bytes paths is specifically to avoid decoding to str, because the encoding can't be known with certainty on POSIX.

    @barneygale
    Copy link
    Contributor

    @AlexWaygood
    Copy link
    Member

    AlexWaygood commented Mar 22, 2023

    Relevant SO: https://stackoverflow.com/questions/45724240/processing-non-utf-8-posix-filenames-using-python-pathlib

    (I mainly use Windows so apologies if this question exposes my ignorance about bytes paths!)

    Are there any drawbacks to the solution given in the answer to that Stack Overflow question? It looks pretty "clean" to me — if there are no drawbacks, maybe we should just document in the pathlib docs that this is how you can use pathlib if you have a bytes path?

    @barneygale
    Copy link
    Contributor

    I think what I'm missing is a proper understanding of surrogateescape encoding. I'll get back to you :-).

    @pitrou
    Copy link
    Member

    pitrou commented Mar 22, 2023

    PEP 383 is the fundamental reference here.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 22, 2023

    I don't follow, sorry - why would they have to be converted to str?

    Because pathlib uses str internally.

    barneygale added a commit to barneygale/cpython that referenced this issue May 4, 2023
    Move implementation from `shutil.chown()`. This function now calls through
    to pathlib.
    barneygale added a commit to barneygale/cpython that referenced this issue May 4, 2023
    Move implementation from `shutil.chown()`. This function now calls through
    to pathlib.
    barneygale added a commit to barneygale/cpython that referenced this issue May 4, 2023
    Move implementation from `shutil.chown()`. This function now calls through
    to pathlib.
    @barneygale
    Copy link
    Contributor

    PR available:

    barneygale added a commit to barneygale/cpython that referenced this issue May 7, 2023
    @erlend-aasland erlend-aasland added 3.13 bugs and security fixes and removed 3.12 bugs and security fixes labels Jan 5, 2024
    @barneygale barneygale added 3.14 new features, bugs and security fixes and removed 3.13 bugs and security fixes labels May 8, 2024
    @barneygale barneygale removed their assignment May 11, 2024
    @nineteendo
    Copy link
    Contributor

    Barney, what's the status here?

    @barneygale
    Copy link
    Contributor

    I'm still undecided as to whether this is a good idea.

    IIUC we can't make shutil use pathlib because pathlib strips trailing slashes, and so shutil.chown('not_a_dir/', ...) would succeed where previously it failed with NotADirectoryError. This is fixable if we introduce a private superclass of Path that skips normalization, but there's little appetite for that right now.

    Consequently we'd either need to make pathlib use shutil, which feels wrong to some core devs and I'm trying to avoid if I can, or we'd just copy-paste the shutil implementation into pathlib, which I feel pretty "meh" about too.

    @barneygale
    Copy link
    Contributor

    #73991 is the more important shutil/pathlib issue. I hope to solve that first, and by doing so, shed some light on whether this request is worth doing or not.

    @barneygale
    Copy link
    Contributor

    I'm going to close this as "won't do". It turns out that Path.chown() won't be helpful for copying file metadata in #73991, and I can see no other compelling reason to add it. Plus it's POSIX-only and usually needs root. Use os.chown() or shutil.chown() instead please! And feel free to leave a comment if you disagree and would like to see this re-opened.

    @barneygale barneygale closed this as not planned Won't fix, can't repro, duplicate, stale Jul 3, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.14 new features, bugs and security fixes stdlib Python modules in the Lib dir topic-pathlib type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants