Revert "GH-120754: Add a strace helper and test set of syscalls for o… (#123303)

hauntsaninja · web-flow · commit 52caaef6d01a · 2024-08-24T21:54:31.000Z
Revert "GH-120754: Add a strace helper and test set of syscalls for open().read() (#121143)" This reverts commit e38d0af.
diff --git a/Lib/test/support/strace_helper.py b/Lib/test/support/strace_helper.py
diff --git a/Lib/test/test_fileio.py b/Lib/test/test_fileio.py
@@ -11,7 +11,7 @@
 
 from test.support import (
     cpython_only, swap_attr, gc_collect, is_emscripten, is_wasi,
-    infinite_recursion, strace_helper
+    infinite_recursion,
 )
 from test.support.os_helper import (
     TESTFN, TESTFN_ASCII, TESTFN_UNICODE, make_bad_fd,
@@ -24,9 +24,6 @@
 import _pyio # Python implementation of io
 
 
-_strace_flags=["--trace=%file,%desc"]
-
-
 class AutoFileTests:
     # file tests for which a test file is automatically set up
 
@@ -362,94 +359,6 @@ def testErrnoOnClosedReadinto(self, f):
         a = array('b', b'x'*10)
         f.readinto(a)
 
-    @strace_helper.requires_strace()
-    def test_syscalls_read(self):
-        """Check that the set of system calls produced by the I/O stack is what
-        is expected for various read cases.
-
-        It's expected as bits of the I/O implementation change, this will need
-        to change. The goal is to catch changes that unintentionally add
-        additional systemcalls (ex. additional calls have been looked at in
-        bpo-21679 and gh-120754).
-        """
-        self.f.write(b"Hello, World!")
-        self.f.close()
-
-
-        def check_readall(name, code, prelude="", cleanup=""):
-            with self.subTest(name=name):
-                syscalls = strace_helper.get_syscalls(code, _strace_flags,
-                                                      prelude=prelude,
-                                                      cleanup=cleanup)
-
-                # There are a number of related syscalls used to implement
-                # behaviors in a libc (ex. fstat, newfstatat, open, openat).
-                # Allow any that use the same substring.
-                def count_similarname(name):
-                    return len([sc for sc in syscalls if name in sc])
-
-                # Should open and close the file exactly once
-                self.assertEqual(count_similarname('open'), 1)
-                self.assertEqual(count_similarname('close'), 1)
-
-                # Should only have one fstat (bpo-21679, gh-120754)
-                self.assertEqual(count_similarname('fstat'), 1)
-
-
-        # "open, read, close" file using different common patterns.
-        check_readall(
-            "open builtin with default options",
-            f"""
-            f = open('{TESTFN}')
-            f.read()
-            f.close()
-            """
-        )
-
-        check_readall(
-            "open in binary mode",
-            f"""
-            f = open('{TESTFN}', 'rb')
-            f.read()
-            f.close()
-            """
-        )
-
-        check_readall(
-            "open in text mode",
-            f"""
-            f = open('{TESTFN}', 'rt')
-            f.read()
-            f.close()
-            """
-        )
-
-        check_readall(
-            "pathlib read_bytes",
-            "p.read_bytes()",
-            prelude=f"""from pathlib import Path; p = Path("{TESTFN}")"""
-
-        )
-
-        check_readall(
-            "pathlib read_text",
-            "p.read_text()",
-            prelude=f"""from pathlib import Path; p = Path("{TESTFN}")"""
-        )
-
-        # Focus on just `read()`.
-        calls = strace_helper.get_syscalls(
-            prelude=f"f = open('{TESTFN}')",
-            code="f.read()",
-            cleanup="f.close()",
-            strace_flags=_strace_flags
-        )
-        # One to read all the bytes
-        # One to read the EOF and get a size 0 return.
-        self.assertEqual(calls.count("read"), 2)
-
-
-
 class CAutoFileTests(AutoFileTests, unittest.TestCase):
     FileIO = _io.FileIO
     modulename = '_io'
diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
@@ -4,7 +4,6 @@
 from test.support import check_sanitizer
 from test.support import import_helper
 from test.support import os_helper
-from test.support import strace_helper
 from test.support import warnings_helper
 from test.support.script_helper import assert_python_ok
 import subprocess
@@ -3416,33 +3415,44 @@ def __del__(self):
 
     @unittest.skipIf(not sysconfig.get_config_var("HAVE_VFORK"),
                      "vfork() not enabled by configure.")
-    @strace_helper.requires_strace()
+    @unittest.skipIf(sys.platform != "linux", "Linux only, requires strace.")
     @mock.patch("subprocess._USE_POSIX_SPAWN", new=False)
     def test_vfork_used_when_expected(self):
         # This is a performance regression test to ensure we default to using
         # vfork() when possible.
         # Technically this test could pass when posix_spawn is used as well
         # because libc tends to implement that internally using vfork. But
         # that'd just be testing a libc+kernel implementation detail.
-
-        # Are intersted in the system calls:
-        # clone,clone2,clone3,fork,vfork,exit,exit_group
-        # Unfortunately using `--trace` with that list to strace fails because
-        # not all are supported on all platforms (ex. clone2 is ia64 only...)
-        # So instead use `%process` which is recommended by strace, and contains
-        # the above.
+        strace_binary = "/usr/bin/strace"
+        # The only system calls we are interested in.
+        strace_filter = "--trace=clone,clone2,clone3,fork,vfork,exit,exit_group"
         true_binary = "/bin/true"
-        strace_args = ["--trace=%process"]
+        strace_command = [strace_binary, strace_filter]
+
+        try:
+            does_strace_work_process = subprocess.run(
+                    strace_command + [true_binary],
+                    stderr=subprocess.PIPE,
+                    stdout=subprocess.DEVNULL,
+            )
+            rc = does_strace_work_process.returncode
+            stderr = does_strace_work_process.stderr
+        except OSError:
+            rc = -1
+            stderr = ""
+        if rc or (b"+++ exited with 0 +++" not in stderr):
+            self.skipTest("strace not found or not working as expected.")
 
         with self.subTest(name="default_is_vfork"):
-            vfork_result = strace_helper.strace_python(
-                f"""\
-                import subprocess
-                subprocess.check_call([{true_binary!r}])""",
-                strace_args
+            vfork_result = assert_python_ok(
+                    "-c",
+                    textwrap.dedent(f"""\
+                    import subprocess
+                    subprocess.check_call([{true_binary!r}])"""),
+                    __run_using_command=strace_command,
             )
             # Match both vfork() and clone(..., flags=...|CLONE_VFORK|...)
-            self.assertRegex(vfork_result.event_bytes, br"(?i)vfork")
+            self.assertRegex(vfork_result.err, br"(?i)vfork")
             # Do NOT check that fork() or other clones did not happen.
             # If the OS denys the vfork it'll fallback to plain fork().
 
@@ -3455,20 +3465,21 @@ def test_vfork_used_when_expected(self):
                 ("setgroups", "", "extra_groups=[]", True),
         ):
             with self.subTest(name=sub_name):
-                non_vfork_result = strace_helper.strace_python(
-                    f"""\
+                non_vfork_result = assert_python_ok(
+                    "-c",
+                    textwrap.dedent(f"""\
                     import subprocess
                     {preamble}
                     try:
                         subprocess.check_call(
                                 [{true_binary!r}], **dict({sp_kwarg}))
                     except PermissionError:
                         if not {expect_permission_error}:
-                            raise""",
-                    strace_args
+                            raise"""),
+                    __run_using_command=strace_command,
                 )
                 # Ensure neither vfork() or clone(..., flags=...|CLONE_VFORK|...).
-                self.assertNotRegex(non_vfork_result.event_bytes, br"(?i)vfork")
+                self.assertNotRegex(non_vfork_result.err, br"(?i)vfork")
 
 
 @unittest.skipUnless(mswindows, "Windows specific tests")
diff --git a/Misc/ACKS b/Misc/ACKS
@@ -1164,7 +1164,6 @@ Grzegorz Makarewicz
 David Malcolm
 Greg Malcolm
 William Mallard
-Cody Maloney
 Ken Manheimer
 Vladimir Marangozov
 Colin Marc
