gh-95672 skip fcntl when pipesize is smaller than pagesize#102163
gh-95672 skip fcntl when pipesize is smaller than pagesize#102163corona10 merged 13 commits intopython:mainfrom
Conversation
Lib/test/test_fcntl.py
Outdated
| pipesize_default = fcntl.fcntl(test_pipe_w, fcntl.F_GETPIPE_SZ) | ||
| pipesize = pipesize_default // 2 # A new value to detect change. | ||
| if pipesize < 512: # the POSIX minimum | ||
| minimum_pipe_size = os.sysconf('SC_PAGESIZE') # There's a check that attempts to skip the tests if the pipe capacity is 512 bytes, but that's less than the smallest page size on x86. |
There was a problem hiding this comment.
Please follow PEP8: https://peps.python.org/pep-0008/#maximum-line-length
Lib/test/test_subprocess.py
Outdated
| os.close(test_pipe_w) | ||
| pipesize = pipesize_default // 2 | ||
| if pipesize < 512: # the POSIX minimum | ||
| minimum_pipe_size = os.sysconf('SC_PAGESIZE') # There's a check that attempts to skip the tests if the pipe capacity is 512 bytes, but that's less than the smallest page size on x86. |
|
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 |
Lib/test/test_fcntl.py
Outdated
| pipesize_default = fcntl.fcntl(test_pipe_w, fcntl.F_GETPIPE_SZ) | ||
| pipesize = pipesize_default // 2 # A new value to detect change. | ||
| if pipesize < 512: # the POSIX minimum | ||
| minimum_pipe_size = os.sysconf('SC_PAGESIZE') # There's a check that attempts to skip the tests if the pipe capacity is 512 bytes, but that's less than the smallest page size on x86. |
There was a problem hiding this comment.
There's a check that attempts to skip the tests if the pipe capacity is 512 bytes, but that's less than the smallest page size on x86.
You need to describe why the os.sysconf('SC_PAGESIZE') is needed rather than describe the issue itself.
Lib/test/test_fcntl.py
Outdated
| hasattr(fcntl, "F_SETPIPE_SZ") and hasattr(fcntl, "F_GETPIPE_SZ"), | ||
| "F_SETPIPE_SZ and F_GETPIPE_SZ are not available on all platforms.") | ||
| def test_fcntl_f_pipesize(self): | ||
| resource = import_module('resource') |
There was a problem hiding this comment.
Why not just using import resource?
There was a problem hiding this comment.
Oh that's right, I'll fix it.
corona10
added
needs backport to 3.10
Lib/test/test_fcntl.py
Outdated
| import struct | ||
| import sys | ||
| import unittest | ||
| import resource |
There was a problem hiding this comment.
@hyeongyun0916 resource module looks like not available on Windows platform.
There was a problem hiding this comment.
I'll add a function to the test support.
| interval = minimum_interval | ||
| return sys.setswitchinterval(interval) | ||
|
|
||
| def get_pagesize(): |
There was a problem hiding this comment.
I'll change https://github.com/python/cpython/blob/main/Lib/test/memory_watchdog.py#L13 to get_pagesize in other pr.
corona10
removed
needs backport to 3.10
Lib/test/test_fcntl.py
Outdated
| import sys | ||
| import unittest | ||
| from test import support | ||
| from test.support import verbose, cpython_only |
There was a problem hiding this comment.
| from test.support import verbose, cpython_only | |
| from test.support import verbose, cpython_only, get_pagesize |
Lib/test/test_fcntl.py
Outdated
| import struct | ||
| import sys | ||
| import unittest | ||
| from test import support |
There was a problem hiding this comment.
| from test import support |
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
corona10
left a comment
corona10
left a comment
There was a problem hiding this comment.
lgtm!
Thanks for the hardwork!
3 participants
skip fcntl when pipesize is smaller than pagesize