use os.execl to "run" subcommand on Unix ( close #315 ) #320

Merged
merged 3 commits into from Apr 25, 2017

Conversation

Projects
None yet
2 participants
@nonylene
Contributor

nonylene commented Apr 19, 2017

see #315.

Pipe and redirection are working well in my Linux and Windows computer.

nonylene added some commits Apr 19, 2017

@nateprewitt

Thanks for putting this together @nonylene! This seems like a reasonable first step but I do have a couple outstanding questions.

Before we remove --no-interactive I think we should definitely make sure that this code works on Docker, particularly Alpine Linux which this was added for. Also running this inside of nested shell calls should be verified.

I'd also like @kennethreitz to give this a go ahead before we remove --no-interactive. That's technically a breaking change so we should do a 4.0.0 bump, but I don't know if he has thoughts on that. We'd also want to get as many breaking changes together as we can before merging this.

else:
- c.interact()
- c.close()
- sys.exit(c.exitstatus)

This comment has been minimized.

@nateprewitt

nateprewitt Apr 19, 2017

Member

When os.execl runs and the process fails, will this return an appropriate exit status?

@nateprewitt

nateprewitt Apr 19, 2017

Member

When os.execl runs and the process fails, will this return an appropriate exit status?

This comment has been minimized.

@nonylene

nonylene Apr 19, 2017

Contributor

yes.

$ python3 -m pipenv run python -c "exit()"

$ python3 -m pipenv run python -c "exit(1)"
zsh: exit 1     python3 -m pipenv run python -c "exit(1)"

$ python3 -m pipenv run python -c "exit(2)"
zsh: exit 2     python3 -m pipenv run python -c "exit(2)"

$ python3 -m pipenv run python -c "exit(3)"
zsh: exit 3     python3 -m pipenv run python -c "exit(3)"

$ python3 -m pipenv run python -c "exit(3"
  File "<string>", line 1
    exit(3
         ^
SyntaxError: unexpected EOF while parsing
zsh: exit 1     python3 -m pipenv run python -c "exit(3"
@nonylene

nonylene Apr 19, 2017

Contributor

yes.

$ python3 -m pipenv run python -c "exit()"

$ python3 -m pipenv run python -c "exit(1)"
zsh: exit 1     python3 -m pipenv run python -c "exit(1)"

$ python3 -m pipenv run python -c "exit(2)"
zsh: exit 2     python3 -m pipenv run python -c "exit(2)"

$ python3 -m pipenv run python -c "exit(3)"
zsh: exit 3     python3 -m pipenv run python -c "exit(3)"

$ python3 -m pipenv run python -c "exit(3"
  File "<string>", line 1
    exit(3
         ^
SyntaxError: unexpected EOF while parsing
zsh: exit 1     python3 -m pipenv run python -c "exit(3"
pipenv/cli.py
sys.exit(1)
- # Windows!

This comment has been minimized.

@nateprewitt

nateprewitt Apr 19, 2017

Member

This is a minor nit, but any reason to move this comment inside the conditional? This project tends to have comments precede the code they're describing.

@nateprewitt

nateprewitt Apr 19, 2017

Member

This is a minor nit, but any reason to move this comment inside the conditional? This project tends to have comments precede the code they're describing.

This comment has been minimized.

@nonylene

nonylene Apr 19, 2017

Contributor

Oh, I have no reason!

fixed : 2fe5d2e

@nonylene

nonylene Apr 19, 2017

Contributor

Oh, I have no reason!

fixed : 2fe5d2e

@nateprewitt nateprewitt requested a review from kennethreitz Apr 19, 2017

@nateprewitt

Alright, things appear to be working as expected on Docker and Windows, so I'm going to call this good to go. Thanks so much @nonylene!

@nateprewitt nateprewitt merged commit 237e67f into pypa:master Apr 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nonylene nonylene deleted the nonylene:run-with-execl branch Apr 25, 2017

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