[Lldb-commits] [PATCH] D64251: Don't depend on psutil on AIX

[Dan Liew via Phabricator via lldb-commits]

delcypher added inline comments.


================
Comment at: libcxx/utils/libcxx/util.py:256
     """
-    import psutil
-    try:
-        psutilProc = psutil.Process(pid)
-        # Handle the different psutil API versions
+    if platform.system() == 'AIX':
+        subprocess.call('kill -kill $(ps -o pid= -L{})'.format(pid), shell=True)
----------------
Wait what... why is this duplicated in `libcxx/utils/libcxx/util.py`?


================
Comment at: lldb/lit/lit.cfg.py:81
+# lit complains if the value is set but it is not supported.
+if lit_config.killProcessAndChildrenIsSupported:
     lit_config.maxIndividualTestTime = 600
----------------
Shouldn't this be

```lang=python
if lit_config.killProcessAndChildrenIsSupported():
```

?


================
Comment at: llvm/utils/lit/lit/LitConfig.py:80
 
+    def killProcessAndChildrenIsSupported(self):
+        """
----------------
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.

```lang=python
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64251/new/

https://reviews.llvm.org/D64251