gh-95672 skip fcntl when pipesize is smaller than pagesize#102163

hyeongyun0916 · 2023-02-23T02:02:41Z

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.

skip fcntl when pipesize is smaller than pagesize

Issue: pipe test failures on some Linux configurations #95672

corona10 · 2023-02-23T03:32:59Z

Lib/test/test_fcntl.py

 pipesize_default=fcntl.fcntl(test_pipe_w, fcntl.F_GETPIPE_SZ)
 pipesize=pipesize_default//2# A new value to detect change.
-ifpipesize<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.


Please follow PEP8: https://peps.python.org/pep-0008/#maximum-line-length

corona10 · 2023-02-23T03:33:04Z

Lib/test/test_subprocess.py

 os.close(test_pipe_w)
 pipesize=pipesize_default//2
-ifpipesize<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.


bedevere-bot · 2023-02-23T03:33:11Z

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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

corona10 · 2023-02-23T03:35:22Z

Lib/test/test_fcntl.py

 pipesize_default=fcntl.fcntl(test_pipe_w, fcntl.F_GETPIPE_SZ)
 pipesize=pipesize_default//2# A new value to detect change.
-ifpipesize<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'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.

corona10 · 2023-02-24T15:18:23Z

Lib/test/test_fcntl.py

 hasattr(fcntl, "F_SETPIPE_SZ") andhasattr(fcntl, "F_GETPIPE_SZ"),
 "F_SETPIPE_SZ and F_GETPIPE_SZ are not available on all platforms.")
 deftest_fcntl_f_pipesize(self):
+resource=import_module('resource')


Why not just using import resource?

Oh that's right, I'll fix it.

corona10 · 2023-02-25T03:40:34Z

Lib/test/test_fcntl.py

 importstruct
 importsys
 importunittest
+importresource


@hyeongyun0916 resource module looks like not available on Windows platform.

I'll add a function to the test support.

hyeongyun0916 · 2023-03-01T10:31:17Z

Lib/test/support/__init__.py

 interval=minimum_interval
 returnsys.setswitchinterval(interval)

+defget_pagesize():


I'll change https://github.com/python/cpython/blob/main/Lib/test/memory_watchdog.py#L13 to get_pagesize in other pr.

Doc/library/test.rst

Lib/test/support/__init__.py

corona10 · 2023-03-01T11:41:30Z

Lib/test/test_fcntl.py

 importsys
 importunittest
+fromtestimportsupport
 fromtest.supportimportverbose, cpython_only


Suggested change
fromtest.supportimportverbose, cpython_only
fromtest.supportimportverbose, cpython_only, get_pagesize

corona10 · 2023-03-01T11:41:38Z

Lib/test/test_fcntl.py

 importstruct
 importsys
 importunittest
+fromtestimportsupport


Suggested change
from test import support

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

corona10

lgtm!
Thanks for the hardwork!

skip fcntl when pipesize is smaller than pagesize
a3e90fc

hyeongyun0916 requested a review from gpshead as a code owner February 23, 2023 02:02

bedevere-bot added the awaiting review label Feb 23, 2023

bedevere-bot mentioned this pull request Feb 23, 2023
pipe test failures on some Linux configurations #95672
Closed

bedevere-bot added the tests Tests in the Lib/test dir label Feb 23, 2023

corona10 self-assigned this Feb 23, 2023

corona10 self-requested a review February 23, 2023 03:31

corona10 added the skip news label Feb 23, 2023

corona10 requested changes Feb 23, 2023
View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Feb 23, 2023

corona10 reviewed Feb 23, 2023
View reviewed changes

apply review
89cc89f

corona10 reviewed Feb 24, 2023
View reviewed changes

hyeongyun0916 added 2 commits February 25, 2023 02:34

Merge branch 'main' into test-fcntcl-skip-when-smaller-than-pagesize
81a7ac4

apply review
aa752c6

corona10 added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Feb 25, 2023

Merge branch 'main' into test-fcntcl-skip-when-smaller-than-pagesize
f8fb6dd

corona10 reviewed Feb 25, 2023
View reviewed changes

hyeongyun0916 added 2 commits February 28, 2023 02:07

Merge branch 'main' into test-fcntcl-skip-when-smaller-than-pagesize
ec6a0cf

add get_pagesize to test.support
eea71c3

hyeongyun0916 commented Mar 1, 2023
View reviewed changes

Merge branch 'main' into test-fcntcl-skip-when-smaller-than-pagesize
0464b45

corona10 removed needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Mar 1, 2023

corona10 reviewed Mar 1, 2023
View reviewed changes

Doc/library/test.rstShow resolvedHide resolved

corona10 reviewed Mar 1, 2023
View reviewed changes

Lib/test/support/__init__.pyShow resolvedHide resolved

corona10 reviewed Mar 1, 2023
View reviewed changes

Lib/test/support/__init__.pyShow resolvedHide resolved

corona10 reviewed Mar 1, 2023
View reviewed changes

Lib/test/test_fcntl.py Outdated
importstruct
importsys
importunittest
fromtestimportsupport
Copy link
Member
corona10Mar 1, 2023
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
from test import support

hyeongyun0916and others added 5 commits March 1, 2023 21:01

Update Lib/test/support/__init__.py
ca6a96e
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

Update Lib/test/support/__init__.py
e14a345
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

Update Doc/library/test.rst
d5c4006
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

apply reivew
60fe601

Merge branch 'main' into test-fcntcl-skip-when-smaller-than-pagesize
bf11762

corona10 approved these changes Mar 1, 2023
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Mar 1, 2023

corona10 merged commit 2f62a5d into python:mainMar 1, 2023

bedevere-bot removed the awaiting merge label Mar 1, 2023

hyeongyun0916 mentioned this pull request Mar 2, 2023
gh-95672: Update memory_watchdog to use test.support.get_pagesize #102365
Merged

	fromtest.supportimportverbose, cpython_only
	fromtest.supportimportverbose, cpython_only, get_pagesize

Uh oh!

gh-95672 skip fcntl when pipesize is smaller than pagesize#102163

gh-95672 skip fcntl when pipesize is smaller than pagesize #102163

Uh oh!

Conversation

hyeongyun0916 commented Feb 23, 2023• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Feb 23, 2023

Uh oh!

corona10Feb 23, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10Feb 24, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hyeongyun0916 commented Feb 23, 2023•
edited by bedevere-bot
Loading

corona10Feb 23, 2023•
edited
Loading

corona10Feb 24, 2023•
edited
Loading