Repository navigation
Conversation
os.dup2(fd, fd, inheritable=True) never changed fd inheritability, but with inheritable=False the function might fail or change it in an inconsistent manner depending on the platform.
| # dup2(fd, fd) must have no effect for a valid fd | ||
| # Issue #26935: Avoid failure due to a bionic bug | ||
| # in old Android. | ||
| if (not hasattr(sys, 'getandroidapilevel') or |
There was a problem hiding this comment.
I think this new test code should be placed in a new method. The Android checks should be put in a @unittest.skip* decorator.
| <fd_inheritance>` by default or non-inheritable if *inheritable* | ||
| is ``False``. | ||
|
|
||
| If *fd* is valid and equal to *fd2*, this function has no effect |
There was a problem hiding this comment.
There should be a versionchanged directive.
There was a problem hiding this comment.
"this function has no effect": that's wrong, it does validate the FD which is a legit usage of this function. I would rather remove this sentence.
| self.assertFalse(os.get_inheritable(fd3)) | ||
|
|
||
| # dup2(fd, fd) must have no effect for a valid fd | ||
| # Issue #26935: Avoid failure due to a bionic bug |
There was a problem hiding this comment.
| # Issue #26935: Avoid failure due to a bionic bug | |
| # bpo-26935: Avoid failure due to a bionic bug |
I believe it's preferred to use bpo-n in code comments.
| <fd_inheritance>` by default or non-inheritable if *inheritable* | ||
| is ``False``. | ||
|
|
||
| If *fd* is valid and equal to *fd2*, this function has no effect |
There was a problem hiding this comment.
"this function has no effect": that's wrong, it does validate the FD which is a legit usage of this function. I would rather remove this sentence.
| self.assertEqual(os.dup2(fd, fd3, inheritable=False), fd3) | ||
| self.assertFalse(os.get_inheritable(fd3)) | ||
|
|
||
| # dup2(fd, fd) must have no effect for a valid fd |
There was a problem hiding this comment.
Please move this test into a separated method and emit somehow a SkipTest exception to record that the test is skipped on old Android versions.
| @@ -8041,7 +8041,7 @@ os_dup2_impl(PyObject *module, int fd, int fd2, int inheritable) | |||
| res = fd2; // msvcrt dup2 returns 0 on success. | |||
|
|
|||
There was a problem hiding this comment.
Please document somewhere the rationale for "fd != fd2" tests with a reference to "bpo-32862".
|
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 |
|
This PR is stale because it has been open for 30 days with no activity. |
|
Last time this PR was modified five years ago and still has unresolved reviews. If it stays abandoned, it will be closed. |
|
I'm going to rebase this PR and address the review within a week, please don't close. |
arhadthedev
removed
the
pending
github-actions
Bot
removed
the
stale
|
This PR is stale because it has been open for 30 days with no activity. |
serhiy-storchaka
changed the title
github-actions
Bot
removed
the
stale
7 participants
os.dup2(fd, fd, inheritable=True) never changed fd inheritability,
but with inheritable=False the function might fail or change it
in an inconsistent manner depending on the platform.
https://bugs.python.org/issue32862