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

Ensure command names in search path of run() or spawn() commands do not match folders. #37

Closed
wants to merge 6 commits into from

Conversation

lcm337
Copy link

@lcm337 lcm337 commented Feb 4, 2014

fix for incorrect directory match

fix for incorrect directory match
@takluyver
Copy link
Member

That makes sense. Could you add a test for this in tests/test_misc.py?

if platform.system() == "Linux":
exe = "ls"
elif platform.system() == "Windows":
exe = "cmd.exe"
Copy link
Member

Choose a reason for hiding this comment

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

No need for this check - pexpect only works on Unix-y systems anyway.

@takluyver
Copy link
Member

@lcm337 , I'll be happy to merge this if you fix the couple of things I pointed out - the change to command lookup, and the unnecessary Windows test code.

@lcm337
Copy link
Author

lcm337 commented Feb 14, 2014

Will do.
On 13 Feb 2014 19:13, "Thomas Kluyver" notifications@github.com wrote:

@lcm337 https://github.com/lcm337 , I'll be happy to merge this if you
fix the couple of things I pointed out - the change to command lookup, and
the unnecessary Windows test code.

Reply to this email directly or view it on GitHubhttps://github.com//pull/37#issuecomment-35014126
.

@takluyver
Copy link
Member

Ping.

elif platform.system() == "Windows":
exe = "cmd.exe"
else:
assert True, "Unknown platform"
Copy link
Member

Choose a reason for hiding this comment

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

weird, wouldn't this never fail?

>>> assert True, 'never fails'
>>>

@jquast
Copy link
Member

jquast commented Mar 19, 2014

just got to this to say i like the isdir() check. the test is kindof ok.
The ENV thing is pretty strange, I don't get it. this is to help with testing?

This could be done with mock -- or more simply, overriden.
The child process is an exact duplicate of the parent, environment values and all.

os.environ['PATH'] = os.path.pathsep.join(('/XYZZY', os.environ['PATH']))
if not os.fork():
    time.sleep(0.1)
    print('child: {}'.format(os.environ['PATH']))
else:
    print('parent: {}'.format(os.environ['PATH']))

outputs

parent: /XYZZY:/Users/dingo1/.virtualenvs/pexpect/bin:/usr/local/bin:/usr/local/share/python3:/usr/local/share/python:/Users/dingo1/bin:/usr/local/sbin:/usr/sbin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/opt/X11/bin
child: /XYZZY:/Users/dingo1/.virtualenvs/pexpect/bin:/usr/local/bin:/usr/local/share/python3:/usr/local/share/python:/Users/dingo1/bin:/usr/local/sbin:/usr/sbin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/opt/X11/bin

@jquast jquast changed the title Update __init__.py Ensure command names in search path of run() or spawn() commands do not match folders. Mar 19, 2014
@jquast
Copy link
Member

jquast commented Mar 19, 2014

does sh have the same bug? Interesting.

https://github.com/amoffat/sh/blob/master/sh.py#L193

@jquast
Copy link
Member

jquast commented May 5, 2014

just submitted a similar fix to sh. I can see @lcm337 timed out regarding your suggestions, will pick it up.

jquast added a commit that referenced this pull request May 25, 2014
@jquast
Copy link
Member

jquast commented May 25, 2014

Closed by pull #58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants