-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Enhance documentation and error logging for preexec_fn usage #787
Conversation
c901f19
to
8a087e9
Compare
Also fix other incorrect log usage Fixes Gallopsled#786
Assigned to @idolf to merge into stable |
@@ -667,8 +667,12 @@ def process(self, argv=None, executable=None, tty=True, cwd=None, env=None, time | |||
See ``stdin``. | |||
preexec_fn(callable): | |||
Function which is executed on the remote side before execve(). | |||
This **MUST** be a self-contained function -- it must perform | |||
all of its own imports, and cannot refer to variables outside | |||
its scope. |
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.
We could just check whether this is the case by inspecting the callable object.
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 can't, actually. And if you can, it's more complex than I care to worry about. If things break, the exception is thrown on the other side and all we have is the Python stack trace -- which appears as part of the tube
data. There's not a way around this that I'm aware of.
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 thought you chould just check if func_closure
is None/()
and if func_code.co_names
only contains names from __builtins__
.
However def f(): import x
also has x
in its co_names
. So I guess you are right.
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 am not doing a new 3.1.0 release, since the 3.2.0 release is done today. Changing the milestone to match. |
Target
stable
since this has real bug fixes for logging issues.Fixes #786