Skip to content

Improve run command#1428

Closed
maksbotan wants to merge 1 commit into
python-poetry:masterfrom
maksbotan:run-fix
Closed

Improve run command#1428
maksbotan wants to merge 1 commit into
python-poetry:masterfrom
maksbotan:run-fix

Conversation

@maksbotan
Copy link
Copy Markdown
Contributor

Currently "poetry run" command uses subprocess.call for starting
programs and scripts. This approach has a problem --- signals like
Ctrl-C are caught by poetry process itself and not by the process the
user has run.

This can be observed this way: run "poetry run python" and press Ctrl-C.
Normally Python interpreter will catch the signal and print
"KeyboardInterrupt" exception, continuing execution normally. But when
run by poetry, it exits immediately. This behavior is rather annoying,
as one has to restart interpreter after accidentially doing Ctrl-C.
Moreover, after python exits, it leaves terminal in a broken state,
requiring the user to use "reset" or similar command.

This commit fixes this behavior by using "os.execvp" instead of
subprocess. This replaces poetry process directly with target process,
eliminating an intermediate process.

This approach is used in other products like Pipenv and Haskell's build
system stack. See these links for examples and more info:

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

  • Added tests for changed code.
  • Updated documentation for changed code.

Note: If your Pull Request introduces a new feature or changes the current behavior, it should be based
on the develop branch. If it's a bug fix or only a documentation update, it should be based on the master branch.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

Notes

  • I'm unsure if it makes sense to add tests for this functionality. Right now there are no tests for run command in poetry.
  • Windows runs the old way, this issue seems not to be present there.
  • Also, it seems that nothing has to be added to the docs, as this a simple fix.

Currently "poetry run" command uses subprocess.call for starting
programs and scripts. This approach has a problem --- signals like
Ctrl-C are caught by poetry process itself and not by the process the
user has run.

This can be observed this way: run "poetry run python" and press Ctrl-C.
Normally Python interpreter will catch the signal and print
"KeyboardInterrupt" exception, continuing execution normally. But when
run by poetry, it exits immediately. This behavior is rather annoying,
as one has to restart interpreter after accidentially doing Ctrl-C.
Moreover, after python exits, it leaves terminal in a broken state,
requiring the user to use "reset" or similar command.

This commit fixes this behavior by using "os.execvp" instead of
subprocess. This replaces poetry process directly with target process,
eliminating an intermediate process.

This approach is used in other products like Pipenv and Haskell's build
system stack. See these links for examples and more info:

- https://github.com/pypa/pipenv/blob/master/pipenv/core.py#L2462
- commercialhaskell/stack#527
- https://hackage.haskell.org/package/rio-0.1.12.0/docs/RIO-Process.html#v:exec
@sdispater
Copy link
Copy Markdown
Member

Thanks for taking the time to make this PR!

However, this is already covered by #1236 so I am closing this one to avoid duplicates.

@sdispater sdispater closed this Oct 4, 2019
@maksbotan
Copy link
Copy Markdown
Contributor Author

Oh, great!
Sorry for that. I'm sure I tried searching github before making my pr, but did not find that one for some reason.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 1, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants