Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a more helpful error message on spawn() failures #34

Merged
merged 2 commits into from
Jan 30, 2023
Merged

Provide a more helpful error message on spawn() failures #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f5588c
Provide a more helpful error message on spawn() failures
nisimond Jan 27, 2023
3892035
Minor cleanups based on review feedback
nisimond Jan 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion soufi/finders/yum.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import abc
import lzma
import pickle # nosec
import sys
import textwrap
import warnings
from multiprocessing import Process, Queue
from types import SimpleNamespace
Expand Down Expand Up @@ -230,7 +232,24 @@ 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 as e:
if 'not using fork' in str(e):
sys.exit(
textwrap.dedent(
"""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI I would accept and prefer this:

textwrap.dedent("""\
    wibble
    blobble
""")

But no biggie.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, is that how you get Black to leave that alone? TIL...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it's how I've traditionally done it but who knows with Black these days...

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)
Expand Down
18 changes: 18 additions & 0 deletions soufi/tests/finders/test_yum_finder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,24 @@ 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(
'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
Expand Down
14 changes: 9 additions & 5 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ setenv =
deps = -r{toxinidir}/requirements.txt
-r{toxinidir}/test-requirements.txt

whitelist_externals =
allowlist_externals =
bash
echo
mkdir
Expand Down Expand Up @@ -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 =
Expand Down