classification
Title: uu.py calls os.path.chmod which doesn't exist
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.8, Python 3.7, Python 3.6, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: bsdphk, christian.heimes, martin.panter, r.david.murray, serhiy.storchaka, tuxtimo, vstinner, xiang.zhang
Priority: normal Keywords: easy, patch

Created on 2018-05-29 22:02 by bsdphk, last changed 2018-07-12 14:18 by vstinner.

Pull Requests
URL Status Linked Edit
PR 7282 open tuxtimo, 2018-05-31 11:33
Messages (15)
msg318119 - (view) Author: Poul-Henning Kamp (bsdphk) Date: 2018-05-29 22:02
Library file uu.py on at least 2.7 and 3.6 contains:

            try:
                os.path.chmod(out_file, mode)
            except AttributeError:
                pass

As far as I can tell, os.path.chmod does not exist, so this always raises AttributeError and becomes a no-op.

Is suspect the proper fix is to remove the ".path" ?
msg318151 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-05-30 04:11
Looks as the proper fix to me.
msg318286 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-05-31 11:43
Would be nice to add a test.
msg318293 - (view) Author: Timo Furrer (tuxtimo) * Date: 2018-05-31 12:38
I've added a test and updated the PR.
msg318810 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-06-06 10:41
Is the uu module still maintained? Christian Heimes wants to remove the module:
https://github.com/tiran/peps/blob/oldbatteries/pep-9999.rst

Xiang Zhang made a change in uu last year to add a new feature: new backtick optional parameter, bpo-30103.
msg318814 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2018-06-06 11:46
The email module uses it, so I would object to its being removed.  It may not be used often (probably only when working with old email archives), but there's no good reason I can see to break something that currently works.
msg318815 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2018-06-06 11:56
I modified it for the feature, not maintain it. I remember at that time I get the feeling it's somewhat strange there are two APIs, with similar functionality resides in two modules, need to be updated.
msg319208 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2018-06-10 11:30
What is your use case, Poul-Henning?

It looks like the module has never set the file mode, at least since it was added to Python in 1994. I suggest just remove the dead code and fix the documentation. At least you shouldn’t make this change in bug fix releases. It is just as likely to break people’s code as fix it.

If you do want to change the behaviour, I suggest using “os.open” to set the mode when the file is created. Otherwise you risk errors and messing up the filesystem when using special files e.g. uu.decode(..., "/dev/null").
msg319211 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-06-10 11:55
> It looks like the module has never set the file mode, at least since it was added to Python in 1994. I suggest just remove the dead code and fix the documentation.

You cannot just remove the mode parameter without a deprecation period. Right now, the flag is documented:

https://docs.python.org/dev/library/uu.html
"mode is used to set the permission bits if the file must be created"

I agree with Martin that I am not comfortable to fix the bug in stable branches (2.7, 3.6, 3.7).

I'm +0 to fix the uu module in master (future 3.8).

If we fix the bug in master, we should document that mode is ignored in 2.7, 3.6 and 3.7 (update their documentation).
msg319273 - (view) Author: Poul-Henning Kamp (bsdphk) Date: 2018-06-11 05:39
I was just playing with it in a prototype and noticed that it didn't work.
msg319275 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-06-11 06:21
I concur with Martin. If this feature never worked, there is a risk of breaking user code when fix it. Let consider this as adding a new feature in 3.8. For older versions it should be documented that the mode of the output file is not set.

And I agree that it would be more safe to use os.open(). Either call os.open() and pass the file descriptor to the builtin open() (but be careful to not leak it) or use the opener argument of open().

    def opener(path, flags):
        return os.open(path, flags, mode)
    fp = open(out_file, 'wb', opener=opener)

It would be worth also to use the 'xb' opening mode instead of 'wb'.
msg319276 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-06-11 06:23
But 'xb' should be used only if out_file is not specified.
msg321552 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-12 13:54
Is there really an use case which requires to set the permission of the file created by uu.decode()? It is already possible to call uu.decode() with an open file which has been created with the expected permission. Moreover, it's also possible to explicitly call chmod() *after* uu.decode().

I would suggest to deprecate the mode parameter in Python 3.7 and remove it from Python 3.8.
msg321554 - (view) Author: Poul-Henning Kamp (bsdphk) Date: 2018-07-12 14:02
Please note that the mode is not just a parameter, it is also a data field inside the encoded input.

See: https://en.wikipedia.org/wiki/Uuencoding

(search for "mode")
msg321557 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-12 14:18
>  Please note that the mode is not just a parameter, it is also a data field inside the encoded input.

Oh... right...

"<mode> is the file's Unix file permissions as three octal digits (e.g. 644, 744). This is typically only significant to unix-like operating systems."

Ok, in that case, it's maybe better to fix the bug instead of removing the parameter.
History
Date User Action Args
2018-07-12 14:18:54vstinnersetmessages: + msg321557
2018-07-12 14:02:13bsdphksetmessages: + msg321554
2018-07-12 13:54:02vstinnersetmessages: + msg321552
2018-06-11 06:23:53serhiy.storchakasetmessages: + msg319276
2018-06-11 06:21:44serhiy.storchakasetmessages: + msg319275
2018-06-11 05:39:42bsdphksetmessages: + msg319273
2018-06-10 11:55:59vstinnersetmessages: + msg319211
2018-06-10 11:30:36martin.pantersetnosy: + martin.panter
messages: + msg319208
2018-06-06 11:58:19xiang.zhangsetnosy: + r.david.murray
2018-06-06 11:56:59xiang.zhangsetnosy: - r.david.murray
messages: + msg318815
2018-06-06 11:46:43r.david.murraysetnosy: + r.david.murray
messages: + msg318814
2018-06-06 10:41:25vstinnersetnosy: + christian.heimes, xiang.zhang, vstinner
messages: + msg318810
2018-05-31 12:38:26tuxtimosetnosy: + tuxtimo
messages: + msg318293
2018-05-31 11:43:30serhiy.storchakasetmessages: + msg318286
2018-05-31 11:33:48tuxtimosetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request6908
2018-05-30 04:12:32serhiy.storchakasetkeywords: + easy
stage: needs patch
versions: + Python 3.7, Python 3.8
2018-05-30 04:11:55serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg318151
2018-05-29 22:02:07bsdphkcreate