-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-33687: uu: fix call to os.chmod #7282
Conversation
8d1f4b3
to
f5f8168
Compare
Lib/uu.py
Outdated
@@ -134,7 +134,7 @@ def decode(in_file, out_file=None, mode=None, quiet=False): | |||
elif isinstance(out_file, str): | |||
fp = open(out_file, 'wb') | |||
try: | |||
os.path.chmod(out_file, mode) | |||
os.chmod(out_file, mode) | |||
except AttributeError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm missing something but os.chmod()
is defined undonditionally in Modules/posixmodule.c
:
Line 2821 in 941ec21
os_chmod_impl(PyObject *module, path_t *path, int mode, int dir_fd, |
Do we still need the try...except AttributeError
check? It was added in 1995 in 8b74512#diff-bd8ce21226a634ebc2701b92641e3424R108 and I think we can remove it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I've removed the try ... except
block.
Why test_decodetwice was changed? Seems this breaks the purpose of this test. It would be better to test with a real |
I see, there is something off with the current test implementation - I'll have a look at it.
Why is that? I could argue that this would be a test for |
d8cea61
to
3fc26d2
Compare
@serhiy-storchaka I've seen that you've been working on the tests .. |
It seems like your PR breaks test_uu on Windows (AppVeyor). |
Unfortunately, I don't have access to a Windows. Do you know if |
The test has failed on Linux too: https://python.visualstudio.com/cpython/_build?buildId=6360&tab=details&_a=summary This line returns self.assertFalse(os.access(self.tmpout, os.W_OK)) |
By the way, the with open(self.tmpin, 'rb') as f:
uu.decode(f) |
This is not always the case, because at Line 124 in 1bcb8a6
out_file is set to the out_file encoded into the in_file . And this actually happens before the check if out_file is str or not.
With which user are the tests running?
... which would explain the seen behavior on CI. |
3fc26d2
to
48fb285
Compare
I've now replace the |
Just try to open a file for writing. It should fail. |
That's actually not the case as you can see in: https://python.visualstudio.com/cpython/_build?buildId=8085 The only way I could explain this behavior is that the tests are run a root. |
f607979
to
56211ef
Compare
According to https://python.visualstudio.com/cpython/_build?buildId=8085 (click to "Display build info") UID is indeed 0. |
56211ef
to
49939d5
Compare
Lib/test/test_uu.py
Outdated
|
||
if __name__=="__main__": | ||
if __name__ == "__main__": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this may be done by your editor/IDE automatically, but please don't include cosmetic changes to the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted it. When I see something like that, should I create a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tend to reject cosmetic-only PRs, but if you find similar cases in Lib/test/
(not in the standard library) and have time to prepare a PR, it may be accepted.
49939d5
to
c65cf29
Compare
Would be nice to have a separate user to run the tests .. |
a67107d
to
f6f6e42
Compare
uu is an ancient module and no one has noticed this for the past ~20 years, so perhaps we can just skip the test if UID is 0? |
8614a61
to
e2d8bac
Compare
e2d8bac
to
1b73f9a
Compare
1b73f9a
to
7b12978
Compare
@berkerpeksag, @serhiy-storchaka: Berker approved this change, but it's still not merged. Is it ready to be merged? Since last Serhiy comments, updated have been pushed. |
Is there anything missing from my side to get this merged? |
Thanks @timofurrer for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
GH-11592 is a backport of this pull request to the 3.7 branch. |
(cherry picked from commit 17f05bb) Co-authored-by: Timo Furrer <tuxtimo@gmail.com>
@timofurrer sorry for my late response and thank you for your contribution and patience! :) |
This change addresses the issue reported here: https://bugs.python.org/issue33687
Do I have to pull request this to other branches, too?
The issue exists on all actively maintained cpython branches.
https://bugs.python.org/issue33687