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

@@ -201,7 +201,8 @@ def test_fcntl_f_pipesize(self):
            # Get the default pipesize with F_GETPIPE_SZ
            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.


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

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

Lib/test/test_subprocess.py

@@ -717,7 +717,8 @@ def test_pipesizes(self):
            os.close(test_pipe_r)
            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.


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

@@ -201,7 +201,8 @@ def test_fcntl_f_pipesize(self):
            # Get the default pipesize with F_GETPIPE_SZ
            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'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

@@ -196,12 +196,14 @@ def test_fcntl_f_getpath(self):
        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')


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

@@ -6,6 +6,7 @@
 import struct
 import sys
 import unittest
+import resource


@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

@@ -1892,6 +1894,16 @@ def setswitchinterval(interval):
            interval = minimum_interval
    return sys.setswitchinterval(interval)

+def get_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

@@ -6,6 +6,7 @@
 import struct
 import sys
 import unittest
+from test import support
 from test.support import verbose, cpython_only


Suggested change

from test.support import verbose, cpython_only

from test.support import verbose, cpython_only, get_pagesize

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

Lib/test/test_fcntl.py

@@ -6,6 +6,7 @@
 import struct
 import sys
 import unittest
+from test import support


Suggested change

from test import support

Co-authored-by: Dong-hee Na <[email protected]>

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.rst Show resolved Hide resolved

corona10 reviewed Mar 1, 2023

View reviewed changes

Lib/test/support/__init__.py Show resolved Hide resolved

corona10 reviewed Mar 1, 2023

View reviewed changes

Lib/test/support/__init__.py Show resolved Hide resolved

corona10 reviewed Mar 1, 2023

View reviewed changes

hyeongyun0916 and others added 5 commits March 1, 2023 21:01

Update Lib/test/support/__init__.py

ca6a96e

Co-authored-by: Dong-hee Na <[email protected]>

Update Lib/test/support/__init__.py

e14a345

Co-authored-by: Dong-hee Na <[email protected]>

Update Doc/library/test.rst

d5c4006

Co-authored-by: Dong-hee Na <[email protected]>

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:main Mar 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hyeongyun0916 commented Feb 23, 2023 •

edited by bedevere-bot

Loading

corona10 Feb 23, 2023

corona10 Feb 23, 2023

bedevere-bot commented Feb 23, 2023

corona10 Feb 23, 2023 •

edited

Loading

corona10 Feb 24, 2023 •

edited

Loading

hyeongyun0916 Feb 25, 2023

corona10 Feb 25, 2023

hyeongyun0916 Mar 1, 2023

hyeongyun0916 Mar 1, 2023

corona10 Mar 1, 2023

corona10 Mar 1, 2023

corona10 left a comment

	from test.support import verbose, cpython_only
	from test.support import verbose, cpython_only, get_pagesize

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

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

Conversation

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Feb 23, 2023

corona10 Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

corona10 Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 left a comment

Choose a reason for hiding this comment

hyeongyun0916 commented Feb 23, 2023 •

edited by bedevere-bot

Loading

corona10 Feb 23, 2023 •

edited

Loading

corona10 Feb 24, 2023 •

edited

Loading