Skip to content

Conversation

anwilli5
Copy link
Contributor

@anwilli5 anwilli5 commented Oct 9, 2014

  • Adding more robust code to handle the case where the exec call within fails. Now, spawn will raise an exception if this happens.
  • Adding a test to ensure this exception is raised when an invalid binary is run

…hin spawn fails. Now, spawn will raise an exception if this happens.

 - Adding a test to ensure this exception is raised when an invalid binary is run
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to use the low-level os.open interface here - you should just be able to open a regular file object in binary mode - with open(fullpath, 'wb') as f:

@takluyver
Copy link
Member

Thanks for this, and especially for writing a test to go with it.

   pexpect#1 with the
   exception of the generic exec OSError handling - waiting
   on thoughts on how best to implement this.
@takluyver
Copy link
Member

Would it make more sense to pull it out with a regex or to send it again as, say, the first few bytes sent over the pipe?

I'd prefer to send it separately as the first few bytes, that way we're not relying on the way Python formats error messages. We're already using the struct module, so doing struct.pack('i', e.errno) + errbytes should work.

…is now the one returned from the child process exception
@anwilli5
Copy link
Contributor Author

Thanks for the feedback - my python dev experience isn't too extensive, so I'm still learning the best ways of doing things. :)

Copy link
Member

Choose a reason for hiding this comment

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

Theoretically, some infintesimal fraction of the time, this could accidentally generate a valid executable and do something random on your computer. I expect that's massively unlikely, but I might hardcode some nonsense bytes so it's always the same.

@takluyver
Copy link
Member

You're welcome. There's just a couple more little things I spotted with the test, then I think this is good to go.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I think b'\x00\x00' should work on both Python versions.

Copy link
Member

Choose a reason for hiding this comment

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

(It wouldn't work on Python 2.5 or older, but I'm not interested in supporting that)

@takluyver
Copy link
Member

Thanks! Merging this now.

takluyver added a commit that referenced this pull request Oct 13, 2014
Potential fix for 'exec' failure case
@takluyver takluyver merged commit 9f0cfbe into pexpect:master Oct 13, 2014
@takluyver
Copy link
Member

Hmmm, tests are failing on my machine, with AssertionError: OSError was not raised. Is it working for you?

@anwilli5
Copy link
Contributor Author

Weird... Yes, the tests pass for me:
platform linux -- Python 3.4.0 -- py-1.4.25 -- pytest-2.6.3
platform linux2 -- Python 2.7.6 -- py-1.4.25 -- pytest-2.6.3

Just as a sanity check, is the latest working directory in your PYTHONPATH?

@takluyver
Copy link
Member

Ah, you're right, it was testing against an older version. D'oh. Normally py.test automatically adds the working directory to sys.path - I don't know why it's not at the moment.

@takluyver
Copy link
Member

Got it: if the tests are a package, i.e. with an __init__.py, py.test adds the parent directory to sys.path. If they're just a bunch of Python files in a directory, it doesn't.

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.

2 participants