python / cpython

The Python programming language
https://www.python.org
Other
62.51k stars 30.01k forks source link

Restore os.makedirs ability to apply mode to all directories created #86533

Open gpshead opened 3 years ago

gpshead commented 3 years ago
BPO 42367
Nosy @gpshead, @tiran, @serhiy-storchaka, @ZackerySpytz, @JustAnotherArchivist, @kartiksubbarao
PRs
  • python/cpython#23901
  • 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: ```python assignee = None closed_at = None created_at = labels = ['type-feature', 'library', '3.11'] title = 'Restore os.makedirs ability to apply mode to all directories created' updated_at = user = 'https://github.com/gpshead' ``` bugs.python.org fields: ```python activity = actor = 'gregory.p.smith' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'gregory.p.smith' dependencies = [] files = [] hgrepos = [] issue_num = 42367 keywords = ['patch', '3.7regression'] message_count = 5.0 messages = ['381082', '381084', '381086', '381087', '406802'] nosy_count = 7.0 nosy_names = ['gregory.p.smith', 'christian.heimes', 'serhiy.storchaka', 'ZackerySpytz', 'JustAnotherArchivist', 'kartiksubbarao', 'awtimmering'] pr_nums = ['23901'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue42367' versions = ['Python 3.11'] ```

    gpshead commented 3 years ago

    os.makedirs used to pass its mode argument on down recursively so that every level of directory it created would have the specified permissions.

    That was removed in Python 3.7 as https://bugs.python.org/issue19930 as if it were a mis-feature. Maybe it was in one situation, but it was also a desirable *feature*. It wasn't a bug.

    We've got 15 year old code depending on that and the only solution for it to work on Python 3.7-3.9 is to reimplement recursive directory creation. :(

    This feature needs to be brought back. Rather than flip flop on the API, adding an flag to indicate if the permissions should be applied recursively or not seems like the best way forward.

    The "workaround" documented in the above bug is invalid. umask cannot be used to control the intermediate directory creation permissions as that is a process wide global that would impact other threads or signal handlers. umask also cannot be used as umask does not allow setting of special bits such as stat.S_ISVTX.

    result: Our old os.makedirs() code that tried to set the sticky bit (ISVTX) on all directories now fails to set it at all levels.

    gpshead commented 3 years ago

    For consistency, pathlib.Path.mkdir should also gain this option.

    serhiy-storchaka commented 3 years ago

    Alternatively we can add an optional argument for the mode of intermediate directories.

    makedirs(name, mode=0o777, exist_ok=False, intermediate_mode=0o777)
    tiran commented 3 years ago

    +1 for restoring the feature

    +0 for Serhiy's proposal iff intermediate_mode defaults to the value of mode:

        def makedirs(name, mode=0o777, exist_ok=False, *, intermediate_mode=None):
            if intermediate_mode is None:
                intermediate_mode = mode
    gpshead commented 2 years ago

    A new intermediate_mode= kwarg can't default to mode= as that would flip flop the API's default behavior again and no doubt disrupt someone elses 3.7-3.10 authored code depending on it. :(

    Regardless I do somewhat like intermediate_mode= more than the PR's existing recursive_mode= as it opens up more possible behaviors... Allowing None to mean "use mode=" is desirable as that'll likely be the most common case. As I expect this to be very common, an easier to type name than intermediate_mode is desirable. Brainstorming:

    My preference leans towards the latter names. Probably submode=.

    jwilk commented 7 months ago

    Another reason why passing the mode to mkdir() might be preferable to setting umask is that the former overrides default ACLs:

    $ setfacl -d -m o::rwx .
    $ umask 077
    $ mkdir spam
    $ mkdir -m 700 eggs
    $ ls -ld *
    drwx------+ 2 jwilk jwilk 4096 2024-02-20 00:00 eggs
    drwx---rwx+ 2 jwilk jwilk 4096 2024-02-20 00:00 spam
    jwilk commented 7 months ago

    Regarding the argument name, how about pmode (short for "parents mode")?