[CodeHealth] Migrate PPAPI/PRESUBMIT.py to Python3.
This CL represents some minimal effort to un-block the codebase wide
migration of PRESUBMIT.py to Python3. It does not migrate the full suite
of PPAPI to Python3.
Tested with `git cl presubmit --force --file "ppapi/*" --verbose` and no
Python related warnings/errors.
Bug: 1222512
Change-Id: Ic121d5ea10284f13c78e517cd49f631ccb70744c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3810377
Commit-Queue: William Liu <liuwilliam@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Derek Schuff <dschuff@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032529}
diff --git a/ppapi/PRESUBMIT.py b/ppapi/PRESUBMIT.py
index 61e78a77..6138cc37 100644
--- a/ppapi/PRESUBMIT.py
+++ b/ppapi/PRESUBMIT.py
@@ -6,16 +6,26 @@
import os
import re
import subprocess
+import sys
+
+
+# In this file `sys.executable` is used instead of
+# `input_api.python3_executable` because on Windows
+# `input_api.python3_executable` is `vpython3.bat` whereas `sys.executable` is
+# `python.exe`. If `input_api.python3_executable` is used, we need to explicitly
+# pass `shell=True` to `subprocess.Popen()`, which is a security risk
+# (https://docs.python.org/3/library/subprocess.html#security-considerations).
+#
+# TODO: Investigate the incompatibility of `input_api.python3_executable` on
+# Windows, for this particular PRESUBMIT script.
USE_PYTHON3 = True
-def RunCmdAndCheck(cmd, err_string, output_api, cwd=None, warning=False,
- shell=False):
+def RunCmdAndCheck(cmd, err_string, output_api, cwd=None, warning=False):
results = []
p = subprocess.Popen(cmd, cwd=cwd,
- shell=shell,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
(_, p_stderr) = p.communicate()
@@ -40,16 +50,12 @@
if name_parts[0:2] == ['ppapi', 'generators']:
generator_files.append(filename)
if generator_files != []:
- # TODO(crbug.com/1222512): Use sys.executable instead of
- # input_api.python_executable once idl_tests.py is py3 compatible, drop
- # shell=True in RunCmdAndCheck.
- cmd = [input_api.python_executable, 'idl_tests.py']
+ cmd = [sys.executable, 'idl_tests.py']
ppapi_dir = input_api.PresubmitLocalPath()
results.extend(RunCmdAndCheck(cmd,
'PPAPI IDL unittests failed.',
output_api,
- os.path.join(ppapi_dir, 'generators'),
- shell=input_api.is_windows))
+ os.path.join(ppapi_dir, 'generators')))
return results
@@ -162,11 +168,8 @@
# The command line is too long.
files_per_command = 25 if input_api.is_windows else 1000
results = []
- for i in range(0, len(nacl_sdk_files), files_per_command):
- # TODO(crbug.com/1222512): Use sys.executable instead of
- # input_api.python_executable once idl_tests.py is py3 compatible, drop
- # shell=True.
- cmd = [input_api.python3_executable, verify_ppapi_py
+ for i in range(len(nacl_sdk_files), files_per_command):
+ cmd = [sys.executable, verify_ppapi_py
] + nacl_sdk_files[i:i + files_per_command]
results.extend(
RunCmdAndCheck(
@@ -176,8 +179,7 @@
'To ignore a file, add it to IGNORED_FILES in '
'native_client_sdk/src/build_tools/verify_ppapi.py)',
output_api,
- warning=True,
- shell=input_api.is_windows))
+ warning=True))
return results
# Verify that changes to ppapi/thunk/interfaces_* files have a corresponding
@@ -346,10 +348,7 @@
# --diff to generate a unified diff
# --out to pick which files to examine (only the ones in the CL)
ppapi_dir = input_api.PresubmitLocalPath()
- # TODO(crbug.com/1222512): Use sys.executable instead of
- # input_api.python_executable once idl_tests.py is py3 compatible, drop
- # shell=True in RunCmdAndCheck.
- cmd = [input_api.python_executable, 'generator.py',
+ cmd = [sys.executable, 'generator.py',
'--wnone', '--diff', '--test','--cgen', '--range=start,end']
# Only generate output for IDL files references (as *.h or *.idl) in this CL
@@ -357,8 +356,7 @@
cmd_results = RunCmdAndCheck(cmd,
'PPAPI IDL Diff detected: Run the generator.',
output_api,
- os.path.join(ppapi_dir, 'generators'),
- shell=input_api.is_windows)
+ os.path.join(ppapi_dir, 'generators'))
if cmd_results:
results.extend(cmd_results)
diff --git a/ppapi/generators/idl_node.py b/ppapi/generators/idl_node.py
index feeb430..76ecfa7c 100755
--- a/ppapi/generators/idl_node.py
+++ b/ppapi/generators/idl_node.py
@@ -277,8 +277,8 @@
my_releases = set([my_min])
r = self.GetRelease(self.GetProperty('version'))
- if not r in my_releases:
- my_releases |= set([r])
+ if r is not None and r not in my_releases:
+ my_releases.add(r)
# Break cycle if we reference ourselves
if self in visited:
@@ -315,7 +315,7 @@
for rel in child_releases | type_releases:
if rel >= my_min and rel <= my_max:
- my_releases |= set([rel])
+ my_releases.add(rel)
self.releases = sorted(my_releases)
return self.releases
diff --git a/ppapi/generators/idl_outfile.py b/ppapi/generators/idl_outfile.py
index 9b830ad..95dbd2ac 100755
--- a/ppapi/generators/idl_outfile.py
+++ b/ppapi/generators/idl_outfile.py
@@ -112,11 +112,12 @@
if not self.always_write:
if os.path.isfile(filename):
- oldtext = open(filename, 'rb').read()
- if self.IsEquivalent_(oldtext):
- if GetOption('verbose'):
- InfoOut.Log('Output %s unchanged.' % self.filename)
- return False
+ with open(filename, 'r', encoding='utf-8') as fin:
+ oldtext = fin.read()
+ if self.IsEquivalent_(oldtext):
+ if GetOption('verbose'):
+ InfoOut.Log('Output %s unchanged.' % self.filename)
+ return False
if GetOption('diff'):
for line in difflib.unified_diff(oldtext.split('\n'), outtext.split('\n'),
@@ -134,9 +135,8 @@
os.makedirs(basepath)
if not GetOption('test'):
- outfile = open(filename, 'wb')
- outfile.write(outtext)
- outfile.close();
+ with open(filename, 'w', newline='\n', encoding='utf=8') as fout:
+ fout.write(outtext)
InfoOut.Log('Output %s written.' % self.filename)
return True
