Don't depend on psutil on AIX

[daltenty]

Summary

On AIX psutil can run into problems with permissions to read the process
tree, which causes problems for python timeout tests which need to kill off
a test and it's children.

This patch adds a workaround by invoking shell via subprocess and using a
platform specific option to ps to list all the descendant processes so we can
kill them. We add some checks so lit can tell whether timeout tests are
supported with out exposing whether we are utilizing the psutil
implementation or the alternative.

[hubert.reinterpretcast]

llvm/utils/lit/lit/LitConfig.py

98 ↗

(On Diff #208190)

Minor nit: The quote indentation no longer lines up with the previous line.

llvm/utils/lit/tests/lit.cfg

62 ↗

(On Diff #208190)

Removing python-psutil as a feature entirely may be a bit aggressive. It has the potential of quietly disabling "out-of-tree" tests. I'm not sure that a Phabricator patch about AIX has the right level of visibility for making such a change. Can you send an RFC about the cleanup to the mailing list?

[delcypher]

@daltenty The "This removes our dependency on psutil." text sounds broader than it actually is. Looking at the implementation the removal of the dependency is only for AIX. All other platforms still depends on the psutil module. I think the commit message should be made clearer on this point.

llvm/utils/lit/tests/lit.cfg

62 ↗

(On Diff #208190)

The python-psutil "feature" is only available in lit's testsuite AFAIK. Hopefully that means that removing it shouldn't effect the testsuites of other projects apart from lit itself. However, giving the mailing list a heads up about this seems like a nice courtesy .

[delcypher]

llvm/utils/lit/lit/LitConfig.py

93 ↗

(On Diff #208190)

If we're going to start specializing how we support this feature on different problems then I think we need to refactor this so we don't have a platform specific code littered throughout the lit code base.

There probably should be a function that determines if killProcessAndChildren() is supported. Here's a sketch

def killProcessAndChildrenIsSupported():
   """
      Returns a tuple (<supported> , <error message>)
      where 
       `<supported>` is True if `killProcessAndChildren()` is supported on the current host, returns False otherwise.
      `<error message>` is an empty string if `<supported>` is True, otherwise is contains a string describing why the function is not supported.
    """
    if platform.system() == 'AIX':
       return (True, "")
     try:
       import psutil
       return (True, "")
     except ImportError:
        return (False, "Python psutil module could not be found")

This killProcessAndChildrenIsSupported() function can then called in any place where we currently try to do import psutil. This hides the platform specific logic that you are trying to introduce here.

llvm/utils/lit/lit/util.py

440 ↗

(On Diff #208190)

This from statement seems unnecessary given that we already have subprocess imported and we just use call() once in this context. I'd rather this was just

subprocess.call('kill -kill $(ps -o pid= -L{})'.format(pid), shell=True)

[daltenty]

• Use subprocess.call instead of importing it
• Create killProcessAndChildrenIsSupported check and rewrite things to use it

[delcypher]

libcxx/utils/libcxx/util.py

256 ↗

(On Diff #208805)

Wait what... why is this duplicated in libcxx/utils/libcxx/util.py?

lldb/lit/lit.cfg.py

81 ↗

(On Diff #208805)

Shouldn't this be

if lit_config.killProcessAndChildrenIsSupported():

?

llvm/utils/lit/lit/LitConfig.py

80 ↗

(On Diff #208805)

I'd prefer if this code lived in llvm/utils/lit/lit/util.py next to killProcessAndChildren() given that this function describes if that function works.

I realise you need to use lit_config.killProcessAndChildrenIsSupported() from inside a lit config but I'd suggest you add another level of indirection to hide this implementation detail so that the API of LitConfig doesn't leak it.

class LitConfig(object):
   ...
   @property
   def maxIndividualTestTimeIsSupported(self):
     """
            Returns a tuple (<supported> , <error message>)
            where
            `<supported>` is True if `killProcessAndChildren()` is supported on
                the current host, returns False otherwise.
            `<error message>` is an empty string if `<supported>` is True,
                otherwise is contains a string describing why the function is
                not supported.
      """
      return lit.util.killProcessAndChildrenIsSupported()

Alternatively you could change the implementation of LitObject.maxIndividualTestTime setter to throw a custom exception if an attempt is made to set the timeout when the platform doesn't support it. This custom exception would store the error message so it could be programmatically accessed.

[davide]

This is adding a fair amount of complexity on something that just works fine on basically every platform but AIX.
If AIX has issue with psutil, maybe the fix should be submitted to psutil upstream instead of having this dance here?

[daltenty]

In D64251#1577374, @davide wrote:

This is adding a fair amount of complexity on something that just works fine on basically every platform but AIX.
If AIX has issue with psutil, maybe the fix should be submitted to psutil upstream instead of having this dance here?

I appreciate the concern, however in the original change which added timeout functionality to lit (review D14706), it was already noted that depending on psutil wasn't supposed to be permanent. Removing this external dependency from lit would be desirable and hopefully other platforms will follow suit in the future.

[daltenty]

• Address review comments round 2

[delcypher]

llvm/utils/lit/lit/LitConfig.py

81 ↗

(On Diff #209409)

Sorry to be pedantic but this (LitConfig.killProcessAndChildrenIsSupported) should be called maxIndividualTestTimeIsSupported to be consistent with the rest of LitConfig's public interface. The name killProcessAndChildrenIsSupported is leaking implementation details.

llvm/utils/lit/lit/util.py

426 ↗

(On Diff #209409)

I don't really like how we're now coupling this function with the LitConfig object just so we can print out the text Found python psutil module. Do we actually need to print out that message? If the tests don't rely on this I either suggest we remove this.

[daltenty]

llvm/utils/lit/lit/util.py

426 ↗

(On Diff #209409)

I don't really think it's necessary, this was just to mimic the previous behavior. I will remove it.

[daltenty]

• Address review comments round 3

[daltenty]

• Add a space to warning in LLDB lit config

[delcypher]

@daltenty Other than the minor nit, LGTM.

llvm/utils/lit/lit/util.py

449 ↗

(On Diff #209621)

Minor nit. s/plaftorms/platforms/

[daltenty]

• Fix typo in comment

[daltenty]

Ping for an explicit acceptance.

[delcypher]

LGTM.

@davide While I agree with sentiment of fixing psutil, the use of psutil was never meant to be permanent. As the owner of this code I don't mind making accommodations for other platforms provided we keep implementation details well contained as this patch now does.

libcxx/trunk/utils/libcxx/util.py

def killProcessAndChildren(pid):
     This function kills a process with ``pid`` and all its
     running children (recursively). It is currently implemented
     using the psutil module which provides a simple platform
     neutral implementation.
 
     TODO: Reimplement this without using psutil so we can
           remove our dependency on it.
     """
+    if platform.system() == 'AIX':
+        subprocess.call('kill -kill $(ps -o pid= -L{})'.format(pid), shell=True)
+    else:
         import psutil
         try:
             psutilProc = psutil.Process(pid)
             # Handle the different psutil API versions
             try:
                 # psutil >= 2.x
                 children_iterator = psutilProc.children(recursive=True)
             except AttributeError:
                 # psutil 1.x
                 children_iterator = psutilProc.get_children(recursive=True)
             for child in children_iterator:
                 try:
                     child.kill()
                 except psutil.NoSuchProcess:
                     pass
             psutilProc.kill()
         except psutil.NoSuchProcess:
             pass
 
 
 def executeCommandVerbose(cmd, *args, **kwargs):
     """
     Execute a command and print its output on failure.
     """
     out, err, exitCode = executeCommand(cmd, *args, **kwargs)
     if exitCode != 0:
         report = makeReport(cmd, out, err, exitCode)
         report += "\n\nFailed!"
         sys.stderr.write('%s\n' % report)
     return out, err, exitCode

lldb/trunk/lit/lit.cfg.py

 # -*- Python -*-
 
 import os
+import platform
 import re
 import shutil
 import site
 import sys
 
 import lit.formats
 from lit.llvm import llvm_config
 from lit.llvm.subst import FindTool

 for i in ['module-cache-clang', 'module-cache-lldb']:
     cachedir = os.path.join(config.lldb_libs_dir, '..',
                             'lldb-test-build.noindex', i)
     if os.path.isdir(cachedir):
         print("Deleting module cache at %s."%cachedir)
         shutil.rmtree(cachedir)
 
 # Set a default per-test timeout of 10 minutes. Setting a timeout per test
-# requires the psutil module and lit complains if the value is set but the
-# module can't be found.
-try:
-    import psutil  # noqa: F401
+# requires that killProcessAndChildren() is supported on the platform and
+# lit complains if the value is set but it is not supported.
+supported, errormsg = lit_config.maxIndividualTestTimeIsSupported
+if supported:
     lit_config.maxIndividualTestTime = 600
-except ImportError:
-    pass
+else:
+    lit_config.warning("Could not set a default per-test timeout. " + errormsg)
+
 
 # If running tests natively, check for CPU features needed for some tests.
 
 if 'native' in config.available_features:
     cpuid_exe = lit.util.which('lit-cpuid', config.lldb_tools_dir)
     if cpuid_exe is None:
         lit_config.warning("lit-cpuid not found, tests requiring CPU extensions will be skipped")
     else:

llvm/trunk/utils/lit/lit/LitConfig.py

 from __future__ import absolute_import
 import inspect
 import os
+import platform
 import sys
 
 import lit.Test
 import lit.formats
 import lit.TestingConfig
 import lit.util
 
 # LitConfig must be a new style class for properties to work
class LitConfig(object):
     @property
     def maxIndividualTestTime(self):
         """
             Interface for getting maximum time to spend executing
             a single test
         """
         return self._maxIndividualTestTime
 
+    @property
+    def maxIndividualTestTimeIsSupported(self):
+        """
+            Returns a tuple (<supported> , <error message>)
+            where
+            `<supported>` is True if setting maxIndividualTestTime is supported
+                on the current host, returns False otherwise.
+            `<error message>` is an empty string if `<supported>` is True,
+                otherwise is contains a string describing why setting
+                maxIndividualTestTime is not supported.
+        """
+        return lit.util.killProcessAndChildrenIsSupported()
+
     @maxIndividualTestTime.setter
     def maxIndividualTestTime(self, value):
         """
             Interface for setting maximum time to spend executing
             a single test
         """
         if not isinstance(value, int):
             self.fatal('maxIndividualTestTime must set to a value of type int.')
         self._maxIndividualTestTime = value
         if self.maxIndividualTestTime > 0:
-            # The current implementation needs psutil to set
+            # The current implementation needs psutil on some platforms to set
             # a timeout per test. Check it's available.
             # See lit.util.killProcessAndChildren()
-            try:
-                import psutil  # noqa: F401
-            except ImportError:
-                self.fatal("Setting a timeout per test requires the"
-                           " Python psutil module but it could not be"
-                           " found. Try installing it via pip or via"
-                           " your operating system's package manager.")
+            supported, errormsg = self.maxIndividualTestTimeIsSupported
+            if not supported:
+                self.fatal('Setting a timeout per test not supported. ' +
+                           errormsg)
         elif self.maxIndividualTestTime < 0:
             self.fatal('The timeout per test must be >= 0 seconds')
 
     def load_config(self, config, path):
         """load_config(config, path) - Load a config object from an alternate
         path."""
         if self.debug:
             self.note('load_config from %r' % path)

llvm/trunk/utils/lit/lit/util.py

if 'darwin' in config.target_triple:
             out = out.strip()
             res = cmd.wait()
         except OSError:
             res = -1
         if res == 0 and out:
             return out.decode()
     return None
 
+def killProcessAndChildrenIsSupported():
+    """
+        Returns a tuple (<supported> , <error message>)
+        where
+        `<supported>` is True if `killProcessAndChildren()` is supported on
+            the current host, returns False otherwise.
+        `<error message>` is an empty string if `<supported>` is True,
+            otherwise is contains a string describing why the function is
+            not supported.
+    """
+    if platform.system() == 'AIX':
+        return (True, "")
+    try:
+        import psutil  # noqa: F401
+        return (True, "")
+    except ImportError:
+        return (False,  "Requires the Python psutil module but it could"
+                        " not be found. Try installing it via pip or via"
+                        " your operating system's package manager.")
 
 def killProcessAndChildren(pid):
     """This function kills a process with ``pid`` and all its running children
-    (recursively). It is currently implemented using the psutil module which
-    provides a simple platform neutral implementation.
+    (recursively). It is currently implemented using the psutil module on some
+    platforms which provides a simple platform neutral implementation.
 
-    TODO: Reimplement this without using psutil so we can       remove
-    our dependency on it.
+    TODO: Reimplement this without using psutil on all platforms so we can
+    remove our dependency on it.
 
     """
+    if platform.system() == 'AIX':
+        subprocess.call('kill -kill $(ps -o pid= -L{})'.format(pid), shell=True)
+    else:
         import psutil
         try:
             psutilProc = psutil.Process(pid)
             # Handle the different psutil API versions
             try:
                 # psutil >= 2.x
                 children_iterator = psutilProc.children(recursive=True)
             except AttributeError:
                 # psutil 1.x
                 children_iterator = psutilProc.get_children(recursive=True)
             for child in children_iterator:
                 try:
                     child.kill()
                 except psutil.NoSuchProcess:
                     pass
             psutilProc.kill()
         except psutil.NoSuchProcess:
             pass
 
 
 try:
     import win32api
 except ImportError:
     win32api = None
 
 def abort_now():
     """Abort the current process without doing any exception teardown"""
     sys.stdout.flush()
     if win32api:
         win32api.TerminateProcess(win32api.GetCurrentProcess(), 3)
     else:
         os.kill(0, 9)

llvm/trunk/utils/lit/tests/googletest-timeout.py

-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # Check that the per test timeout is enforced when running GTest tests.
 #
 # RUN: not %{lit} -j 1 -v %{inputs}/googletest-timeout --timeout=1 > %t.cmd.out
 # RUN: FileCheck < %t.cmd.out %s
 
 # Check that the per test timeout is enforced when running GTest tests via
 # the configuration file

llvm/trunk/utils/lit/tests/lit.cfg

 # -*- Python -*-
 
 import os
+import platform
 import sys
 
 import lit.formats
 from lit.llvm import llvm_config
 
 # Configuration file for the 'lit' test runner.
 
 # name: The name of this test suite.

 
 # Enable coverage.py reporting, assuming the coverage module has been installed
 # and sitecustomize.py in the virtualenv has been modified appropriately.
 if lit_config.params.get('check-coverage', None):
     config.environment['COVERAGE_PROCESS_START'] = os.path.join(
         os.path.dirname(__file__), ".coveragerc")
 
 # Add a feature to detect if psutil is available
-try:
-    import psutil
-    lit_config.note('Found python psutil module')
-    config.available_features.add("python-psutil")
-except ImportError:
-    lit_config.warning('Could not import psutil. Some tests will be skipped and'
-                       ' the --timeout command line argument will not work.')
+supported, errormsg = lit_config.maxIndividualTestTimeIsSupported
+if supported:
+    config.available_features.add("lit-max-individual-test-time")
+else:
+    lit_config.warning('Setting a timeout per test not supported. ' + errormsg
+                       + ' Some tests will be skipped and the --timeout'
+                         ' command line argument will not work.')

llvm/trunk/utils/lit/tests/shtest-timeout.py

-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # llvm.org/PR33944
 # UNSUPPORTED: system-windows
 
 # FIXME: This test is fragile because it relies on time which can
 # be affected by system performance. In particular we are currently
 # assuming that `short.py` can be successfully executed within 2
 # seconds of wallclock time.