[libc++][NFC] Resolve Python 2 FIXME

[gAlfonso-bit]

Summary

We don't use Python 2 anymore, so let us do the recommended fix instead of using the workaround made for Python 2.

[gAlfonso-bit]

Removed all remnants of python 2 support, since python 3.6 and above is now required

[Mordante]

Thanks for this cleanup! Since we just branched LLVM 13 this looks a good time to do this.
I like to see it pass all builds before approving and I really would like @ldionne's approval as well.

[• Quuxplusone]

Current revision LGTM. I was less thrilled about the proposed diff in libcxx/utils/libcxx/util.py... but it seems to be gone from this latest revision?

[arichardson]

libcxx/utils/libcxx/util.py

218

This code can be simplified with python 3: In our CHERI fork I changed this to use the timeout= parameter in https://github.com/CTSRD-CHERI/llvm-project/commit/824490b545707689e47cb6fc3a94633468c003a4. I was going to clean-up and upstream that once python3 is required but never got around to it.

However, I think we can avoid all of this complexity with Popen+communicate now that we require python 3.6:
We should be able to use subprocess.run (https://docs.python.org/3.6/library/subprocess.html#subprocess.run), translating the TimeoutExpired exception to ExecuteCommandTimeoutException (or just updating all callers).

subprocess.run(command, cwd=cwd, input=input, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env) should be sufficient I believe?

[ldionne]

This LGTM with or without the suggested improvements. Ideally we'd make the suggested improvements, but I'll take this patch as-is right now as an improvement over the status quo even without @arichardson's comments.

[arichardson]

I did not intend to add a blocking review. I agree this is an improvement, was just suggesting further simplifications

[jloser]

libcxx/CMakeLists.txt

47

Does this message provide anything useful from just adding REQUIRED argument to the find_package(Python3 COMPONENTS Interpreter) right above?

libcxx/utils/ci/runtimes/CMakeLists.txt

6

Does this message provide anything useful from just adding REQUIRED argument to the find_package(Python3 COMPONENTS Interpreter) right above?

[gAlfonso-bit]

Rebased to main

[gAlfonso-bit]

I do not have commit access. Could someone please land this for me?

[xgupta]

libcxx/CMakeLists.txt

47

@gAlfonso-bit can you address this comment. so I can commit this patch on your behalf.

[gAlfonso-bit]

libcxx/CMakeLists.txt

47

I mean, it tells the user Python 3 will be required. I admit it's a little quick and dirty but this will be expanded upon in future patches.

[jloser]

libcxx/CMakeLists.txt

47

I don't think it adds additional value over the error that would be provided by find_package at CMake configure time, but I won't block your review on it - just food for thought.

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

libcxx/CMakeLists.txt

if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR OR LIBCXX_STANDALONE_BUILD)
 
   # Find the LLVM sources and simulate LLVM CMake options.
   include(HandleOutOfTreeLLVM)
 endif()
 
 if (LIBCXX_STANDALONE_BUILD)
   find_package(Python3 COMPONENTS Interpreter)
   if(NOT Python3_Interpreter_FOUND)
-    message(WARNING "Python3 not found, using python2 as a fallback")
-    find_package(Python2 COMPONENTS Interpreter REQUIRED)
-    if(Python2_VERSION VERSION_LESS 2.7)
-      message(SEND_ERROR "Python 2.7 or newer is required")
-    endif()
-
-    # Treat python2 as python3
-    add_executable(Python3::Interpreter IMPORTED)
-    set_target_properties(Python3::Interpreter PROPERTIES
-      IMPORTED_LOCATION ${Python2_EXECUTABLE})
-    set(Python3_EXECUTABLE ${Python2_EXECUTABLE})
+    message(SEND_ERROR "Python3 not found. Python3 is required")
   endif()
 endif()
 
 # Require out of source build.
 include(MacroEnsureOutOfSourceBuild)
 MACRO_ENSURE_OUT_OF_SOURCE_BUILD(
  "${PROJECT_NAME} requires an out of source build. Please create a separate
  build directory and run 'cmake /path/to/${PROJECT_NAME} [options]' there."

libcxx/utils/ci/runtimes/CMakeLists.txt

 cmake_minimum_required(VERSION 3.13.4)
 project(LLVM_RUNTIMES C CXX ASM)
 
 find_package(Python3 COMPONENTS Interpreter)
 if(NOT Python3_Interpreter_FOUND)
-  message(WARNING "Python3 not found, using python2 as a fallback")
-  find_package(Python2 COMPONENTS Interpreter REQUIRED)
-  if(Python2_VERSION VERSION_LESS 2.7)
-    message(SEND_ERROR "Python 2.7 or newer is required")
-  endif()
-
-  # Treat python2 as python3
-  add_executable(Python3::Interpreter IMPORTED)
-  set_target_properties(Python3::Interpreter PROPERTIES
-    IMPORTED_LOCATION ${Python2_EXECUTABLE})
-  set(Python3_EXECUTABLE ${Python2_EXECUTABLE})
+  message(SEND_ERROR "Python3 not found. Python3 is required")
 endif()
 
 # This needs to be set before we add any Lit target for `add_lit_target` to
 # select the right Lit to run the tests.
 set(LLVM_LIT_OUTPUT_DIR "${CMAKE_CURRENT_BINARY_DIR}/bin")
 
 # Automatically define a few variables that are normally set globally by LLVM.
 # This is to keep some amount of similarity between the global LLVM build and

libcxx/utils/gdb/libcxx/printers.py

class _Children(object):
 
         def __next__(self):
             # child_iter raises StopIteration when appropriate.
             field_name = next(self.child_iter)
             child = self.val["__base_"][field_name]["__value_"]
             self.count += 1
             return ("[%d]" % self.count, child)
 
-        # TODO Delete when we drop Python 2.
-        def next(self):
-            return self.__next__()
-
     def __init__(self, val):
         self.val = val
 
     def to_string(self):
         typename = _remove_generics(_prettify_typename(self.val.type))
         if not self.val.type.fields():
             return "empty %s" % typename
         return "%s containing" % typename
class _VectorBoolIterator(object):
             else:
                 outbit = 0
             self.offset += 1
             if self.offset >= self.bits_per_word:
                 self.item += 1
                 self.offset = 0
             return ("[%d]" % self.count, outbit)
 
-        # TODO Delete when we drop Python 2.
-        def next(self):
-            return self.__next__()
-
     class _VectorIterator(object):
         """Class to iterate over the non-bool vector's children."""
 
         def __init__(self, begin, end):
             self.item = begin
             self.end = end
             self.count = 0
 
         def __iter__(self):
             return self
 
         def __next__(self):
             self.count += 1
             if self.item == self.end:
                 raise StopIteration
             entry = self.item.dereference()
             self.item += 1
             return ("[%d]" % self.count, entry)
 
-        # TODO Delete when we drop Python 2.
-        def next(self):
-            return self.__next__()
-
     def __init__(self, val):
         """Set val, length, capacity, and iterator for bool and normal vectors."""
         self.val = val
         self.typename = _remove_generics(_prettify_typename(val.type))
         begin = self.val["__begin_"]
         if self.val.type.template_argument(0).code == gdb.TYPE_CODE_BOOL:
             self.typename += "<bool>"
             self.length = self.val["__size_"]

libcxx/utils/libcxx/util.py

def capture(args, env=None):
     return out
 
 
 def which(command, paths = None):
     """which(command, [paths]) - Look up the given command in the paths string
     (or the PATH environment variable, if unspecified)."""
 
     if paths is None:
-        paths = os.environ.get('PATH','')
+        paths = os.environ.get('PATH', '')
 
     # Check for absolute match first.
     if os.path.isfile(command):
         return command
 
     # Would be nice if Python had a lib function for this.
     if not paths:
         paths = os.defpath
def executeCommand(command, cwd=None, env=None, input=None, timeout=0):
     if input is not None:
         input = to_bytes(input)
     p = subprocess.Popen(command, cwd=cwd,
                          stdin=subprocess.PIPE,
                          stdout=subprocess.PIPE,
                          stderr=subprocess.PIPE,
                          env=env, close_fds=kUseCloseFDs)
     timerObject = None
-    # FIXME: Because of the way nested function scopes work in Python 2.x we
-    # need to use a reference to a mutable object rather than a plain
-    # bool. In Python 3 we could use the "nonlocal" keyword but we need
-    # to support Python 2 as well.
-    hitTimeOut = [False]
+    hitTimeOut = False
     try:
         if timeout > 0:
             def killProcess():
                 # We may be invoking a shell so we need to kill the
                 # process and all its children.
-                hitTimeOut[0] = True
+                nonlocal hitTimeOut
+                hitTimeOut = True
                 killProcessAndChildren(p.pid)
 
             timerObject = threading.Timer(timeout, killProcess)
             timerObject.start()
 
-        out,err = p.communicate(input=input)
+        out, err = p.communicate(input=input)
         exitCode = p.wait()
     finally:
         if timerObject != None:
             timerObject.cancel()
 
     # Ensure the resulting output is always of string type.
     out = convert_string(out)
     err = convert_string(err)
 
-    if hitTimeOut[0]:
+    if hitTimeOut:
         raise ExecuteCommandTimeoutException(
             msg='Reached timeout of {} seconds'.format(timeout),
             out=out,
             err=err,
             exitCode=exitCode
             )
 
     # Detect Ctrl-C in subprocess.

libcxx/utils/ssh.py

 import os
 import posixpath
 import shlex
 import subprocess
 import sys
 import tarfile
 import tempfile
 
-try:
-   from shlex import quote as cmd_quote
-except ImportError:
-   # for Python 2 compatibility
-   from pipes import quote as cmd_quote
 
 def ssh(args, command):
     cmd = ['ssh', '-oBatchMode=yes']
     if args.extra_ssh_args is not None:
         cmd.extend(shlex.split(args.extra_ssh_args))
     return cmd + [args.host, command]