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

Add PTY-Support for windows #975

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

DavidDotCheck
Copy link

@DavidDotCheck DavidDotCheck commented Nov 5, 2023

Summary:

As mentioned in #561, PTY is currently missing support on windows, which somewhat limits invoke's usefulness for windows users.
I quickly threw together a PR to demo the changes necessary, which I present here as a starting of point.

Changes:

  • I added a conditional import to use pywinpty on Windows.
  • I added requirements for pywinpty and windows-curses to dev-requirements.txt
  • I've made the changes necessary successfully to run a quick example on widows

Notes:

  • Because I don't have a lot of time, this PR work in progress and serves as a proof of concept
  • Right now it looks like this project doesn't have any other non-dev dependencies.
  • In the end, conditionally importing the additional dependencies on windows would be for the best. For example:
dependencies = [
    "pywinpty; sys_platform == 'win32'",
    "windows-curses; sys_platform == 'win32'",
]

Testing:

  • On windows, I ran a quick test invoke in a different project using pty=true and false to make sure the changes worked as expected
  • I still have to run test's on other platforms, but I will use the CI workflow as a starting of point for that.

If there is any interest in fixing this, I'd be happy for any help, feedback or changes. I'm strapped for time with other projects as it stands unfortunately, so feel free to help out!

@tpjnorton
Copy link

Hi @DavidDotCheck - do you think you'll finish this PR up? Or is it just for demo purposes?

@DavidDotCheck
Copy link
Author

DavidDotCheck commented Jan 3, 2024

It's just for demo purposes since I didn't get any feedback or interest. I'm open to polishing it up and finishing the PR, but there are a few questions and problems in the way for now so until it get's some momentum or help, it will probably take a while.
As it stands, the changes are small and concise enough, that a more involved contributor could probably help iron out the issues far faster.
On the top of my hat, still left is:

  • Figure out if adding a dependency (pywinpty) is possible in the first place and if my method would be ok
  • Polish up the logic, formatting and structure since I put little effort into them yet
  • Get some more insight on the CI, CD and testing workflows of this project
  • And lastly, make sure that Linux is still supported and all tests pass

I also had to make a lot of unrelated changes to the code and encountered quite a few errors and bugs that I had to fix, which makes me wonder if I set up my environment wrong.

@tpjnorton
Copy link

Okay, thanks for the detailed update!

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