classification
Title: os.makedirs exist_ok documentation is incorrect, as is some of the behavior
Type: behavior Stage: patch review
Components: Documentation, Library (Lib) Versions: Python 3.4
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, Axel.Wegen, Laurent.Mazuel, TheAdityaKedia, belopolsky, d0ugal, docs@python, draghuram, eric.araujo, georg.brandl, ggenellina, giampaolo.rodola, hynek, ijmorlan, jokoala, python-dev, r.david.murray, terry.reedy, ysj.ray, zooko
Priority: normal Keywords: easy, patch

Created on 2011-11-29 13:42 by r.david.murray, last changed 2012-10-23 13:17 by hynek.

Files
File name Uploaded Description Edit
os-makedirs.diff hynek, 2012-08-13 10:19
makedirs-on_wrong_mode-1.diff hynek, 2012-10-23 13:17 review
Messages (18)
msg148564 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-11-29 13:42
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.
msg155383 - (view) Author: Johannes Kolb (jokoala) Date: 2012-03-11 13:48
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.
msg166985 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-07-31 09:23
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?
msg167333 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-08-03 14:37
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.
msg167400 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-08-04 10:59
do you want it by default or a new flag? default sounds like a source for obscure bugs to me.
msg167432 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-08-04 19:28
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"?
msg167489 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-08-05 11:28
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_

?
msg168078 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-08-13 10:19
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.
msg168093 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-08-13 12:40
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.
msg168114 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-08-13 14:52
> 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.
msg168115 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-08-13 15:13
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).
msg168515 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-08-18 15:14
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. :'(
msg168517 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-08-18 16:14
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.
msg172282 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-10-07 09:27
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?
msg172309 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-10-07 15:32
Looks good to me.
msg172348 - (view) Author: Aditya Kedia (TheAdityaKedia) Date: 2012-10-07 21:41
Right.. Looks good.
msg172361 - (view) Author: Roundup Robot (python-dev) Date: 2012-10-08 05:48
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
msg173612 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-10-23 13:17
As announced, I hereby present an idea how to solve this problem for 3.4. Please have a look at it. :)
History
Date User Action Args
2012-10-23 13:17:24hyneksetfiles: + makedirs-on_wrong_mode-1.diff
assignee: docs@python ->
messages: + msg173612

versions: - Python 3.2, Python 3.3
2012-10-08 05:48:38python-devsetnosy: + python-dev
messages: + msg172361
2012-10-07 21:41:57TheAdityaKediasetnosy: + TheAdityaKedia
messages: + msg172348
2012-10-07 15:32:32r.david.murraysetmessages: + msg172309
2012-10-07 09:27:52hyneksetmessages: + msg172282
2012-08-18 16:14:02r.david.murraysetmessages: + msg168517
2012-08-18 15:14:01hyneksetmessages: + msg168515
versions: + Python 3.4
2012-08-13 15:13:45r.david.murraysetmessages: + msg168115
2012-08-13 14:52:03hyneksetmessages: + msg168114
2012-08-13 12:40:20r.david.murraysetmessages: + msg168093
2012-08-13 10:19:53hyneksetfiles: + os-makedirs.diff
keywords: + patch
messages: + msg168078

stage: needs patch -> patch review
2012-08-05 11:28:45hyneksetmessages: + msg167489
2012-08-04 19:28:23r.david.murraysetmessages: + msg167432
2012-08-04 10:59:53hyneksetmessages: + msg167400
2012-08-03 14:37:22r.david.murraysetmessages: + msg167333
2012-07-31 09:23:44hyneksetmessages: + msg166985
2012-07-07 10:13:58d0ugalsetnosy: + d0ugal
2012-07-03 00:13:00Axel.Wegensetnosy: + Axel.Wegen
2012-06-16 14:21:08hyneksetnosy: + hynek
2012-03-11 13:48:07jokoalasetnosy: + jokoala
messages: + msg155383
2011-12-14 09:26:45Laurent.Mazuelsetnosy: + Laurent.Mazuel
2011-12-03 14:49:21eric.araujosetnosy: + georg.brandl, terry.reedy, zooko, belopolsky, ggenellina, draghuram, giampaolo.rodola, ijmorlan, eric.araujo, Arfrever, ysj.ray
2011-11-29 13:42:02r.david.murraycreate