some subprocess.Popen() related Python cleanups / added TODOs about missing returncode usage (danmar#7065)

firewave · web-flow · commit 469c5e7c3a88 · 2024-12-06T17:22:37.000+01:00
diff --git a/addons/cppcheckdata.py b/addons/cppcheckdata.py
@@ -1714,8 +1714,9 @@ def get_path_premium_addon():
 
 def cmd_output(cmd):
     with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) as p:
-        comm = p.communicate()
-        out = comm[0]
-        if p.returncode == 1 and len(comm[1]) > 2:
-            out = comm[1]
-        return out.decode(encoding='utf-8', errors='ignore')
+        stdout, stderr = p.communicate()
+        rc = p.returncode
+    out = stdout
+    if rc == 1 and len(stderr) > 2:
+        out = stderr
+    return out.decode(encoding='utf-8', errors='ignore')
diff --git a/test/cli/clang-import_test.py b/test/cli/clang-import_test.py
@@ -9,6 +9,7 @@
 from testutils import cppcheck, assert_cppcheck
 
 try:
+    # TODO: handle exitcode?
     subprocess.call(['clang', '--version'])
 except OSError:
     pytest.skip("'clang' does not exist", allow_module_level=True)
diff --git a/test/cli/testutils.py b/test/cli/testutils.py
@@ -119,7 +119,7 @@ def __run_subprocess(args, env=None, cwd=None, timeout=None):
     p = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env, cwd=cwd)
 
     try:
-        comm = p.communicate(timeout=timeout)
+        stdout, stderr = p.communicate(timeout=timeout)
         return_code = p.returncode
         p = None
     except subprocess.TimeoutExpired:
@@ -141,10 +141,9 @@ def __run_subprocess(args, env=None, cwd=None, timeout=None):
             # sending the signal to the process groups causes the parent Python process to terminate as well
             #os.killpg(os.getpgid(p.pid), signal.SIGTERM)  # Send the signal to all the process groups
             p.terminate()
-            comm = p.communicate()
+            stdout, stderr = p.communicate()
+            p = None
 
-    stdout = comm[0]
-    stderr = comm[1]
     return return_code, stdout, stderr
 
 
diff --git a/test/signal/test-signalhandler.py b/test/signal/test-signalhandler.py
@@ -24,10 +24,11 @@ def __call_process(arg):
     if exe is None:
         raise Exception('executable not found')
     with subprocess.Popen([exe, arg], stdout=subprocess.PIPE, stderr=subprocess.PIPE) as p:
-        comm = p.communicate()
-        stdout = comm[0].decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
-        stderr = comm[1].decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
-        return p.returncode, stdout, stderr
+        stdout, stderr = p.communicate()
+        rc = p.returncode
+    stdout = stdout.decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
+    stderr = stderr.decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
+    return rc, stdout, stderr
 
 
 def test_assert():
diff --git a/test/signal/test-stacktrace.py b/test/signal/test-stacktrace.py
@@ -23,10 +23,11 @@ def __call_process():
     if exe is None:
         raise Exception('executable not found')
     with subprocess.Popen([exe], stdout=subprocess.PIPE, stderr=subprocess.PIPE) as p:
-        comm = p.communicate()
-        stdout = comm[0].decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
-        stderr = comm[1].decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
-        return p.returncode, stdout, stderr
+        stdout, stderr = p.communicate()
+        rc = p.returncode
+    stdout = stdout.decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
+    stderr = stderr.decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
+    return rc, stdout, stderr
 
 
 def test_stack():
diff --git a/tools/bisect/bisect_res.py b/tools/bisect/bisect_res.py
@@ -10,16 +10,17 @@ def run(cppcheck_path, options):
     print('running {}'.format(cppcheck_path))
     with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True) as p:
         stdout, stderr = p.communicate()
-        # only 0 and 1 are well-defined in this case
-        if p.returncode > 1:
-            print('error')
-            return None, None, None
-        # signals are reported as negative exitcode (e.g. SIGSEGV -> -11)
-        if p.returncode < 0:
-            print('crash')
-            return p.returncode, stderr, stdout
-        print('done')
-        return p.returncode, stderr, stdout
+        rc = p.returncode
+    # only 0 and 1 are well-defined in this case
+    if rc > 1:
+        print('error')
+        return None, None, None
+    # signals are reported as negative exitcode (e.g. SIGSEGV -> -11)
+    if rc < 0:
+        print('crash')
+        return rc, stderr, stdout
+    print('done')
+    return rc, stderr, stdout
 
 
 # TODO: check arguments
diff --git a/tools/ci.py b/tools/ci.py
@@ -41,8 +41,10 @@ def gitpush():
 def iconv(filename):
     with subprocess.Popen(['file', '-i', filename],
                           stdout=subprocess.PIPE, stderr=subprocess.STDOUT) as p:
-        comm = p.communicate()
-    if 'charset=iso-8859-1' in comm[0]:
+        # TODO: handle p.returncode?
+        stdout, _ = p.communicate()
+    if 'charset=iso-8859-1' in stdout:
+        # TODO: handle exitcode?
         subprocess.call(
             ["iconv", filename, "--from=ISO-8859-1", "--to=UTF-8", "-o", filename])
 
@@ -51,14 +53,19 @@ def iconv(filename):
 def generate_webreport():
     for filename in glob.glob('*/*.cpp'):
         iconv(filename)
+    # TODO: handle exitcode?
     subprocess.call(
         ["git", "commit", "-a", "-m", '"automatic conversion from iso-8859-1 formatting to utf-8"'])
     gitpush()
 
+    # TODO: handle exitcode?
     subprocess.call(["rm", "-rf", "devinfo"])
+    # TODO: handle exitcode?
     subprocess.call(['nice', "./webreport.sh"])
     upload('-r devinfo', 'htdocs/')
+    # TODO: handle exitcode?
     subprocess.call(["make", "clean"])
+    # TODO: handle exitcode?
     subprocess.call(["rm", "-rf", "devinfo"])
 
 
diff --git a/tools/compare_ast_symdb.py b/tools/compare_ast_symdb.py
@@ -15,8 +15,9 @@ def run_cppcheck(cppcheck_parameters:str, clang:str):
     cmd = '{} {} {} --debug --verbose'.format(CPPCHECK, cppcheck_parameters, clang)
     #print(cmd)
     with subprocess.Popen(cmd.split(), stdout=subprocess.PIPE, stderr=subprocess.PIPE) as p:
-        comm = p.communicate()
-    return comm[0].decode('utf-8', 'ignore')
+        # TODO: handle p.returncode?
+        stdout, _ = p.communicate()
+    return stdout.decode('utf-8', 'ignore')
 
 
 def get_ast(debug):
diff --git a/tools/daca2-download.py b/tools/daca2-download.py
@@ -23,6 +23,7 @@ def wget(filepath):
     if '/' in filepath:
         filename = filename[filename.rfind('/') + 1:]
     for d in DEBIAN:
+        # TODO: handle exitcode?
         subprocess.call(
             ['nice', 'wget', '--tries=10', '--timeout=300', '-O', filename, d + filepath])
         if os.path.isfile(filename):
@@ -40,9 +41,11 @@ def latestvername(names):
 def getpackages():
     if not wget('ls-lR.gz'):
         return []
+    # TODO: handle exitcode?
     subprocess.call(['nice', 'gunzip', 'ls-lR.gz'])
     with open('ls-lR', 'rt') as f:
         lines = f.readlines()
+    # TODO: handle exitcode?
     subprocess.call(['rm', 'ls-lR'])
 
     path = None
@@ -141,10 +144,13 @@ def downloadpackage(filepath, outpath):
 
     filename = filepath[filepath.rfind('/') + 1:]
     if filename[-3:] == '.gz':
+        # TODO: handle exitcode?
         subprocess.call(['tar', 'xzvf', filename])
     elif filename[-3:] == '.xz':
+        # TODO: handle exitcode?
         subprocess.call(['tar', 'xJvf', filename])
     elif filename[-4:] == '.bz2':
+        # TODO: handle exitcode?
         subprocess.call(['tar', 'xjvf', filename])
     else:
         return
@@ -153,6 +159,7 @@ def downloadpackage(filepath, outpath):
 
     for g in glob.glob('[#_A-Za-z0-9]*'):
         if os.path.isdir(g):
+            # TODO: handle exitcode?
             subprocess.call(['tar', '-cJvf', outpath + filename[:filename.rfind('.')] + '.xz', g])
             break
 
diff --git a/tools/daca2-getpackages.py b/tools/daca2-getpackages.py
@@ -24,6 +24,7 @@ def wget(filepath):
     if '/' in filepath:
         filename = filename[filename.rfind('/') + 1:]
     for d in DEBIAN:
+        # TODO: handle exitcode?
         subprocess.call(
             ['nice', 'wget', '--tries=10', '--timeout=300', '-O', filename, d + filepath])
         if os.path.isfile(filename):
@@ -41,11 +42,13 @@ def latestvername(names):
 def getpackages():
     if not wget('ls-lR.gz'):
         sys.exit(1)
+    # TODO: handle exitcode?
     subprocess.call(['nice', 'gunzip', 'ls-lR.gz'])
     if not os.path.isfile('ls-lR'):
         sys.exit(1)
     with open('ls-lR', 'rt') as f:
         lines = f.readlines()
+    # TODO: handle exitcode?
     subprocess.call(['rm', 'ls-lR'])
 
     # Example content in ls-lR:
diff --git a/tools/donate-cpu.py b/tools/donate-cpu.py
@@ -251,10 +251,11 @@ def get_client_version_head(path):
                 cmd = 'python3' + ' ' + os.path.join(path, 'tools', 'donate-cpu.py') + ' ' + '--version'
                 with subprocess.Popen(cmd.split(), stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, universal_newlines=True) as p:
                     try:
-                        comm = p.communicate()
-                        return comm[0].strip()
+                        # TODO: handle p.returncode?
+                        stdout, _ = p.communicate()
                     except:
                         return None
+                return stdout.strip()
 
             client_version_head = get_client_version_head(tree_path)
         c, errout, info, t, cppcheck_options, timing_info = lib.scan_package(tree_path, source_path, libraries, capture_callstack)
diff --git a/tools/donate_cpu_lib.py b/tools/donate_cpu_lib.py
@@ -16,7 +16,7 @@
 # Version scheme (MAJOR.MINOR.PATCH) should orientate on "Semantic Versioning" https://semver.org/
 # Every change in this script should result in increasing the version number accordingly (exceptions may be cosmetic
 # changes)
-CLIENT_VERSION = "1.3.65"
+CLIENT_VERSION = "1.3.66"
 
 # Timeout for analysis with Cppcheck in seconds
 CPPCHECK_TIMEOUT = 30 * 60
@@ -403,13 +403,12 @@ def __run_command(cmd, print_cmd=True):
     if print_cmd:
         print(cmd)
     time_start = time.time()
-    comm = None
     if sys.platform == 'win32':
         p = subprocess.Popen(shlex.split(cmd, comments=False, posix=False), stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True, errors='surrogateescape')
     else:
         p = subprocess.Popen(shlex.split(cmd), stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True, errors='surrogateescape', preexec_fn=os.setsid)
     try:
-        comm = p.communicate(timeout=CPPCHECK_TIMEOUT)
+        stdout, stderr = p.communicate(timeout=CPPCHECK_TIMEOUT)
         return_code = p.returncode
         p = None
     except subprocess.TimeoutExpired:
@@ -422,16 +421,16 @@ def __run_command(cmd, print_cmd=True):
                 child.terminate()
             try:
                 # call with timeout since it might get stuck e.g. gcc-arm-none-eabi
-                comm = p.communicate(timeout=5)
+                stdout, stderr = p.communicate(timeout=5)
                 p = None
             except subprocess.TimeoutExpired:
                 pass
     finally:
         if p:
             os.killpg(os.getpgid(p.pid), signal.SIGTERM)  # Send the signal to all the process groups
-            comm = p.communicate()
+            stdout, stderr = p.communicate()
+            p = None
     time_stop = time.time()
-    stdout, stderr = comm
     elapsed_time = time_stop - time_start
     return return_code, stdout, stderr, elapsed_time
 
@@ -545,7 +544,7 @@ def scan_package(cppcheck_path, source_path, libraries, capture_callstack=True,
                 cmd += dir_to_scan
             _, st_stdout, _, _ = __run_command(cmd)
             gdb_pos = st_stdout.find(" received signal")
-            if not gdb_pos == -1:
+            if gdb_pos != -1:
                 last_check_pos = st_stdout.rfind('Checking ', 0, gdb_pos)
                 if last_check_pos == -1:
                     stacktrace = st_stdout[gdb_pos:]
@@ -765,10 +764,10 @@ def has_include(filedata):
 def get_compiler_version():
     if __make_cmd == 'msbuild.exe':
         _, _, stderr, _ = __run_command('cl.exe', False)
-        return stderr.split('\n')[0]
+        return stderr.split('\n', maxsplit=1)[0]
 
     _, stdout, _, _ = __run_command('g++ --version', False)
-    return stdout.split('\n')[0]
+    return stdout.split('\n', maxsplit=1)[0]
 
 
 def get_client_version():
diff --git a/tools/reduce.py b/tools/reduce.py
@@ -64,7 +64,7 @@ def runtool(self, filedata=None):
             timeout = self.__elapsed_time * 2
         p = subprocess.Popen(self.__cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
         try:
-            comm = self.__communicate(p, timeout=timeout)
+            stdout, stderr = self.__communicate(p, timeout=timeout)
         except TimeoutExpired:
             print('timeout')
             p.kill()
@@ -78,13 +78,13 @@ def runtool(self, filedata=None):
             if p.returncode != 0:
                 return True
         elif p.returncode == 0:
-            out = comm[0] + '\n' + comm[1]
+            out = stdout + '\n' + stderr
             if self.__expected in out:
                 return True
         else:
             # Something could be wrong, for example the command line for Cppcheck (CMD).
             # Print the output to give a hint how to fix it.
-            print('Error: {}\n{}'.format(comm[0], comm[1]))
+            print('Error: {}\n{}'.format(stdout, stderr))
         return False
 
     def __writefile(self, filename, filedata):
diff --git a/tools/trac-keywords.py b/tools/trac-keywords.py
@@ -8,8 +8,9 @@
 def readdb():
     cmds = ['sqlite3', TRACDB, 'SELECT id,keywords FROM ticket WHERE status<>"closed";']
     with subprocess.Popen(cmds, stdout=subprocess.PIPE, stderr=subprocess.PIPE) as p:
-        comm = p.communicate()
-    data = comm[0]
+        # TODO: handle p.returncode?
+        stdout, _ = p.communicate()
+    data = stdout
     ret = {}
     for line in data.splitlines():
         pos1 = line.find('|')
diff --git a/tools/triage_py/triage_version.py b/tools/triage_py/triage_version.py
@@ -30,10 +30,11 @@ def sort_commit_hashes(commits):
     git_cmd = 'git rev-list --abbrev-commit --topo-order --no-walk=sorted --reverse ' + ' '.join(commits)
     with subprocess.Popen(git_cmd.split(), stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=git_repo, universal_newlines=True) as p:
         stdout, stderr = p.communicate()
-        if p.returncode != 0:
-            print('error: sorting commit hashes failed')
-            print(stderr)
-            sys.exit(1)
+        rc = p.returncode
+    if rc != 0:
+        print('error: sorting commit hashes failed')
+        print(stderr)
+        sys.exit(1)
     return stdout.splitlines()
 
 verbose = args.verbose
@@ -132,6 +133,7 @@ def sort_commit_hashes(commits):
         # get version string
         version_cmd = exe + ' ' + '--version'
         with subprocess.Popen(version_cmd.split(), stdout=subprocess.PIPE, universal_newlines=True) as p:
+            # TODO: handle p.returncode?
             version = p.stdout.read().strip()
         # sanitize version
         version = version.replace('Cppcheck ', '').replace(' dev', '')
@@ -181,22 +183,23 @@ def sort_commit_hashes(commits):
         start = time.time_ns()
     p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=exe_path, universal_newlines=True)
     try:
-        comm = p.communicate(timeout=args.timeout)
+        stdout, stderr = p.communicate(timeout=args.timeout)
         if args.perf:
             end = time.time_ns()
         out = ''
         if not args.no_stdout:
-            out += comm[0]
+            out += stdout
         if not args.no_stdout and not args.no_stderr:
             out += '\n'
         if not args.no_stderr:
-            out += comm[1]
+            out += stderr
     except subprocess.TimeoutExpired:
         out = "timeout"
         p.kill()
-        comm = p.communicate()
+        p.communicate()
 
     ec = p.returncode
+    p = None
 
     if not do_compare:
         if not use_hashes:
