msg148564 - (view) |
Author: R. David Murray (r.david.murray) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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. :)
|
msg393713 - (view) |
Author: Bonifacio (Bonifacio2) * |
Date: 2021-05-15 12:25 |
I just read this issue's history and it looks like it is out of date. The docs now match the behaviour.
> 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.
This isn’t true anymore.
> 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.
The docs now state the behaviour explicitly: “The file permission bits of existing parent directories are not changed.”
This means the reported issue no longer exists. But somewhere along the discussion it became one about adding a new flag to control the behaviour when there are mode mismatches. I tried applying Hynek’s patch locally, but it doesn’t work anymore. If we want to go ahead with adding this new flag we would have to create a new patch. If we want to drop the change then this issue can be closed.
If we decide to go with the flag addition I’m glad to help fixing Hynek’s patch’s incompatibilities if he is ok with that.
|
msg393728 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2021-05-15 20:26 |
This issue was really two issues: fix doc of existing behavior; change behavior (which would be an enhancement). The first has been done; the second not. I think this should be closed and if anyone still proposes the second, open a new enhancement issue for 3.11, referencing this one, that clearly lays out the proposed change and rationale, given the 3.10 status quo. (I have no opinion about the proposed change.)
Once someone submits a patch, we are free to change it before or after applying it. But before, we give the author first crack and wait a week or so for an answer.
Hynek, do you still propose the change in your second patch? If so, do you want to update it and make a PR?
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:24 | admin | set | github: 57707 |
2021-05-15 20:26:39 | terry.reedy | set | messages:
+ msg393728 |
2021-05-15 12:25:29 | Bonifacio2 | set | nosy:
+ Bonifacio2 messages:
+ msg393713
|
2012-10-23 13:17:24 | hynek | set | files:
+ makedirs-on_wrong_mode-1.diff assignee: docs@python -> messages:
+ msg173612
versions:
- Python 3.2, Python 3.3 |
2012-10-08 05:48:38 | python-dev | set | nosy:
+ python-dev messages:
+ msg172361
|
2012-10-07 21:41:57 | TheAdityaKedia | set | nosy:
+ TheAdityaKedia messages:
+ msg172348
|
2012-10-07 15:32:32 | r.david.murray | set | messages:
+ msg172309 |
2012-10-07 09:27:52 | hynek | set | messages:
+ msg172282 |
2012-08-18 16:14:02 | r.david.murray | set | messages:
+ msg168517 |
2012-08-18 15:14:01 | hynek | set | messages:
+ msg168515 versions:
+ Python 3.4 |
2012-08-13 15:13:45 | r.david.murray | set | messages:
+ msg168115 |
2012-08-13 14:52:03 | hynek | set | messages:
+ msg168114 |
2012-08-13 12:40:20 | r.david.murray | set | messages:
+ msg168093 |
2012-08-13 10:19:53 | hynek | set | files:
+ os-makedirs.diff keywords:
+ patch messages:
+ msg168078
stage: needs patch -> patch review |
2012-08-05 11:28:45 | hynek | set | messages:
+ msg167489 |
2012-08-04 19:28:23 | r.david.murray | set | messages:
+ msg167432 |
2012-08-04 10:59:53 | hynek | set | messages:
+ msg167400 |
2012-08-03 14:37:22 | r.david.murray | set | messages:
+ msg167333 |
2012-07-31 09:23:44 | hynek | set | messages:
+ msg166985 |
2012-07-07 10:13:58 | d0ugal | set | nosy:
+ d0ugal
|
2012-07-03 00:13:00 | Axel.Wegen | set | nosy:
+ Axel.Wegen
|
2012-06-16 14:21:08 | hynek | set | nosy:
+ hynek
|
2012-03-11 13:48:07 | jokoala | set | nosy:
+ jokoala messages:
+ msg155383
|
2011-12-14 09:26:45 | Laurent.Mazuel | set | nosy:
+ Laurent.Mazuel
|
2011-12-03 14:49:21 | eric.araujo | set | nosy:
+ georg.brandl, terry.reedy, zooko, belopolsky, ggenellina, draghuram, giampaolo.rodola, ijmorlan, eric.araujo, Arfrever, ysj.ray
|
2011-11-29 13:42:02 | r.david.murray | create | |