gh-81881: Raise a shutil.SpecialFileError when copying a Unix socket #16575
Conversation
This used to raise an OSError with a platform dependent message. This change always raises a SpecialFileError with a consistent message no matter the platform.
|
Thanks for the PR @AWhetter! |
|
Good idea. I'll get that changed. |
|
I think it helps everyone follow the flow of the PR when things aren't force pushed and history isn't rewritten. It will get squashed in the end, anyways! I just break up my commits so that the individual diffs are easy to follow. So if I'm editing and moving a function, I'll do those in separate commits in order to preserve an easy-to-follow storyline. |
|
Similar to the issue mentioned in bpo-37701, this raises |
Lib/test/test_shutil.py
Outdated
| else: | ||
| self.fail("shutil.Error should have been raised") | ||
| finally: | ||
| shutil.rmtree(TESTFN2, ignore_errors=True) |
There was a problem hiding this comment.
This seems unnecessary as per:
self.addCleanup(shutil.rmtree, TESTFN, ignore_errors=True)
...some lines above.
| @@ -663,6 +664,29 @@ def test_copytree_named_pipe(self): | |||
| shutil.rmtree(TESTFN, ignore_errors=True) | |||
| shutil.rmtree(TESTFN2, ignore_errors=True) | |||
|
|
|||
| @unittest.skipUnless(hasattr(socket, 'AF_UNIX'), 'requires socket.AF_UNIX') | |||
| def test_copytree_socket(self): | |||
There was a problem hiding this comment.
IMO testing also copytree() is not very useful since internally it uses copyfile (which you are testing).
There was a problem hiding this comment.
I was copying what was done for named pipes for consistency. But having a test for copytree() makes sense to me because if in the future copytree() stopped using copyfile() for some reason, we would still want copytree() to raise an error?
Doc/library/shutil.rst
Outdated
| @@ -54,6 +54,9 @@ Directory and files operations | |||
| *dst* and return *dst* in the most efficient way possible. | |||
| *src* and *dst* are path-like objects or path names given as strings. | |||
|
|
|||
| When *src* is a named pipe or a Unix socket, a :exc:`SpecialFileError` | |||
There was a problem hiding this comment.
- When *src* is a named pipe or a Unix socket, a :exc:`SpecialFileError`
+ When *src* is a named pipe or a Unix socket, :exc:`SpecialFileError`
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @giampaolo: please review the changes made to this pull request. |
|
Is there anything else that I can do to help progress this review? |
|
@giampaolo can this PR be unblocked? |
arhadthedev
changed the title
|
@arhadthedev thanks for the quick response! |
|
|
This used to raise an OSError with a platform dependent message.
This change always raises a SpecialFileError with a consistent message
no matter the platform.
Tackling special devices wasn't part of the original issue, but it is still technically an issue. Although devices are technically copyable (at least they are with
cp), they will likely hang the same as a named pipe would even though a named pipe is technically copyable as well.https://bugs.python.org/issue37700