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

[David Tenty via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL366912: [AIX][lit] Dont depend on psutil on AIX 
(authored by daltenty, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D64251?vs=211104=211511#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64251

Files:
  libcxx/trunk/utils/libcxx/util.py
  lldb/trunk/lit/lit.cfg.py
  llvm/trunk/utils/lit/lit/LitConfig.py
  llvm/trunk/utils/lit/lit/util.py
  llvm/trunk/utils/lit/tests/googletest-timeout.py
  llvm/trunk/utils/lit/tests/lit.cfg
  llvm/trunk/utils/lit/tests/shtest-timeout.py

Index: llvm/trunk/utils/lit/lit/LitConfig.py
===
--- llvm/trunk/utils/lit/lit/LitConfig.py
+++ llvm/trunk/utils/lit/lit/LitConfig.py
@@ -1,6 +1,7 @@
 from __future__ import absolute_import
 import inspect
 import os
+import platform
 import sys
 
 import lit.Test
@@ -76,6 +77,19 @@
 """
 return self._maxIndividualTestTime
 
+@property
+def maxIndividualTestTimeIsSupported(self):
+"""
+Returns a tuple ( , )
+where
+`` is True if setting maxIndividualTestTime is supported
+on the current host, returns False otherwise.
+`` is an empty string if `` is True,
+otherwise is contains a string describing why setting
+maxIndividualTestTime is not supported.
+"""
+return lit.util.killProcessAndChildrenIsSupported()
+
 @maxIndividualTestTime.setter
 def maxIndividualTestTime(self, value):
 """
@@ -86,16 +100,13 @@
 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')
 
Index: llvm/trunk/utils/lit/lit/util.py
===
--- llvm/trunk/utils/lit/lit/util.py
+++ llvm/trunk/utils/lit/lit/util.py
@@ -423,34 +423,56 @@
 return out.decode()
 return None
 
+def killProcessAndChildrenIsSupported():
+"""
+Returns a tuple ( , )
+where
+`` is True if `killProcessAndChildren()` is supported on
+the current host, returns False otherwise.
+`` is an empty string if `` 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.
 
 """
-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)
+else:
+import psutil
 try:
-# psutil >= 2.x
-children_iterator = psutilProc.children(recursive=True)
-except AttributeError:
-# 

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

[Dan Liew via Phabricator via lldb-commits]

delcypher accepted this revision.
delcypher added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[David Tenty via Phabricator via lldb-commits]

daltenty updated this revision to Diff 211104.
daltenty added a comment.

- Fix typo in comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251

Files:
  libcxx/utils/libcxx/util.py
  lldb/lit/lit.cfg.py
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/tests/googletest-timeout.py
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/lit/tests/shtest-timeout.py

Index: llvm/utils/lit/tests/shtest-timeout.py
===
--- llvm/utils/lit/tests/shtest-timeout.py
+++ llvm/utils/lit/tests/shtest-timeout.py
@@ -1,4 +1,4 @@
-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # llvm.org/PR33944
 # UNSUPPORTED: system-windows
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -1,6 +1,7 @@
 # -*- Python -*-
 
 import os
+import platform
 import sys
 
 import lit.formats
@@ -56,10 +57,10 @@
 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.')
Index: llvm/utils/lit/tests/googletest-timeout.py
===
--- llvm/utils/lit/tests/googletest-timeout.py
+++ llvm/utils/lit/tests/googletest-timeout.py
@@ -1,4 +1,4 @@
-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # Check that the per test timeout is enforced when running GTest tests.
 #
Index: llvm/utils/lit/lit/util.py
===
--- llvm/utils/lit/lit/util.py
+++ llvm/utils/lit/lit/util.py
@@ -423,34 +423,56 @@
 return out.decode()
 return None
 
+def killProcessAndChildrenIsSupported():
+"""
+Returns a tuple ( , )
+where
+`` is True if `killProcessAndChildren()` is supported on
+the current host, returns False otherwise.
+`` is an empty string if `` 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.
 
 """
-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)
+else:
+import psutil
 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:
+psutilProc = psutil.Process(pid)
+# Handle the different psutil API versions
 try:
-child.kill()
-except psutil.NoSuchProcess:
-pass
-psutilProc.kill()
-except psutil.NoSuchProcess:
-pass
+# psutil >= 2.x
+children_iterator = psutilProc.children(recursive=True)
+except AttributeError:
+# psutil 1.x
+children_iterator = psutilProc.get_children(recursive=True)
+

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

[Dan Liew via Phabricator via lldb-commits]

delcypher added a comment.

@daltenty Other than the minor nit, LGTM.




Comment at: llvm/utils/lit/lit/util.py:449
+(recursively). It is currently implemented using the psutil module on some
+plaftorms which provides a simple platform neutral implementation.
 

Minor nit. s/plaftorms/platforms/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[David Tenty via Phabricator via lldb-commits]

daltenty updated this revision to Diff 209621.
daltenty added a comment.

- Add a space to warning in LLDB lit config


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251

Files:
  libcxx/utils/libcxx/util.py
  lldb/lit/lit.cfg.py
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/tests/googletest-timeout.py
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/lit/tests/shtest-timeout.py

Index: llvm/utils/lit/tests/shtest-timeout.py
===
--- llvm/utils/lit/tests/shtest-timeout.py
+++ llvm/utils/lit/tests/shtest-timeout.py
@@ -1,4 +1,4 @@
-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # llvm.org/PR33944
 # UNSUPPORTED: system-windows
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -1,6 +1,7 @@
 # -*- Python -*-
 
 import os
+import platform
 import sys
 
 import lit.formats
@@ -56,10 +57,10 @@
 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.')
Index: llvm/utils/lit/tests/googletest-timeout.py
===
--- llvm/utils/lit/tests/googletest-timeout.py
+++ llvm/utils/lit/tests/googletest-timeout.py
@@ -1,4 +1,4 @@
-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # Check that the per test timeout is enforced when running GTest tests.
 #
Index: llvm/utils/lit/lit/util.py
===
--- llvm/utils/lit/lit/util.py
+++ llvm/utils/lit/lit/util.py
@@ -423,34 +423,56 @@
 return out.decode()
 return None
 
+def killProcessAndChildrenIsSupported():
+"""
+Returns a tuple ( , )
+where
+`` is True if `killProcessAndChildren()` is supported on
+the current host, returns False otherwise.
+`` is an empty string if `` 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
+plaftorms 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.
 
 """
-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)
+else:
+import psutil
 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:
+psutilProc = psutil.Process(pid)
+# Handle the different psutil API versions
 try:
-child.kill()
-except psutil.NoSuchProcess:
-pass
-psutilProc.kill()
-except psutil.NoSuchProcess:
-pass
+# psutil >= 2.x
+children_iterator = psutilProc.children(recursive=True)
+except AttributeError:
+# psutil 1.x
+children_iterator = 

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

[David Tenty via Phabricator via lldb-commits]

daltenty updated this revision to Diff 209619.
daltenty marked an inline comment as done.
daltenty added a comment.

- Address review comments round 3


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251

Files:
  libcxx/utils/libcxx/util.py
  lldb/lit/lit.cfg.py
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/tests/googletest-timeout.py
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/lit/tests/shtest-timeout.py

Index: llvm/utils/lit/tests/shtest-timeout.py
===
--- llvm/utils/lit/tests/shtest-timeout.py
+++ llvm/utils/lit/tests/shtest-timeout.py
@@ -1,4 +1,4 @@
-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # llvm.org/PR33944
 # UNSUPPORTED: system-windows
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -1,6 +1,7 @@
 # -*- Python -*-
 
 import os
+import platform
 import sys
 
 import lit.formats
@@ -56,10 +57,10 @@
 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.')
Index: llvm/utils/lit/tests/googletest-timeout.py
===
--- llvm/utils/lit/tests/googletest-timeout.py
+++ llvm/utils/lit/tests/googletest-timeout.py
@@ -1,4 +1,4 @@
-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # Check that the per test timeout is enforced when running GTest tests.
 #
Index: llvm/utils/lit/lit/util.py
===
--- llvm/utils/lit/lit/util.py
+++ llvm/utils/lit/lit/util.py
@@ -423,34 +423,56 @@
 return out.decode()
 return None
 
+def killProcessAndChildrenIsSupported():
+"""
+Returns a tuple ( , )
+where
+`` is True if `killProcessAndChildren()` is supported on
+the current host, returns False otherwise.
+`` is an empty string if `` 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
+plaftorms 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.
 
 """
-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)
+else:
+import psutil
 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:
+psutilProc = psutil.Process(pid)
+# Handle the different psutil API versions
 try:
-child.kill()
-except psutil.NoSuchProcess:
-pass
-psutilProc.kill()
-except psutil.NoSuchProcess:
-pass
+# psutil >= 2.x
+children_iterator = psutilProc.children(recursive=True)
+except AttributeError:
+# psutil 1.x
+children_iterator = 

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

[David Tenty via Phabricator via lldb-commits]

daltenty marked 6 inline comments as done.
daltenty added inline comments.



Comment at: llvm/utils/lit/lit/util.py:426
 
+def killProcessAndChildrenIsSupported(llvm_config):
+"""

delcypher wrote:
> 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.
I don't really think it's necessary, this was just to mimic the previous 
behavior. I will remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Dan Liew via Phabricator via lldb-commits]

delcypher added inline comments.



Comment at: llvm/utils/lit/lit/LitConfig.py:81
+@property
+def killProcessAndChildrenIsSupported(self):
+"""

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.



Comment at: llvm/utils/lit/lit/util.py:426
 
+def killProcessAndChildrenIsSupported(llvm_config):
+"""

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[David Tenty via Phabricator via lldb-commits]

daltenty updated this revision to Diff 209409.
daltenty added a comment.

- Address review comments round 2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251

Files:
  libcxx/utils/libcxx/util.py
  lldb/lit/lit.cfg.py
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/tests/googletest-timeout.py
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/lit/tests/shtest-timeout.py

Index: llvm/utils/lit/tests/shtest-timeout.py
===
--- llvm/utils/lit/tests/shtest-timeout.py
+++ llvm/utils/lit/tests/shtest-timeout.py
@@ -1,4 +1,4 @@
-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # llvm.org/PR33944
 # UNSUPPORTED: system-windows
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -1,6 +1,7 @@
 # -*- Python -*-
 
 import os
+import platform
 import sys
 
 import lit.formats
@@ -56,10 +57,10 @@
 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.killProcessAndChildrenIsSupported
+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.')
Index: llvm/utils/lit/tests/googletest-timeout.py
===
--- llvm/utils/lit/tests/googletest-timeout.py
+++ llvm/utils/lit/tests/googletest-timeout.py
@@ -1,4 +1,4 @@
-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # Check that the per test timeout is enforced when running GTest tests.
 #
Index: llvm/utils/lit/lit/util.py
===
--- llvm/utils/lit/lit/util.py
+++ llvm/utils/lit/lit/util.py
@@ -423,6 +423,26 @@
 return out
 return None
 
+def killProcessAndChildrenIsSupported(llvm_config):
+"""
+Returns a tuple ( , )
+where
+`` is True if `killProcessAndChildren()` is supported on
+the current host, returns False otherwise.
+`` is an empty string if `` 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
+llvm_config.note('Found python psutil module')
+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
@@ -433,24 +453,27 @@
 our dependency on it.
 
 """
-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)
+else:
+import psutil
 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:
+psutilProc = psutil.Process(pid)
+# Handle the different psutil API versions
 try:
-child.kill()
-except psutil.NoSuchProcess:
-pass
-psutilProc.kill()
-except psutil.NoSuchProcess:
-pass
+# 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:
Index: llvm/utils/lit/lit/LitConfig.py
===
--- 

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

[David Tenty via Phabricator via lldb-commits]

daltenty added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Davide Italiano via Phabricator via lldb-commits]

davide requested changes to this revision.
davide added a comment.
This revision now requires changes to proceed.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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 ( , )
where
`` is True if `killProcessAndChildren()` is supported on
the current host, returns False otherwise.
`` is an empty string if `` 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



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[David Tenty via Phabricator via lldb-commits]

daltenty updated this revision to Diff 208805.
daltenty added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251

Files:
  libcxx/utils/libcxx/util.py
  lldb/lit/lit.cfg.py
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/tests/googletest-timeout.py
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/lit/tests/shtest-timeout.py

Index: llvm/utils/lit/tests/shtest-timeout.py
===
--- llvm/utils/lit/tests/shtest-timeout.py
+++ llvm/utils/lit/tests/shtest-timeout.py
@@ -1,4 +1,4 @@
-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # llvm.org/PR33944
 # UNSUPPORTED: system-windows
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -1,6 +1,7 @@
 # -*- Python -*-
 
 import os
+import platform
 import sys
 
 import lit.formats
@@ -56,10 +57,10 @@
 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.killProcessAndChildrenIsSupported()
+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.')
Index: llvm/utils/lit/tests/googletest-timeout.py
===
--- llvm/utils/lit/tests/googletest-timeout.py
+++ llvm/utils/lit/tests/googletest-timeout.py
@@ -1,4 +1,4 @@
-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # Check that the per test timeout is enforced when running GTest tests.
 #
Index: llvm/utils/lit/lit/util.py
===
--- llvm/utils/lit/lit/util.py
+++ llvm/utils/lit/lit/util.py
@@ -433,24 +433,29 @@
 our dependency on it.
 
 """
-import psutil
-try:
-psutilProc = psutil.Process(pid)
-# Handle the different psutil API versions
+import platform
+
+if platform.system() == 'AIX':
+subprocess.call('kill -kill $(ps -o pid= -L{})'.format(pid), shell=True)
+else:
+import psutil
 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:
+psutilProc = psutil.Process(pid)
+# Handle the different psutil API versions
 try:
-child.kill()
-except psutil.NoSuchProcess:
-pass
-psutilProc.kill()
-except psutil.NoSuchProcess:
-pass
+# 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:
Index: llvm/utils/lit/lit/LitConfig.py
===
--- llvm/utils/lit/lit/LitConfig.py
+++ llvm/utils/lit/lit/LitConfig.py
@@ -1,6 +1,7 @@
 from __future__ import absolute_import
 import inspect
 import os
+import platform
 import sys
 
 import lit.Test
@@ -76,6 +77,27 @@
 """
 return self._maxIndividualTestTime
 
+def killProcessAndChildrenIsSupported(self):
+"""
+Returns a tuple ( , )
+where
+`` is True if `killProcessAndChildren()` is supported on
+the current host, returns False otherwise.
+`` is an empty string if `` 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
+self.note('Found python psutil module')
+return 

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

[Dan Liew via Phabricator via lldb-commits]

delcypher added inline comments.



Comment at: llvm/utils/lit/lit/LitConfig.py:93
 # 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.")
+if platform.system() != 'AIX':
+try:

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

```lang=python
def killProcessAndChildrenIsSupported():
   """
  Returns a tuple ( , )
  where 
   `` is True if `killProcessAndChildren()` is supported on the 
current host, returns False otherwise.
  `` is an empty string if `` 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.



Comment at: llvm/utils/lit/lit/util.py:440
+from subprocess import call
+call('kill -kill $(ps -o pid= -L{})'.format(pid), shell=True)
+else:

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)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Dan Liew via Phabricator via lldb-commits]

delcypher added a comment.

@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.




Comment at: llvm/utils/lit/tests/lit.cfg:62
-lit_config.note('Found python psutil module')
-config.available_features.add("python-psutil")
 except ImportError:

hubert.reinterpretcast wrote:
> 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?
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 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Hubert Tong via Phabricator via lldb-commits]

hubert.reinterpretcast added inline comments.



Comment at: llvm/utils/lit/lit/LitConfig.py:98
+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"

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



Comment at: llvm/utils/lit/tests/lit.cfg:62
-lit_config.note('Found python psutil module')
-config.available_features.add("python-psutil")
 except ImportError:

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64251



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[David Tenty via Phabricator via lldb-commits]

daltenty created this revision.
Herald added subscribers: llvm-commits, libcxx-commits, lldb-commits, christof, 
delcypher.
Herald added projects: LLDB, libc++, LLVM.

On AIX psutil can run into problems with permissions to read the process
tree, which causes problems for python tests. This patch adds a workaround
by invoking shell via subprocess and a platform specific
option to ps to list all the decendant processes so we can kill them. This 
removes our dependency on psutil.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64251

Files:
  libcxx/utils/libcxx/util.py
  lldb/lit/lit.cfg.py
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/tests/googletest-timeout.py
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/lit/tests/shtest-timeout.py

Index: llvm/utils/lit/tests/shtest-timeout.py
===
--- llvm/utils/lit/tests/shtest-timeout.py
+++ llvm/utils/lit/tests/shtest-timeout.py
@@ -1,4 +1,4 @@
-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # llvm.org/PR33944
 # UNSUPPORTED: system-windows
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -1,6 +1,7 @@
 # -*- Python -*-
 
 import os
+import platform
 import sys
 
 import lit.formats
@@ -57,9 +58,10 @@
 
 # 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")
+if platform.system() != 'AIX':
+import psutil
+lit_config.note('Found python psutil module')
+config.available_features.add("lit-max-individual-test-time")
 except ImportError:
 lit_config.warning('Could not import psutil. Some tests will be skipped and'
' the --timeout command line argument will not work.')
Index: llvm/utils/lit/tests/googletest-timeout.py
===
--- llvm/utils/lit/tests/googletest-timeout.py
+++ llvm/utils/lit/tests/googletest-timeout.py
@@ -1,4 +1,4 @@
-# REQUIRES: python-psutil
+# REQUIRES: lit-max-individual-test-time
 
 # Check that the per test timeout is enforced when running GTest tests.
 #
Index: llvm/utils/lit/lit/util.py
===
--- llvm/utils/lit/lit/util.py
+++ llvm/utils/lit/lit/util.py
@@ -433,24 +433,30 @@
 our dependency on it.
 
 """
-import psutil
-try:
-psutilProc = psutil.Process(pid)
-# Handle the different psutil API versions
+import platform
+
+if platform.system() == 'AIX':
+from subprocess import call
+call('kill -kill $(ps -o pid= -L{})'.format(pid), shell=True)
+else:
+import psutil
 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:
+psutilProc = psutil.Process(pid)
+# Handle the different psutil API versions
 try:
-child.kill()
-except psutil.NoSuchProcess:
-pass
-psutilProc.kill()
-except psutil.NoSuchProcess:
-pass
+# 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:
Index: llvm/utils/lit/lit/LitConfig.py
===
--- llvm/utils/lit/lit/LitConfig.py
+++ llvm/utils/lit/lit/LitConfig.py
@@ -1,6 +1,7 @@
 from __future__ import absolute_import
 import inspect
 import os
+import platform
 import sys
 
 import lit.Test
@@ -86,16 +87,17 @@
 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"
-