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 shutil.chown to allow to use user and group name (and not only uid/gid) #56400

Closed
sandrotosi opened this issue May 26, 2011 · 30 comments
Closed
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@sandrotosi
Copy link
Contributor

BPO 12191
Nosy @rhettinger, @gpshead, @ncoghlan, @pitrou, @vstinner, @ericvsmith, @giampaolo, @ezio-melotti, @merwok, @bitdancer, @sandrotosi, @akheron
Files
  • shutil_chown-default.patch
  • shutil_chown-default-v2.patch
  • shutil_chown-default-v3.patch
  • shutil_chown-default-v4.patch
  • shutil_chown-default-v5.patch
  • shutil_chown-default-v6.patch
  • shutil_chown-default-v7.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/sandrotosi'
    closed_at = <Date 2011-08-22.21:30:00.982>
    created_at = <Date 2011-05-26.20:14:09.878>
    labels = ['type-feature', 'library']
    title = 'Add shutil.chown to allow to use user and group name (and not only uid/gid)'
    updated_at = <Date 2011-09-02.16:15:08.069>
    user = 'https://github.com/sandrotosi'

    bugs.python.org fields:

    activity = <Date 2011-09-02.16:15:08.069>
    actor = 'eric.araujo'
    assignee = 'sandro.tosi'
    closed = True
    closed_date = <Date 2011-08-22.21:30:00.982>
    closer = 'sandro.tosi'
    components = ['Library (Lib)']
    creation = <Date 2011-05-26.20:14:09.878>
    creator = 'sandro.tosi'
    dependencies = []
    files = ['22132', '22200', '22372', '22540', '22841', '22864', '22949']
    hgrepos = []
    issue_num = 12191
    keywords = ['patch']
    message_count = 30.0
    messages = ['137004', '137008', '137011', '137084', '137174', '137195', '137320', '137452', '138388', '138671', '138904', '139603', '140491', '141665', '141686', '141699', '141743', '141749', '141753', '141784', '141792', '142480', '142751', '142752', '142753', '142754', '142757', '142764', '142770', '143411']
    nosy_count = 14.0
    nosy_names = ['rhettinger', 'gregory.p.smith', 'ncoghlan', 'pitrou', 'vstinner', 'eric.smith', 'giampaolo.rodola', 'ezio.melotti', 'eric.araujo', 'r.david.murray', 'sandro.tosi', 'neologix', 'python-dev', 'petri.lehtinen']
    pr_nums = []
    priority = 'low'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue12191'
    versions = ['Python 3.3']

    @sandrotosi
    Copy link
    Contributor Author

    Hi, following up the discussion on http://mail.python.org/pipermail/python-dev/2011-May/111642.html here'a patch to introduce shutil.chown() along with doc, tests and a change in os doc to link to the new function (I also took the occasion to rewrap os.chown() doc text). As of now only the one-file function is implemented, let's see at a later time what's about for a recursive one.

    The patch was prepared against default, but it probably makes sense to backport it to 2.7?

    Any comment/suggestion is welcome!

    @sandrotosi sandrotosi self-assigned this May 26, 2011
    @sandrotosi sandrotosi added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels May 26, 2011
    @ericvsmith
    Copy link
    Member

    As a new feature, it can't be added to 2.7.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 26, 2011

    I didn't test, but after skimming through the code I think that if an invalid user name/group name is provided, os.chown is passed None, which will fail with ValueError.
    It would be better to raise an explicit error like "no such user" / "no such group".
    That's something you could add to the tests.

    Also, you could initialize _user to -1 instead of None, and replace
    if user is None:
    _user = -1
    else:
    [...]
    with
    if user is not None:
    [...]

    Same goes for _group.

    @merwok
    Copy link
    Member

    merwok commented May 27, 2011

    As a general rule, rewrapping is not done in patches, it can make review less easy. The committer can do it, sometimes in a second commit.

    I commented on the review page.

    @merwok merwok changed the title Shutil - add chown() in order to allow to use user and group name (and not only uid/gid) Add shutil.chown to allow to use user and group name (and not only uid/gid) May 27, 2011
    @gpshead
    Copy link
    Member

    gpshead commented May 29, 2011

    I added comments in the code review.

    this patch is looking good once the comments are addressed. thanks for your contribution!

    As for talk of support for recursion... thats what os.walk() is for. it doesn't belong as part of any particular individual function itself.

    @merwok
    Copy link
    Member

    merwok commented May 29, 2011

    The python-dev discussion talked about chowntree vs. os.walk:
    http://mail.python.org/pipermail/python-dev/2011-May/111667.html
    http://mail.python.org/pipermail/python-dev/2011-May/111673.html
    http://mail.python.org/pipermail/python-dev/2011-May/111674.html

    I find those arguments of convenience and precedent convincing.

    @sandrotosi
    Copy link
    Contributor Author

    @eric: ok, I was just asking given the "unusual" situation 2.7 is (i.e. a very long support series), but it's perfectly fine not to backport the feature.

    @Charles-François: I changed a bit the logic: I check for 'None' at first, since it signals that we don't want to change the uid/gid, and then I recheck for None after _get_uid/gid, and only there raise a ValueError (is there a better exception for cases like this?) - what do you think about it?

    @Éric: re rewrap: I kinda find that almost every core dev has his opinion on this :) some don't want to rewrap, others are fine if you're touching that paragraph, and so on. Anyhow, to be on the safe-side, I un-rewrapped the first paragraph of os.chown().

    @gregory: I'd like to first concentrate on the "single path" function, then we'll look at the "tree" version.

    Thanks a lot to everyone for the review/comment/inspiration: attached you can find the updated patch. If something still needs a fix, I'd be happy to work on it.

    @merwok
    Copy link
    Member

    merwok commented Jun 1, 2011

    I was just asking given the "unusual" situation 2.7 is (i.e. a very
    long support series)
    Support means bug fixes. Long-term means that the bugfix period (before going into security mode) is extended, not that it can get new features: that’s exclusively for 3.3.

    rewrap: I kinda find that almost every core dev has his opinion on this :)
    Heh :)

    @sandrotosi
    Copy link
    Contributor Author

    Here's the updated patch following review on Rietveld

    @vstinner
    Copy link
    Member

    shutil_chown-default-v3.patch:

    • os.chown() is only available on Unix, shutil.chown() should not be defined if os.chown() doesn't exist (require to add a @unittest.skipUnless decorator to the test)
    • tests: is it possible that shutil.chown() exists whereas pwd or grp module is missing? if yes, you should split the test into two parts. it looks like pwd and grp are avaiable on Unix.
    • os.chown(path, -1, -1) is accepted, why not accepting shutil.chown(path) (shutil.chown(path, None, None))?
    • style: i don't like _user name, i suggest uid and gid (or user_id and group_id) instead of _user and _group... or you can reuse user/group variables
    • style: "else: \n if not isinstance(user, int):" can be written "elif not isinstance(user, int):" to avoid an useless level of indentation
    • tests: you may only call os.getuid() and os.getgid() once

    @merwok
    Copy link
    Member

    merwok commented Jun 24, 2011

    haypo, you missed this comment from gps on Rietveld:

    As for the _user = user stuff, the reason that is being done is so
    that the original user and or group value can be included in the
    raised exception error message. That is actually a nice thing to do.

    otherwise, yes, i'd normally just reassign the parameters.

    At this point I believe this patch is ready to commit. I'd reword
    the docstring to make it simpler but that is a minor detail and
    something that is easy to change in a future commit.

    I agree the patch is ready.

    @sandrotosi
    Copy link
    Contributor Author

    I've applied some of the suggestions Victor made (and also refresh to be cleanly applicable on default).

    @sandrotosi
    Copy link
    Contributor Author

    Sorry for the late reply: I've applied Éric comments made on Rietveld.

    @merwok
    Copy link
    Member

    merwok commented Aug 5, 2011

    You should replace the v5 file (or even remove all files, for clarity) with the actual output of hg diff, not hg status ;-)

    @sandrotosi
    Copy link
    Contributor Author

    Gaaah, sorry about that: I've just uploaded the correct fifth version.

    @pitrou
    Copy link
    Member

    pitrou commented Aug 6, 2011

    I wonder if we should raise LookupError for unknown uids/gids. Do we have other APIs with similar semantics?

    @sandrotosi
    Copy link
    Contributor Author

    grepping over the stdlib, LookupError is only used in codecs exception; also the documentation of LE is "The base class for the exceptions that are raised when a key or index used on a mapping or sequence is invalid: IndexError, KeyError. This can be raised directly by codecs.lookup()." so I guess it's not the right exception of this case.

    @bitdancer
    Copy link
    Member

    Well, you could consider codecs an example in the stdlib of using it, and the doc could be changed to something like "stdlib modules that look up information in specialized data sources may raise this error directly". Whether this is a good idea or not, I'm not sure, but it feels more logical than ValueError.

    @sandrotosi
    Copy link
    Contributor Author

    On Mon, Aug 8, 2011 at 00:12, R. David Murray <report@bugs.python.org> wrote:

    R. David Murray <rdmurray@bitdance.com> added the comment:

    Well, you could consider codecs an example in the stdlib of using it, and the doc could be changed to something like "stdlib modules that look up information in specialized data sources may raise this error directly".  Whether this is a good idea or not, I'm not sure, but it feels more logical than ValueError.

    Sure, I was just showing my line of reasoning :) I'm ok either way: if
    you feel LookupError is best, I'll change shutil.chown() code and also
    the doc for LE (about that, is it ok a single patch or 2 separate?)

    @merwok
    Copy link
    Member

    merwok commented Aug 8, 2011

    LookupError sounds good. About the latest patch: I wonder if using !a instead of !r in the format strings for exceptions would be more helpful (maybe you’ve seen a recent python-dev subthread about that). I don’t like seeing escapes for perfectly common characters like ß or é, but OTOH escapes help disambiguating different characters that look the same. Apart from these two points, this is good to go.

    A doc addition for LookupError would be an independent changeset; it has nothing to do with adding shutil.chown.

    @sandrotosi
    Copy link
    Contributor Author

    after a review from Ezio (thanks!) we've come out with this updated patch; main changes are in the test suite, where now it's checked that chown() succeed. about !r/!a I've left !r; and changed the 2 ValueError in LookupError (the first, in case no arguments are passed, it's still there).

    @sandrotosi
    Copy link
    Contributor Author

    version 7 here we come :) I've addressed Eric's commenta on rietveld (all but one, I need input from him) and also updated the tests for new helper functions.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 22, 2011

    New changeset d1fd0f0f8e68 by Sandro Tosi in branch 'default':
    bpo-12191: add shutil.chown() to change user and/or group owner of a given path also specifying their names.
    http://hg.python.org/cpython/rev/d1fd0f0f8e68

    @sandrotosi
    Copy link
    Contributor Author

    At last, it's in :) thanks a lot to all the people that helped me in the process!

    @vstinner
    Copy link
    Member

    New changeset d1fd0f0f8e68 by Sandro Tosi in branch 'default'

    You may add shutil.chmod to the "What's New in Python 3.3?" document.

    @ezio-melotti
    Copy link
    Member

    Raymond, are you still taking care of the whatsnew?
    Do you want people to update it when they add something new?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 22, 2011

    New changeset 5d317e38da44 by Sandro Tosi in branch 'default':
    bpo-12191: fix build failures, explicitly passing group argument when I want to test it
    http://hg.python.org/cpython/rev/5d317e38da44

    @merwok
    Copy link
    Member

    merwok commented Aug 22, 2011

    Ezio: I think that Raymond is in charge of the 3.3 whatsnew too. The rules in a comment at the top of the file still apply: there are already around 30 notes to the file.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 22, 2011

    New changeset f2cb733c9a37 by Sandro Tosi in branch 'default':
    bpo-12191: added entry in What's New (+ small editing on shutil section)
    http://hg.python.org/cpython/rev/f2cb733c9a37

    @merwok
    Copy link
    Member

    merwok commented Sep 2, 2011

    I have added 'chown' in shutil.__all__.

    @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

    8 participants