cpython icon indicating copy to clipboard operation
cpython copied to clipboard

GH-120754: Add a strace helper and test set of syscalls for open().read()

Open cmaloney opened this issue 1 year ago • 7 comments

General goal here is to be able to add tests that behavior isn't unintentionally changed around some common python code patterns, in particular Path("README.rst").read_text().

This started with the code that validated subprocess used vfork, building more general utilities from that core code. The work was motivated in https://github.com/python/cpython/pull/120755 for how to test that particular change.

TODO

  • [x] Resolve how to fix subprocess._USE_VFORK. this PR currently just comments out that test. See: https://discuss.python.org/t/subprocess-use-vfork-escape-hatch-broken-fix-or-remove/56915/2
  • [x] ~~Figure out why the print() of markers needs flush=True to not buffer them. Likely add a test for two print statements not being merged using this infrastructure.~~ -- This is the documented behavior of stdout https://docs.python.org/3/library/sys.html#sys.stdout. Because they're files / not interactive it gets block buffered meaning only once a buffer is full does it flush rather than write() when a print() happens.
  • [x] ~~Validate at least once CI bot actually runs and passes the test~~ -- Ubuntu / Build and test found that glibc there uses newfstat which caused the test to fail. Updated the test to allow libc to use other fstat variants.
  • [x] Where should test for Path().read_bytes() and .read_text() live?
  • [x] Update syscall sequence after https://github.com/python/cpython/pull/120755 is merged -- 06066779ec3a938dad8e7cc9feeadb65fd4d1f69
  • Issue: gh-120754

cmaloney avatar Jun 29 '24 03:06 cmaloney

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app[bot] avatar Jun 29 '24 03:06 bedevere-app[bot]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app[bot] avatar Jun 29 '24 03:06 bedevere-app[bot]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app[bot] avatar Jun 29 '24 03:06 bedevere-app[bot]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app[bot] avatar Jun 29 '24 04:06 bedevere-app[bot]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app[bot] avatar Jun 29 '24 09:06 bedevere-app[bot]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app[bot] avatar Jun 29 '24 09:06 bedevere-app[bot]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app[bot] avatar Jun 29 '24 09:06 bedevere-app[bot]

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.

bedevere-app[bot] avatar Jul 06 '24 07:07 bedevere-app[bot]

I have made the requested changes; please review again

also: Could this get the "skip news" label? From my perspective not a python user visible behavior change

cmaloney avatar Jul 10 '24 02:07 cmaloney

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

bedevere-app[bot] avatar Jul 10 '24 02:07 bedevere-app[bot]

Yea, I think I've addressed all of @gpshead's review comments, I haven't clicked 'resolved conversation' though to keep the review visible for context

cmaloney avatar Aug 18 '24 05:08 cmaloney

Thank you again! (gpshead, if you come back to this and want some change, just let me know)

hauntsaninja avatar Aug 24 '24 20:08 hauntsaninja

:warning::warning::warning: Buildbot failure :warning::warning::warning:

Hi! The buildbot ARM Raspbian 3.x has failed when building commit e38d0afe3548b856ccf0b05c01ed3eefc69cb3e7.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/424/builds/7966) and take a look at the build logs.
  4. Check if the failure is related to this commit (e38d0afe3548b856ccf0b05c01ed3eefc69cb3e7) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/424/builds/7966

Failed tests:

  • test_fileio

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/test/test_fileio.py", line 396, in check_readall
    self.assertEqual(count_similarname('fstat'), 1)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 0 != 1

bedevere-bot avatar Aug 24 '24 20:08 bedevere-bot

Hmm, I'd need a few more logs to figure this out. Let's revert for now https://github.com/python/cpython/pull/123303 and fix in another PR? My bad, I should have thought to run the buildbots on this :-)

hauntsaninja avatar Aug 24 '24 21:08 hauntsaninja

Sounds good to me. Think i have all the bits to try that raspbian locally, need to figure out what its syscall name for fstat looks like

cmaloney avatar Aug 24 '24 21:08 cmaloney

Let me go strace on my Raspbian bot to see what it says. This type of test is also okay to just restrict to a known set of platforms if it comes down to that.

gpshead avatar Aug 24 '24 22:08 gpshead

Okay, I changed the new assertEqual(count_similarname("fstat"), 1) lines assertEqual(count_similar("fstat"), 1, msg=f"set(syscalls)=") to get useful info in the error message. Here's a couple examples:

======================================================================
FAIL: test_syscalls_read (test.test_fileio.PyAutoFileTests.test_syscalls_read) (name='pathlib read_bytes')
Check that the set of system calls produced by the I/O stack is what
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pi/repro-pr-120754/cpython-e38d0afe3548b856ccf0b05c01ed3eefc69cb3e7/Lib/test/test_fileio.py", line 396, in check_readall
    self.assertEqual(count_similarname('fstat'), 1, msg=f"{set(syscalls)=}")
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 0 != 1 : set(syscalls)={'openat', 'read', 'close', 'statx'}

======================================================================
FAIL: test_syscalls_read (test.test_fileio.PyAutoFileTests.test_syscalls_read) (name='pathlib read_text')
Check that the set of system calls produced by the I/O stack is what
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pi/repro-pr-120754/cpython-e38d0afe3548b856ccf0b05c01ed3eefc69cb3e7/Lib/test/test_fileio.py", line 396, in check_readall
    self.assertEqual(count_similarname('fstat'), 1, msg=f"{set(syscalls)=}")
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 0 != 1 : set(syscalls)={'read', 'ioctl', 'openat', 'close', '_llseek', 'statx'}

----------------------------------------------------------------------

gpshead avatar Aug 24 '24 23:08 gpshead

statx is indeed a valid Linux system call that could be used for the purpose.

gpshead avatar Aug 24 '24 23:08 gpshead