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

Update to support running on Windows #1617

Merged
merged 5 commits into from
Nov 14, 2023

Conversation

chhackett
Copy link
Contributor

Added support for running on Windows, in command line or Powershell terminals.

Currently terminal emulators such as mintty, ConEmu, alacritty, etc are not supported.

Addresses issues #1607 and #53.

Copy link
Member

@xsebek xsebek left a comment

Choose a reason for hiding this comment

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

Thanks @chhackett, I look forward to playing Swarm on Windows. 🚀

Could you please also update the CI file and add Windows to the test matrix?

src/Swarm/App.hs Outdated Show resolved Hide resolved
swarm.cabal Outdated
Comment on lines 321 to 324
if os(windows)
build-depends: vty-windows >= 0.1.0.3 && < 1.0
else
build-depends: vty-unix >= 0.1.0.0 && < 1.0
Copy link
Member

Choose a reason for hiding this comment

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

Why not use only vty-crossplatform? 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason for the explicit dependency on unix/windows libraries is because the ColorMode setting is not part of the default configuration anymore, but down in the unix/windows settings.

That is an unfortunate consequence of the design choice we made in how to split up the unix/windows implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So now that I'm thinking about this, I wonder if we made a mistake by putting ColorMode in the UnixSettings/WindowsSettings data types. It is something that we could potentially add to the VtyUserConfig
since both implementations support it.
That would make one less reason for users to need to have an explicit dependency on the unix/windows plugins.
@jtdaugherty - what do you think?

Choose a reason for hiding this comment

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

Looking back at the change history, this was an oversight. I moved the color mode detection code to vty-unix, but there was a lot going on with the split-up of Config that the color mode setting got moved in the shuffle. Since the ColorMode type lives in vty, it makes sense to me to put that in the Vty user config as well, even if the backend packages still do their own detection. I think the caveat would be that backends might need to ignore or override the setting if it's unsuitable, but that was already true before Vty 6.

Choose a reason for hiding this comment

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

I'll look into the implementation and see what this would involve.

@xsebek xsebek added the CHANGELOG Once merged, this PR should be highlighted in the changelog for the next release. label Nov 11, 2023
swarm.cabal Outdated Show resolved Hide resolved
@byorgey byorgey linked an issue Nov 11, 2023 that may be closed by this pull request
@chhackett
Copy link
Contributor Author

chhackett commented Nov 12, 2023

Thanks @chhackett, I look forward to playing Swarm on Windows. 🚀

Could you please also update the CI file and add Windows to the test matrix?

I'd be happy to do that, but I'm not sure what change I should make to the CI file (assuming you mean cabal.haskell-ci).

And I don't know where to find the test matrix. Sorry, I'm new to contributing to github projects.

@chhackett
Copy link
Contributor Author

@xsebek
OK, I've been reading about the haskell github CI project, and I'm guessing you were hoping I would add a Windows section to the /.github/workflows/haskell-ci.yml file?

Unfortunately, as far as I can tell, there is no Windows version. Am I missing something?

@p3rsik
Copy link
Collaborator

p3rsik commented Nov 14, 2023

@chhackett I believe you would want to define new job for windows(as done here) with it's own steps and compiler matrix for windows.

This line here specifies the runner OS. Unfortunately you can't reuse much of the setup code in the linux runner, since it's linux specific, but you can use it as a reference on what needs to be done on windows runner.

That said, it's a hard task, if you aren't familiar with github workflows and windows, albeit an interesting one and will definitely add something to your toolbox.

@chhackett
Copy link
Contributor Author

@p3rsik @xsebek
I would love to learn how to define a workflow job for windows. However, my time available for doing this kind of work is very limited. And right now I have my hands full working out some bugs in the vty-windows implementation. So I'm unlikely to get to this task for a while. So if anyone else has bandwidth and wants to take that on, please be my guest.

And just to be clear, I'm not promising I'll ever get around to doing this. I contributed this PR in the spirit of being helpful. But I want to stay focused on improving the experience of using vty/brick on Windows, at least for now.

@byorgey
Copy link
Member

byorgey commented Nov 14, 2023

@chhackett No worries! We really appreciate the contribution, and your work on vty + brick. One of the rest of us can definitely take on the task of setting up the CI for Windows.

@byorgey byorgey linked an issue Nov 14, 2023 that may be closed by this pull request
@byorgey
Copy link
Member

byorgey commented Nov 14, 2023

Incidentally, the .github/workflows/haskell-ci.yml file should not be edited directly, it is auto-generated by https://github.com/haskell-CI/haskell-ci . Now that I look at it, though, I am not sure whether haskell-ci has support for testing on Windows.

@xsebek
Copy link
Member

xsebek commented Nov 14, 2023

I forgot it was so linux specific, we can look into it separately - let's just add an Issue for Windows CI.

@byorgey byorgey mentioned this pull request Nov 14, 2023
@byorgey
Copy link
Member

byorgey commented Nov 14, 2023

So it sounds like there may be changes coming which would allow us to depend only on vty-crossplatform instead of having a conditional dependency on one of vty-unix and vty-windows. Should we wait for that, or should we go ahead and merge now and make those updates later? @jtdaugherty , any thoughts on a potential timeline on your end?

@jtdaugherty
Copy link

@byorgey I plan on looking into it more tonight. I started working on a patch earlier today, but it turns out that it's awkward to have a setting in vty's VtyUserConfig that vty itself doesn't do anything with -- the platform packages will need to make use of it -- so I'm not yet clear on what a good design will turn out to be. If it isn't too much trouble, I'd say go ahead and move forward with what you have and then adjust later. I wouldn't want to hold up your work unless you find the patch unpalatable and would like to hold off in the hope of something better.

@byorgey
Copy link
Member

byorgey commented Nov 14, 2023

@jtdaugherty OK, thanks, no rush. We'll go ahead and merge this, but I'll add an issue to remind us to possibly update in the future. Conceptually, I'd love to eventually be able to depend only on vty-crossplatform and thus have a sort of static guarantee that we are limited to doing things that are supported on both platforms. But I don't have a problem with the conditional platform-specific dependencies for now.

@jtdaugherty
Copy link

Understood, and I definitely don't see depending on specific platform packages as a good thing, either, if you're going for cross-platform support. So I will aim for something that removes that requirement.

@byorgey byorgey changed the title Updating to support running on Windows. Update to support running on Windows Nov 14, 2023
@byorgey byorgey added the merge me Trigger the merge process of the Pull request. label Nov 14, 2023
@mergify mergify bot merged commit ca1918a into swarm-game:main Nov 14, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CHANGELOG Once merged, this PR should be highlighted in the changelog for the next release. merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to brick 2.0 Windows support
5 participants