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.

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

Conversation

0xDEC0DE
Copy link
Collaborator

@0xDEC0DE 0xDEC0DE commented Jan 27, 2023

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.

Fixes: Issue #31


This change is Reviewable

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.finder.factory(*args, **kwargs).find()

Aborting."""
)
Copy link
Owner

Choose a reason for hiding this comment

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

Since this seems to be limited to Macs, I'd be more inclined to add a sys.platform check somewhere, as this could could potentially spit out this error incorrectly if another different RuntimeError occurs.

Copy link
Owner

Choose a reason for hiding this comment

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

Also please consider using dedent so we can indent the error string :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The behavior will surface on any platform that uses spawn instead of fork in its multiprocessing module, which today means Windows and Mac, and could mean other platforms in the future.

But multiprocessing is prone to throw RuntimeErrors for lots of reasons, so a good-faith check that it's whinging about this particular problem is not unreasonable.

And I suppose calling textwrap.dedent is similarly not too much to ask for 😆

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

@juledwar juledwar merged commit 5c74049 into master Jan 30, 2023
@juledwar juledwar deleted the issue/31 branch January 30, 2023 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants