[lit, test] Fix test cancellation feature detection

[thopre]

Summary

A lit feature guards tests for the lit timeout functionality because on
most system it depends on the availability of the psutil Python module.
However, that feature is defined based on the ability of the testing lit
to cancel test, which does not necessarily apply to the ability of the
tested lit.

In particular, RUN commands have a cleared PYTHONPATH and user site
packages are disabled. In the case where psutil is found by the testing
lit from one of those two source of python path, the tested lit would
not be able to find it, causing timeout tests to fail.

This commit fixes the issue by testing the ability to cancel tests in
the RUN command environment.

[yln]

Your overall explanation of the issue and the solution presented here makes sense to me, but I don't have a good enough understanding to know if this is the best approach.

LGTM with some small nits!
Please wait for a second sign-off or a few days before landing, so that other folks have time to take a look.

llvm/utils/lit/tests/check-tested-lit-timeout-ability

2

10

TIL: we can pass things other than integers to sys.exit()! :)

11–12

Not needed, right?

llvm/utils/lit/tests/lit.cfg

83

Please use text=True in favor of universal_newlines=True.

From the docs:

If encoding or errors are specified, or text is true, file objects for stdin, stdout and stderr are opened in text mode using the specified encoding and errors or the io.TextIOWrapper default. The universal_newlines argument is equivalent to text and is provided for backwards compatibility. By default, file objects are opened in binary mode.

We could also use capture_output=True instead of stderr=subprocess.PIPE.

[thopre]

llvm/utils/lit/tests/lit.cfg

83

Both of these require Python 3.7 and thus fail on Ubuntu 18.04 and RHEL

[yln]

llvm/utils/lit/tests/lit.cfg

83

You are right! Please ignore my comment.

[thopre]

Simplify logic

[thopre]

Do not rely on testing script being executable

[thopre]

Do rely on script being executable but not on Python 3.6 being in PATH, only Python3

[thopre]

Call script with python3 to not rely on it being executable

[thopre]

Add print statement

[thopre]

• Use exception to see file

[thopre]

Use slash as path separator

[thopre]

Fix joining

[thopre]

Print not found file's filename

[jhenderson]

llvm/utils/lit/tests/lit.cfg

83

Does sys.executable instead of "python3" work for your problem (I am not familiar enough with the issue to be sure)? It feels nicer to me, although I can't say any particular reason why.

[thopre]

llvm/utils/lit/tests/lit.cfg

83

I'm not sure what's the problem at the moment. There's a file not found exception on Windows but I have no idea why. I could give it a try and see.

[thopre]

use sys.executable to find Python executable

[thopre]

Remove debuging code now that test pass on Windows as well.

[thopre]

llvm/utils/lit/tests/lit.cfg

83

Many thanks, that was the missing bit. I focused on the script but it was Python3 it could not find.

llvm/utils/lit/tests/check-tested-lit-timeout-ability

+#!/usr/bin/python3
+
+import sys
+from lit.util import killProcessAndChildrenIsSupported
+
+supported, errormsg = killProcessAndChildrenIsSupported()
+
+if not supported:
+    sys.exit(errormsg)
+
+sys.exit()

llvm/utils/lit/tests/lit.cfg

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

 config.substitutions.append(('%{python}', '"%s"' % (sys.executable)))
 
 # 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
-supported, errormsg = lit_config.maxIndividualTestTimeIsSupported
-if supported:
+# Add a feature to detect if test cancellation is available. Check the ability
+# to do cancellation in the same environment as where RUN commands are run.
+# The reason is that on most systems cancellation depends on psutil being
+# available and RUN commands are run with a cleared PYTHONPATH and user site
+# packages disabled.
+testing_script_path = "/".join((os.path.dirname(__file__),
+                                "check-tested-lit-timeout-ability"))
+try:
+    proc = subprocess.run([sys.executable, testing_script_path],
+                          stderr=subprocess.PIPE, env=config.environment,
+                          universal_newlines=True)
+except FileNotFoundError as not_found:
+    msg = "__file__: {}, testing_script_path: {}, not_found: {}".format(__file__,
+                                                         testing_script_path, not_found)
+    raise OSError(msg)
+if proc.returncode == 0:
     config.available_features.add("lit-max-individual-test-time")
 else:
+    errormsg = proc.stderr
     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.')
 
 # When running the lit tests standalone, we want to define the same features
 # that the llvm_config defines. This means that the 'system-windows' feature
 # (and any others) need to match the names in llvm_config for consistency
 if not llvm_config: