Make mangled_names.test and update_cc_test_checks.py work with Python 2.

[thakis]

Summary

LLVM hasn't dropped Python 2 support yet, and this makes it easier to check that the test passes.

[arichardson]

I think it might be better to just always use sys.executable and skip the tests if the version is less than (3, 5).

I'm not sure it makes sense to make a developer only script compatible with a python version that will be eol very soon.

[MaskRay]

Do you have a platform where installing Python 3 instead of Python 2 will be such an insurmountable task? Python 2.7 will retire in 15 days.

[thakis]

In D71565#1786966, @MaskRay wrote:

Do you have a platform where installing Python 3 instead of Python 2 will be such an insurmountable task? Python 2.7 will retire in 15 days.

If llvm updates its project policy to say that only py 3.x is supported (which x?), I'm fine with that. For now, we still support 2.7, so this script too must stick to that.

The "will retire in 15 days" is fairly meaningless, it's not like it'll just disappear then.

[MaskRay]

In D71565#1787219, @thakis wrote:

In D71565#1786966, @MaskRay wrote:

Do you have a platform where installing Python 3 instead of Python 2 will be such an insurmountable task? Python 2.7 will retire in 15 days.

If llvm updates its project policy to say that only py 3.x is supported (which x?), I'm fine with that. For now, we still support 2.7, so this script too must stick to that.

The "will retire in 15 days" is fairly meaningless, it's not like it'll just disappear then.

My understanding is that this does not interact with the infrastructure. python3 should be pervasive enough that we probably don't have to worry that not enough build bots run REQUIRES: python3. The infrastructure itself may require Python 2.7, but that is different.

About whether we should add Python 2 compatibility to new features: http://lists.llvm.org/pipermail/llvm-dev/2019-December/137634.html

[thakis]

Per thread, it sounds like you're ok with merging this?

[arichardson]

Looks fine to me if @MaskRay is happy with it.

llvm/test/tools/UpdateTestChecks/lit.local.cfg

20

Not related to this review, but while you are changing this line it might be good to replace the single-quoted %s: i.e.:

(name, '%s %s %s' % (shell_quote(config.python_executable), script_path, extra_args)))

[arichardson]

llvm/test/tools/UpdateTestChecks/lit.local.cfg

13

The parameter here is unused now so can be removed.

[MaskRay]

llvm/utils/update_cc_test_checks.py

1

I am concerned of the shebang change. https://lists.llvm.org/pipermail/llvm-dev/2019-December/137695.html

[MaskRay]

llvm/utils/update_cc_test_checks.py

1

Everything else looks good to me.

[thakis]

llvm/utils/update_cc_test_checks.py

1

This makes the script look like every other python in the script. How is that a cause for concern?

[MaskRay]

llvm/utils/update_cc_test_checks.py

1

Many Python scripts in llvm/utils use #!/usr/bin/env python3 as the shebang now.

[thakis]

Looks like it's exactly 10, out of 140, none of them covered by tests.

Since I debugged this failure, I'll go ahead and land this change.

If someone wants to do the work and pull the py3 detection out of this lit.local.cfg into an higher-level lit place and use that, that's fine with me. But I think per thread there's consensus that we shouldn't have this kind of logic on a per-tool level.

[MaskRay]

In D71565#1797014, @thakis wrote:

Looks like it's exactly 10, out of 140, none of them covered by tests.

Since I debugged this failure, I'll go ahead and land this change.

If someone wants to do the work and pull the py3 detection out of this lit.local.cfg into an higher-level lit place and use that, that's fine with me. But I think per thread there's consensus that we shouldn't have this kind of logic on a per-tool level.

@rnk partially reverted a previous update_*_checks.py change in 287307a0c60b68099d5f9dd22ac1db2a42593533.

(FWIW, I still think changing shebang from python3 back to python does not worth the trouble.)

Looks like it's exactly 10, out of 140, none of them covered by tests.

I doubt there are 140-10 actively used #!.*python scripts. There may be lots of ancient scripts that can be removed.

[rnk]

I kind of wish we'd go back to the plain "#!/usr/bin/env python` version of the shebang. On Windows, because symlinks are not commonplace (they can be enabled, but are usually not), Python packages do not have versioned python binary names (python2.7, python3, etc). There is and has always been just python.exe.

If people want to add a startup version check to the test updating scripts so that the exit with a clear error message ("use Python 3"), IMO that would be the best solution. I want to live in a world where there is only one Python version at a time.

[thakis]

I landed the bits of this that rnk didn't revert in 9d49e5c0876f7cf75ce0b5d8b3c8473300eb096a (including the py3->py change in the shebang line; sounds like that's even necessary to get the test landed and passing on Windows).

This should make it possible to attempt relanding the test.

llvm/test/tools/UpdateTestChecks/lit.local.cfg

 import os
 
 import lit.formats
 import lit.util
 
 # python 2.7 backwards compatibility
 try:
     from shlex import quote as shell_quote
 except ImportError:
     from pipes import quote as shell_quote
 
 
 def add_update_script_substition(name, python_exe=config.python_executable,
                                  extra_args=''):
     assert name.startswith('%')
     script_path = os.path.join(config.llvm_src_root, 'utils', name[1:] + '.py')
     assert os.path.isfile(script_path)
     config.substitutions.append(
-        (name, "'%s' %s %s" % (python_exe, script_path, extra_args)))
+        (name, "'%s' %s %s" % (
+            config.python_executable, script_path, extra_args)))
 
 
 config.test_format = lit.formats.ShTest(execute_external=False)
 config.suffixes = ['.test']
 
 llc_path = os.path.join(config.llvm_tools_dir, 'llc')
 if os.path.isfile(llc_path):
     config.available_features.add('llc-binary')
add_update_script_substition('%update_analyze_test_checks',
                                  extra_args=opt_arg)
 
 llvm_mca_path = os.path.join(config.llvm_tools_dir, 'llvm-mca')
 if os.path.isfile(llvm_mca_path):
     config.available_features.add('llvm-mca-binary')
     mca_arg = '--llvm-mca-binary ' + shell_quote(llvm_mca_path)
     add_update_script_substition('%update_test_checks', extra_args=mca_arg)
 
-# update_cc_test_checks requires python3
-py3_exe = lit.util.which('python3')
-if py3_exe:
-    config.substitutions.append(('%python3', shell_quote(py3_exe)))
-    config.available_features.add('python3')
-
 clang_path = os.path.join(config.llvm_tools_dir, 'clang')
 if os.path.isfile(clang_path):
     config.available_features.add('clang-binary')
-    if py3_exe:
-        # XXX: or use --llvm-bin?
-        extra_args = '--clang ' + shell_quote(clang_path)
-        if os.path.isfile(opt_path):
-            extra_args += ' --opt ' + shell_quote(opt_path)
-        add_update_script_substition('%update_cc_test_checks',
-                                     python_exe=py3_exe,
-                                     extra_args=extra_args)

llvm/test/tools/UpdateTestChecks/update_cc_test_checks/mangled_names.test

 ## Basic test checking that update_cc_test_checks.py works as expected for
 ## functions with mangled names
 
-# %update_cc_test_checks is only set if python3 is on the path.
-# FIXME: Is this the best approach? See discussion on D70660.
-# REQUIRES: python3
-
 # RUN: cp -f %S/Inputs/mangled_names.c %t.c && %update_cc_test_checks -v %t.c
 # RUN: diff -u %t.c %S/Inputs/mangled_names.c.expected
 ## Check that running the script again does not change the result:
 # RUN: %update_cc_test_checks -v %t.c
 # RUN: diff -u %t.c %S/Inputs/mangled_names.c.expected
 ## Also try the --function-signature flag
 # RUN: %update_cc_test_checks %t.c --function-signature
 # RUN: diff -u %t.c %S/Inputs/mangled_names.c.funcsig.expected
 ## Verify that running without the --function-signature flag removes the -SAME: lines:
 # RUN: %update_cc_test_checks %t.c
 # RUN: diff -u %t.c %S/Inputs/mangled_names.c.expected

llvm/utils/update_cc_test_checks.py

-#!/usr/bin/env python3
+#!/usr/bin/env python
 '''A utility to update LLVM IR CHECK lines in C/C++ FileCheck test files.
 
 Example RUN lines in .c/.cc test files:
 
 // RUN: %clang -emit-llvm -S %s -o - -O2 | FileCheck %s
 // RUN: %clangxx -emit-llvm -S %s -o - -O2 | FileCheck -check-prefix=CHECK-A %s
 
 Usage:
 
 % utils/update_cc_test_checks.py --llvm-bin=release/bin test/a.cc
 % utils/update_cc_test_checks.py --clang=release/bin/clang /tmp/c/a.cc
 '''
 
+from __future__ import print_function
+
 import argparse
 import collections
 import distutils.spawn
 import json
 import os
 import shlex
 import string
 import subprocess
SUBST = {
     '%clang': [],
     '%clang_cc1': ['-cc1'],
     '%clangxx': ['--driver-mode=g++'],
 }
 
 def get_line2spell_and_mangled(args, clang_args):
   ret = {}
   # Use clang's JSON AST dump to get the mangled name
-  json_dump_args = [args.clang, *clang_args, '-fsyntax-only', '-o', '-']
+  json_dump_args = [args.clang] + clang_args + ['-fsyntax-only', '-o', '-']
   if '-cc1' not in json_dump_args:
     # For tests that invoke %clang instead if %clang_cc1 we have to use
     # -Xclang -ast-dump=json instead:
     json_dump_args.append('-Xclang')
   json_dump_args.append('-ast-dump=json')
   common.debug('Running', ' '.join(json_dump_args))
-  status = subprocess.run(json_dump_args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-  if status.returncode != 0:
+
+  popen = subprocess.Popen(json_dump_args, stdout=subprocess.PIPE,
+                           stderr=subprocess.PIPE, universal_newlines=True)
+  stdout, stderr = popen.communicate()
+  if popen.returncode != 0:
     sys.stderr.write('Failed to run ' + ' '.join(json_dump_args) + '\n')
-    sys.stderr.write(status.stderr.decode())
-    sys.stderr.write(status.stdout.decode())
+    sys.stderr.write(stderr)
+    sys.stderr.write(stdout)
     sys.exit(2)
 
   # Parse the clang JSON and add all children of type FunctionDecl.
   # TODO: Should we add checks for global variables being emitted?
   def parse_clang_ast_json(node):
     node_kind = node['kind']
     # Recurse for the following nodes that can contain nested function decls:
     if node_kind in ('NamespaceDecl', 'LinkageSpecDecl', 'TranslationUnitDecl'):
def parse_clang_ast_json(node):
     # If there is no line it is probably a builtin function -> skip
     if line is None:
       common.debug('Skipping function without line number:', node['name'], '@', node['loc'])
       return
     spell = node['name']
     mangled = node.get('mangledName', spell)
     ret[int(line)-1] = (spell, mangled)
 
-  ast = json.loads(status.stdout.decode())
+  ast = json.loads(stdout)
   if ast['kind'] != 'TranslationUnitDecl':
     common.error('Clang AST dump JSON format changed?')
     sys.exit(2)
   parse_clang_ast_json(ast)
 
   for line, func_name in sorted(ret.items()):
     common.debug('line {}: found function {}'.format(line+1, func_name), file=sys.stderr)
   if not ret: