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

Restore POSIX support to fdpexpect. #359

Merged
merged 14 commits into from Jul 2, 2016

Conversation

Projects
None yet
3 participants
@MountainRider
Contributor

MountainRider commented Jun 28, 2016

Factor out uninterruptible select and put the code in utils as select_ignore_interrupts. Modify fdpexpect to use select_ignore_interrupts if os.name is posix. This should restore POSIX support without affecting Windows support.

MountainRider added some commits Jun 28, 2016

while True:
try:
return select.select(iwtd, owtd, ewtd, timeout)
except select.error:

This comment has been minimized.

@jquast

jquast Jun 28, 2016

Member

I wonder if we have python3 coverage here, in python 3 this raises InterruptedError exception, not select.error,

https://github.com/jquast/blessed/blob/master/blessed/terminal.py#L40
https://github.com/jquast/blessed/blob/master/blessed/terminal.py#L926

This comment has been minimized.

@MountainRider

MountainRider Jun 28, 2016

Contributor

I didn't write the code, I just moved it:
https://github.com/pexpect/pexpect/blob/master/pexpect/pty_spawn.py#L805
If it has problems on Python3 they were there before I moved it.

This comment has been minimized.

@jquast

jquast Jun 28, 2016

Member

Yes, I am only just making note of it for ourselves. Better in writing than back of my mind!

This comment has been minimized.

@MountainRider

MountainRider Jun 29, 2016

Contributor

ok. I followed the same pattern in the links you gave. We should now have Python3 coverage.

test_proc = multiprocessing.Process(target=socket_fn, args=(self,))
test_proc.daemon = True
test_proc.start()
time.sleep(5.0)

This comment has been minimized.

@jquast

jquast Jun 28, 2016

Member

not a fan of 5.0 second sleep, I have an slightly similar test, where I use semaphores between child/parent stdout, with multiprocessing library we have more simple primitives like https://docs.python.org/2/library/multiprocessing.html#multiprocessing.Event

https://github.com/jquast/blessed/blob/master/blessed/tests/test_keyboard.py#L38-L120

This comment has been minimized.

@MountainRider

MountainRider Jun 28, 2016

Contributor

Not a fan of global, but I can change the test to use an Event.

# This read should timeout
session.read_nonblocking(size=4096)
except pexpect.TIMEOUT:
result = 1

This comment has been minimized.

@jquast

jquast Jun 28, 2016

Member

suggest something more irregular, like exit 111 -- all python exceptions in child process will exit with a value of '1', so testing for '1' is not definitive. If we had a connection timeout or any other error, we would have a "positive" result without exercise

python -c 'raise RuntimeError("Some error")'; echo $? prints 1.

This comment has been minimized.

@MountainRider

MountainRider Jun 28, 2016

Contributor

Are you saying you want to see ECONNREFUSED? Or did you pick 111 out of a hat?
#define ECONNREFUSED 111 /* Connection refused */
If you really want something unique, I recommend something like 255.

This comment has been minimized.

@jquast

jquast Jun 28, 2016

Member

It is chosen out of a hat, it has no meaning. All python exceptions exit value 1, so we want to ensure we are testing something else, '2' would be just as well.

Side note, cannot use exit codes over 255 though, its a strict byte limitation, it just rolls around:

$ python -c 'exit(299)'; echo $?
43
@takluyver

This comment has been minimized.

Member

takluyver commented Jun 28, 2016

@MountainRider would you be interested in co-maintaining Pexpect? You seem to care about it and have a good handle on how it works. It's up to @jquast as well whether we add another co-maintainer, but I think it would be good to have another person on it.

@MountainRider

This comment has been minimized.

Contributor

MountainRider commented Jun 28, 2016

@takluyver: Yes, I would be interested. You folks have done a wonderful job of maintaining Pexpect, and I have been a free-rider user for a long time. It would be more than fair for me to donate some of my time to the project.

@takluyver

This comment has been minimized.

Member

takluyver commented Jun 28, 2016

Cool! @jquast, is that OK by you?

I'll ask for your real identity, because I'm not quite comfortable giving push access to an pseudonymous Github handle. It needn't be publicly visible if you prefer to go by the pseudonym: drop me an email (address on my profile) if you're more comfortable that way.

On Pexpect and all the shared-maintainership projects I work on, we try to work in pull requests for any non-trivial changes, giving each other a chance to review things. Pushing to master is OK for things like correcting simple typos.

@jquast

This comment has been minimized.

Member

jquast commented Jun 28, 2016

@takluyver I approve, agree also with everything @takluyver says.

Keep in mind that the project is in maintenance mode, only making new releases for bugfixes or performance. When we do our job well, we're making very few and very careful releases! Thanks for joining

@MountainRider

This comment has been minimized.

Contributor

MountainRider commented Jun 28, 2016

My name is David Cullen. I work for Technicolor in Lawrenceville, GA, USA. You can reach me at david.cullen@technicolor.com. If you want to talk on the phone, we can exchange numbers and I can arrange to pay the bill. I updated my profile so that it shows my real name and work e-mail.

@takluyver

This comment has been minimized.

Member

takluyver commented Jun 29, 2016

Thanks, that's good enough for me :-) You should be getting an invitation to join the team.

@MountainRider

This comment has been minimized.

Contributor

MountainRider commented Jun 29, 2016

Something seems broken with my pull request. The Python3 tests are failing on Travis CI, but they pass when I run them locally.

@takluyver

This comment has been minimized.

Member

takluyver commented Jun 29, 2016

Certain operations are unusually slow on Travis, so it can expose race conditions that it's very hard to hit normally.

@takluyver

This comment has been minimized.

Member

takluyver commented Jun 29, 2016

I see the tests are passing now. :-)

@MountainRider

This comment has been minimized.

Contributor

MountainRider commented Jun 29, 2016

ok. Everything finally passed. However, I would like some help getting coverage on this line:
https://coveralls.io/builds/6801173/source?filename=pexpect%2Futils.py#L141
I can't see the problem with the test. Any tips on how I can reach that line?

@takluyver

This comment has been minimized.

Member

takluyver commented Jun 29, 2016

I think that line can only be reached with a race condition - the select would have to be interrupted just before it times out, so that by the time time.time() is called a few lines later, it's after end_time. So I wouldn't worry about not covering it.

@MountainRider

This comment has been minimized.

Contributor

MountainRider commented Jun 29, 2016

ok. I rigged up a harness to try to force the race condition, but I was unsuccessful. I guess @takluyver and @jquast need to review my latest changes.

def read_nonblocking(self, size=1, timeout=-1):
'''The read_nonblocking method of fdspawn assumes that the file
will never block. This is not the case for all file like objects

This comment has been minimized.

@takluyver

takluyver Jun 29, 2016

Member

This docstring is a bit confusing - it would assume that if we weren't overriding it, but we are (on POSIX).

This comment has been minimized.

@MountainRider

MountainRider Jun 29, 2016

Contributor

Will this change fix the confusion:
'''The read_nonblocking method of SpawnBase assumes that the file

This comment has been minimized.

@takluyver

takluyver Jun 29, 2016

Member

It's better, but ideally the docstring should start by describing what this method does, rather than the method on the parent class. I like docstrings to follow the format:

One line overview of functionality
<blank line>
If necessary, more detailed description of options, return value, and rationale.
@takluyver

This comment has been minimized.

Member

takluyver commented Jun 29, 2016

This looks OK to me, though I haven't reviewed the new tests.

@MountainRider

This comment has been minimized.

Contributor

MountainRider commented Jun 29, 2016

This line look unreachable. Is this a copy-paste error?
https://github.com/pexpect/pexpect/blob/master/pexpect/pty_spawn.py#L406

@takluyver

This comment has been minimized.

Member

takluyver commented Jun 29, 2016

It does. I guess I overlooked it when separating ptyprocess out of pexpect. Feel free to remove it, since it clearly can't be reached.

@MountainRider

This comment has been minimized.

Contributor

MountainRider commented Jun 29, 2016

ok. What next?

@MountainRider

This comment has been minimized.

Contributor

MountainRider commented Jun 29, 2016

ok. I saw some more docstring feedback from you when I checked my email. I have updated the docstring for the read_nonblocking method of fdspawn. Hopefully, it meets or exceeds your expectations.

@takluyver

This comment has been minimized.

Member

takluyver commented Jun 30, 2016

Thanks, this is looking good. I'll give @jquast a chance to have another look, and then I think it can be merged.

@jquast

This comment has been minimized.

Member

jquast commented Jul 1, 2016

Looks great.

I'll add a changelog entry and prepare it for a version bump before merge to master in a bit after dinner.

Thanks again @MountainRider we appreciate the help!

@jquast jquast merged commit 248cb49 into pexpect:master Jul 2, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.6%) to 88.901%
Details

@jquast jquast referenced this pull request Jul 2, 2016

Merged

Prepare 4.2 release #361

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment