-
Notifications
You must be signed in to change notification settings - Fork 225
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
add hint if rclpy fails to be imported #116
Conversation
import os | ||
if os.path.isfile(e.path): | ||
import sys | ||
print( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use warnings module instead of printing? I think it would allow an advanced user to suppress the output, though I can't think of a use case for that.
try: | ||
rclpy_implementation = importlib.import_module(module_name, package='rclpy') | ||
except ImportError as e: | ||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import should at module level. No need to do it in this local scope. Same for sys
below.
print( | ||
"The module '%s' failed to be imported while being present on the system." | ||
" Please refer to '%s' for possible solutions\n" % | ||
(module_name, 'https://github.com/ros2/ros2/wiki/Rclpy-Import-error-hint'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather print e.path
here since module_name
is already part of the exception string. The message could start with "The C extension '%s' failed ..." then.
new message:
|
rclpy_implementation = importlib.import_module('._rclpy', package='rclpy') | ||
except ImportError as e: | ||
if os.path.isfile(e.path): | ||
raise ImportWarning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be an ImportWarning
. I thought @sloretz was referring to call warnings.warn(...)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I figured I just liked the idea of the message being the last thing the user see rather than having to scroll up the stack trace to see our message burried in the middle.
But I agree it's less semantically correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also extend the msg
of the raised exception e
to add additional information if you prefer a single message.
This reverts commit e32ece2.
if os.path.isfile(e.path): | ||
warnings.warn( | ||
"The C extension '%s' failed to be imported while being present on the system." | ||
" Please refer to '%s' for possible solutions\n" % |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the trailing \n
necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not necessary but made the custom message not visible at all originally that's why I added it. Not that it's part of the exception message it's not necessary anymore
if os.path.isfile(e.path): | ||
e.msg += \ | ||
"\nThe C extension '%s' failed to be imported while being present on the system." \ | ||
" Please refer to '%s' for possible solutions\n" % \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should this have a trailing newline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can you post the resulting stacktrace?
Resulting stack trace:
|
closes #115
connects to #115
Thew content of the referenced wiki page can be updated at a later date
Without this change
With this change
Alternatively we could not reraise the exception and exit. That will give a cleaner error message but has the downside of restricting user ability to catch it and fallback. So I settled on the current behavior
Without reraising