From 4f5588caabb512f580e14a27c3d07ee82a8db953 Mon Sep 17 00:00:00 2001 From: Nicolas Simonds Date: Fri, 27 Jan 2023 10:56:27 -0800 Subject: [PATCH 1/2] Provide a more helpful error message on spawn() failures Make a good-faith effort to catch the `RuntimeError` thrown when improperly invoking the YumFinder on non-Linux platforms, and present a useful error message to the user. Drive-by: update `tox.ini` to work with 4.x Fixes: Issue #31 --- soufi/finders/yum.py | 16 +++++++++++++++- soufi/tests/finders/test_yum_finder.py | 10 ++++++++++ tox.ini | 14 +++++++++----- 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/soufi/finders/yum.py b/soufi/finders/yum.py index 67fcbe3..42faf8e 100644 --- a/soufi/finders/yum.py +++ b/soufi/finders/yum.py @@ -4,6 +4,7 @@ import abc import lzma import pickle # nosec +import sys import warnings from multiprocessing import Process, Queue from types import SimpleNamespace @@ -230,7 +231,20 @@ def do_task(target, *args): """Run the target callable in a subprocess and return its response.""" queue = Queue() process = Process(target=target, args=(queue,) + args) - process.start() + try: + process.start() + except RuntimeError: + sys.exit( + """ +FATAL: Running this finder directly from the global scope is not supported on + this platform. To use this finder, call it instead from the main + module, e.g.: + + if __name__ == '__main__': + soufi.finder.factory(*args, **kwargs).find() + +Aborting.""" + ) # We don't want to wait *forever*, but jobs can take several minutes to # complete, so wait a relatively long time response = queue.get(timeout=600) diff --git a/soufi/tests/finders/test_yum_finder.py b/soufi/tests/finders/test_yum_finder.py index 019938e..112f8aa 100644 --- a/soufi/tests/finders/test_yum_finder.py +++ b/soufi/tests/finders/test_yum_finder.py @@ -478,6 +478,16 @@ def test_do_task_reraises_exceptions(self): err = self.assertRaises(RuntimeError, yum.do_task, kaboom, data) self.assertEqual(data, str(err)) + def test_do_task_handles_spawn_errors_on_silly_platforms(self): + # This simulates calling `process.start()` from the global scope on + # platforms that do not support such things. The default traceback + # is not intrinsically helpful, so test that we kick back a more + # useful error message. See issue #31. + process = self.patch(yum, 'Process') + process.return_value.start.side_effect = RuntimeError + err = self.assertRaises(SystemExit, yum.do_task, None) + self.assertIn('FATAL: ', str(err)) + # A simple subprocess function that throws a test exception. Used by # TestYumFinderHelpers.test_do_task_reraises_exceptions diff --git a/tox.ini b/tox.ini index fa6e7fe..4b850f3 100644 --- a/tox.ini +++ b/tox.ini @@ -13,7 +13,7 @@ setenv = deps = -r{toxinidir}/requirements.txt -r{toxinidir}/test-requirements.txt -whitelist_externals = +allowlist_externals = bash echo mkdir @@ -94,10 +94,14 @@ exclude = .eggs show-source = true ignore = - D10 # ignore missing docstrings (for now) - D202 # No blank lines allowed after function docstring (caused by Black) - E203 # Whitespace before ':' (caused by Black) - W503 # line breaks after operators (caused by Black) + # ignore missing docstrings (for now) + D10 + # No blank lines allowed after function docstring (caused by Black) + D202 + # Whitespace before ':' (caused by Black) + E203 + # line breaks after operators (caused by Black) + W503 [testenv:functional] commands = From 3892035bc8ade236f0ca0f1b57e03d083bd38765 Mon Sep 17 00:00:00 2001 From: Nicolas Simonds Date: Mon, 30 Jan 2023 09:03:48 -0800 Subject: [PATCH 2/2] Minor cleanups based on review feedback --- soufi/finders/yum.py | 29 +++++++++++++++----------- soufi/tests/finders/test_yum_finder.py | 10 ++++++++- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/soufi/finders/yum.py b/soufi/finders/yum.py index 42faf8e..37694f6 100644 --- a/soufi/finders/yum.py +++ b/soufi/finders/yum.py @@ -5,6 +5,7 @@ import lzma import pickle # nosec import sys +import textwrap import warnings from multiprocessing import Process, Queue from types import SimpleNamespace @@ -233,18 +234,22 @@ def do_task(target, *args): process = Process(target=target, args=(queue,) + args) try: process.start() - except RuntimeError: - sys.exit( - """ -FATAL: Running this finder directly from the global scope is not supported on - this platform. To use this finder, call it instead from the main - module, e.g.: - - if __name__ == '__main__': - soufi.finder.factory(*args, **kwargs).find() - -Aborting.""" - ) + except RuntimeError as e: + if 'not using fork' in str(e): + sys.exit( + textwrap.dedent( + """ + FATAL: Running this finder directly from the global scope is + not supported on this platform. To use this finder, call it + instead from the main module, e.g.: + + if __name__ == '__main__': + soufi.finder.factory(*args, **kwargs).find() + + Aborting.""" + ) + ) + raise # We don't want to wait *forever*, but jobs can take several minutes to # complete, so wait a relatively long time response = queue.get(timeout=600) diff --git a/soufi/tests/finders/test_yum_finder.py b/soufi/tests/finders/test_yum_finder.py index 112f8aa..854ac8f 100644 --- a/soufi/tests/finders/test_yum_finder.py +++ b/soufi/tests/finders/test_yum_finder.py @@ -484,10 +484,18 @@ def test_do_task_handles_spawn_errors_on_silly_platforms(self): # is not intrinsically helpful, so test that we kick back a more # useful error message. See issue #31. process = self.patch(yum, 'Process') - process.return_value.start.side_effect = RuntimeError + process.return_value.start.side_effect = RuntimeError( + 'Simulating a platform that is not using fork to start children' + ) err = self.assertRaises(SystemExit, yum.do_task, None) self.assertIn('FATAL: ', str(err)) + def test_do_task_leaves_other_spawn_errors_alone(self): + # As per the above, other RuntimeError exceptions should pass through + process = self.patch(yum, 'Process') + process.return_value.start.side_effect = RuntimeError + self.assertRaises(RuntimeError, yum.do_task, None) + # A simple subprocess function that throws a test exception. Used by # TestYumFinderHelpers.test_do_task_reraises_exceptions