GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-95672 skip fcntl when pipesize is smaller than pagesize by hyeongyun0916 · Pull Request #102163 · python/cpython

Conversation

hyeongyun0916 commented Feb 23, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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

Comment thread

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.

corona10 Feb 23, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hyeongyun0916 reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

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.

corona10 Feb 23, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting changes and removed awaiting review labels

Feb 23, 2023

bedevere-bot commented Feb 23, 2023

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 reviewed

Feb 23, 2023

View reviewed changes

Comment thread

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.

corona10 Feb 23, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hyeongyun0916 reacted with thumbs up emoji

All reactions

👍 1 reaction


          apply review

89cc89f

corona10 reviewed

Feb 24, 2023

View reviewed changes

Comment thread

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')

corona10 Feb 24, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just using import resource?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hyeongyun0916 Feb 25, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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 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

Comment thread

Lib/test/test_fcntl.py Outdated

		import struct
		import sys
		import unittest
		import resource

corona10 Feb 25, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hyeongyun0916 Mar 1, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a function to the test support.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Comment thread

Lib/test/support/__init__.py

		interval = minimum_interval
		return sys.setswitchinterval(interval)

		def get_pagesize():

hyeongyun0916 Mar 1, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


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

0464b45

corona10 removed needs backport to 3.10 needs backport to 3.11 only security fixes labels

Mar 1, 2023

corona10 reviewed

Mar 1, 2023

View reviewed changes

Comment thread

Doc/library/test.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

corona10 reviewed

Mar 1, 2023

View reviewed changes

Comment thread

Lib/test/support/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

corona10 reviewed

Mar 1, 2023

View reviewed changes

Comment thread

Lib/test/support/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

corona10 reviewed

Mar 1, 2023

View reviewed changes

Comment thread

Lib/test/test_fcntl.py Outdated

		import sys
		import unittest
		from test import support
		from test.support import verbose, cpython_only

corona10 Mar 1, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

corona10 reviewed

Mar 1, 2023

View reviewed changes

Comment thread

Lib/test/test_fcntl.py Outdated

		import struct
		import sys
		import unittest
		from test import support

corona10 Mar 1, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

corona10 left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!
Thanks for the hardwork!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Reviewers

corona10

corona10 approved these changes

gpshead

Awaiting requested review from gpshead

gpshead is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

corona10

Labels

skip news tests

Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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!

corona10 Feb 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!

corona10 Feb 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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hyeongyun0916 commented Feb 23, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

corona10 Feb 23, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

corona10 Feb 24, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.