[issue13498] os.makedirs exist_ok documentation is incorrect, as is some of the behavior
New submission from R. David Murray <rdmurray@bitdance.com>: The documentation for os.makedirs says: If the target directory with the same mode as specified already exists, raises an OSError exception if exist_ok is False, otherwise no exception is raised. This is not correct. If the file exists but the mode is different than that specified (or defaulted) after applying the umask, then an error is raised regardless of the value of exist_ok. The above wording also implies that if the directory exists but has a different mode, that the mode will be changed. Again, this is not what the code does. It's not clear how useful this raise behavior is, but reading the original issue that added this option it is clearly intentional. The documented behavior does seem useful, but if it actually reset the mode that would be a fairly significant behavior change, and would not be a good idea if the user had not specified a mode. However, at the very least exists_ok should not raise if no mode was specified in the call. The error message raised is also wrong in this case, since it says that the error is that the file already exists when we've said that that is OK. A custom error message for this case would be better. ---------- assignee: docs@python components: Documentation, Library (Lib) keywords: easy messages: 148564 nosy: docs@python, r.david.murray priority: normal severity: normal stage: needs patch status: open title: os.makedirs exist_ok documentation is incorrect, as is some of the behavior type: behavior versions: Python 3.2, Python 3.3 _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Changes by Éric Araujo <merwok@netwok.org>: ---------- nosy: +Arfrever, belopolsky, draghuram, eric.araujo, gagenellina, georg.brandl, giampaolo.rodola, ijmorlan, terry.reedy, ysj.ray, zooko _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Changes by Laurent Mazuel <laurent.mazuel@gmail.com>: ---------- nosy: +Laurent.Mazuel _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Johannes Kolb <johannes.kolb@gmx.net> added the comment: I want to mention a situation in which the current behaviour of os.makedirs is confusing. At the first glance I would expect that if
os.makedirs(path) succeeds, then a following call to os.makedirs(path, exist_ok=True) will succeed too.
This is not always the case. Consider this (assuming Linux as OS and the umask set to 0o022):
os.makedirs('/tmp/mytest') os.chmod('/tmp/mytest', 0o2755) path='/tmp/mytest/dir1' os.makedirs(path) os.makedirs(path, exist_ok=True) OSError: [Errno 17] File exists: '/tmp/mytest/dir1'
The directory '/tmp/mytest' here has the SETGID flag set, which is inherited automatically. Therefore the flags of '/tmp/mytest/dir1' are not 0o755 as expected by os.makedirs, but 0o2755. The same problem occurs if the user can changes the umask between the calls to os.makedirs. I wonder in what situation the current behaviour of os.makedirs is really helpful and not introducing subtle bugs. Consider using os.makedirs to ensure that the output directory of your script exists. Now the user decides to make this output directory world writeable. For most cases this is no problem, but os.makedirs will complain. ---------- nosy: +jokoala _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Changes by Hynek Schlawack <hs@ox.cx>: ---------- nosy: +hynek _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Changes by Axel Wegen <axel.wegen@googlemail.com>: ---------- nosy: +Axel.Wegen _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Changes by Dougal Matthews <dougal85@gmail.com>: ---------- nosy: +d0ugal _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Hynek Schlawack added the comment: So, IMHO if someone calls os.makedirs with a mode != 0o777, they expect to have the directories having those modes afterward. So raising no error if they exist and have the wrong mode would be a plain bug. Python 3.3 already has a helpful error message: FileExistsError: [Errno 17] File exists (mode 777 != expected mode 755): 'foo' and it also handles the sticky issue gracefully: http://hg.python.org/cpython/file/3a08d766eee3/Lib/os.py#l270 So this are an non-issues for 3.3. I'm not sure if it's severe enough to be back ported to 3.2. So there’s only one thing left: the docs are wrong and should be fixed about exist_ok's behavior for both 3.2 & 3.3. That said, I see the rationale for fixing the permissions but we can't just change os.makedirs at this point. So I'd propose to add a "fix_permissions" bool flag that would allow the "no matter what the state is now, I want dir x with permissions y, do whatever is necessary workflow." Opinions? ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
R. David Murray added the comment: I want the opposite: a way to say I don't care what the mode is as long as it exists. Currently there is no way to do that, as far as I remember. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Hynek Schlawack added the comment: do you want it by default or a new flag? default sounds like a source for obscure bugs to me. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
R. David Murray added the comment: I *want* it to be the default, since I think that is the typical use case, but the existing default behavior means that such a backward incompatible change would not be acceptable for exactly the reason you state. So yes, I want it as a new flag. ("exist_really_ok", he says with tongue in cheek.) I haven't given much thought to the API, but perhaps there could be a value for the umask that means "don't care"? ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Hynek Schlawack added the comment: How about something along of: new arg on_wrong_perm= 1. WRONG_PERM_IGNORE 2. WRONG_PERM_FAIL 3. callable that gets called with the directory name and maybe the existing perms to save stat call_ ? ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Hynek Schlawack added the comment: Silence means consent, so I will supply a patch as soon as 3.4 is open. Meanwhile, I reworded the docs for os.makedirs, the patch is attached. Please have a look at it so we can get it in for 3.3. ---------- keywords: +patch stage: needs patch -> patch review Added file: http://bugs.python.org/file26781/os-makedirs.diff _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
R. David Murray added the comment: Silence doesn't mean consent, but it does mean you can go ahead and see if anyone complains :) I think your proposal is fine, but I'd prefer making the sentinels just "IGNORE" and "FAIL". The module namespace means the names themselves don't have to be fully qualified. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Hynek Schlawack added the comment:
Silence doesn't mean consent, but it does mean you can go ahead and see if anyone complains :)
Well that's what I meant. :)
I think your proposal is fine, but I'd prefer making the sentinels just "IGNORE" and "FAIL". The module namespace means the names themselves don't have to be fully qualified.
I thought about that but found them pretty...generic. Anyway, that's 3.4-fodder. Could you have a look at the doc fix please? It applies against 3.2. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
R. David Murray added the comment: English-wise I would drop the "Also". You say "differs from the one supplied", but given the rest of the text I would expect that it is really "differs from the supplied mode masked with the current umask, on systems where the mode is respected", which is a mouthful :(. Perhaps it would flow better if the discussion of exists_ok came after the discussion of mode (that is, as the last thing in the paragraph). ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Hynek Schlawack added the comment: Ok, let’s do it here, that’s easier: .. function:: makedirs(path, mode=0o777, exist_ok=False) .. index:: single: directory; creating single: UNC paths; and os.makedirs() Recursive directory creation function. Like :func:`mkdir`, but makes all intermediate-level directories needed to contain the leaf directory. The default *mode* is ``0o777`` (octal). On some systems, *mode* is ignored. Where it is used, the current umask value is first masked out. If the target directory exists, :exc:`OSError` is raised unless *exist_ok* is set to ``True`` and the mode doesn't contradict the designated mode as discussed in the previous paragraph. If the mode doesn't match, :exc:`OSError` is raised regardless of the value of *exist_ok*. If the directory cannot be created in other cases, an :exc:`OSError` exception is raised too. .. note:: :func:`makedirs` will become confused if the path elements to create include :data:`pardir`. This function handles UNC paths correctly. .. versionadded:: 3.2 The *exist_ok* parameter. Python is so much easier than English. :'( ---------- versions: +Python 3.4 _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
R. David Murray added the comment: This is much better. But let me try tuning the problem paragraph a bit, since I'm a native English speaker: If *exists_ok* is ``False`` (the default), an :exc:`OSError` is raised if the target directory already exists. If *exists_ok* is ``True`` an :exc:`OSError` is still raised if the umask-masked *mode* is different from the existing mode, on systems where the mode is used. :exc:`OSError` will also be raised if the directory creation fails. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Hynek Schlawack added the comment: Let's get this rolling again. First let's fix the docs for 3.2+ first. My current suggestion would be the following: ~~~ .. function:: makedirs(path, mode=0o777, exist_ok=False) .. index:: single: directory; creating single: UNC paths; and os.makedirs() Recursive directory creation function. Like :func:`mkdir`, but makes all intermediate-level directories needed to contain the leaf directory. The default *mode* is ``0o777`` (octal). On some systems, *mode* is ignored. Where it is used, the current umask value is first masked out. If *exists_ok* is ``False`` (the default), an :exc:`OSError` is raised if the target directory already exists. If *exists_ok* is ``True`` an :exc:`OSError` is still raised if the umask-masked *mode* is different from the existing mode, on systems where the mode is used. :exc:`OSError` will also be raised if the directory creation fails. .. note:: :func:`makedirs` will become confused if the path elements to create include :data:`pardir` (eg. ".." on UNIX systems). This function handles UNC paths correctly. .. versionadded:: 3.2 The *exist_ok* parameter. ~~~ Opinions? ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
R. David Murray added the comment: Looks good to me. ---------- _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Aditya Kedia added the comment: Right.. Looks good. ---------- nosy: +TheAdityaKedia _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Roundup Robot added the comment: New changeset 88a7b9c3b6c0 by Hynek Schlawack in branch '3.2': #13498: Clarify docs of os.makedirs()'s exist_ok argument. http://hg.python.org/cpython/rev/88a7b9c3b6c0 ---------- nosy: +python-dev _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
Hynek Schlawack added the comment: As announced, I hereby present an idea how to solve this problem for 3.4. Please have a look at it. :) ---------- assignee: docs@python -> versions: -Python 3.2, Python 3.3 Added file: http://bugs.python.org/file27683/makedirs-on_wrong_mode-1.diff _______________________________________ Python tracker <report@bugs.python.org> <http://bugs.python.org/issue13498> _______________________________________
participants (9)
-
Aditya Kedia -
Axel Wegen
-
Dougal Matthews
-
Hynek Schlawack
-
Johannes Kolb
-
Laurent Mazuel
-
R. David Murray
-
Roundup Robot
-
Éric Araujo