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

Depend on only vty-crossplatform #1623

Closed
byorgey opened this issue Nov 14, 2023 · 10 comments · Fixed by #1727
Closed

Depend on only vty-crossplatform #1623

byorgey opened this issue Nov 14, 2023 · 10 comments · Fixed by #1727
Labels
P-Linux Issue affecting the Linux platform. P-Windows Issue affecting the Windows platform. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. Z-Dependencies Issues related to packages we depend on, which versions or features we are using, etc.

Comments

@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.

Originally posted by @byorgey in #1617 (comment)

See the rest of the discussion on #1617 for more context. Currently, we conditionally depend on either vty-unix or vty-windows; ideally, if the vty packages are updated in an appropriate way, we will be able to depend only on vty-crossplatform.

@byorgey byorgey added S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. Z-Meta This issue is about other Swarm issues or infrastructure. P-Linux Issue affecting the Linux platform. P-Windows Issue affecting the Windows platform. Z-Dependencies Issues related to packages we depend on, which versions or features we are using, etc. and removed Z-Meta This issue is about other Swarm issues or infrastructure. labels Nov 14, 2023
byorgey added a commit that referenced this issue Nov 15, 2023
Closes #1624.

Note, however, that currently (until #1623) this will not work on
Windows, since I explicitly added `vty-unix` as an `extra-dep` in the
`stack.yaml` file.  However, I expect we will get #1623 soon and then
we can remove it.
@byorgey
Copy link
Member Author

byorgey commented Nov 15, 2023

Don't forget to remove vty-unix from stack.yaml!

mergify bot pushed a commit that referenced this issue Nov 15, 2023
Closes #1624.

Note, however, that currently (until #1623) this will not work on Windows, since I explicitly added `vty-unix` as an `extra-dep` in the `stack.yaml` file.  However, I expect we will get #1623 soon and then we can remove it.
@jtdaugherty
Copy link

@byorgey @chhackett here's a summary of what I have so far in some local uncommitted changes. Let me know what you think:

  • vty's VtyUserConfig type will get a new field, configPreferredColorMode :: Maybe ColorMode.
  • vty's user configuration file parser will start to support a new field, colorMode, whose value is one of the ColorMode choices. It's optional, and if it's absent it's understood that the backend driver will detect and choose an appropriate color mode. If it's present, the backend driver should honor the configured color mode regardless of what its detection finds.
  • vty-unix's UnixSettings type will have its settingColorMode field removed.
  • vty-unix's buildOutput function will start to take a VtyUserConfig in order to respect the configured setting, although this is not an end-user-facing API so that shouldn't matter.

Once the various packages have been updated, this ought to allow users to configure the color mode (provided each backend package respects the setting!) and should allow you to get away from a dependency on a specific backend package.

@byorgey
Copy link
Member Author

byorgey commented Nov 15, 2023

@jtdaugherty Sounds good to me.

@jtdaugherty
Copy link

This is now released in vty-6.1 and vty-unix-0.2.0.0.

@jtdaugherty
Copy link

It turns out that vty-crossplatform also needs to get updated so I'm working on that now. I forgot that it depends on the buildOutput functions from both platform packages for testing and the vty-unix buildOutput changed. The Windows version doesn't need to, necessarily, since I can deal with that with CPP.

@jtdaugherty
Copy link

vty-crossplatform-0.3.0.0 is out.

@chhackett
Copy link
Contributor

@jtdaugherty - i finished updating vty-windows. You can fix the Testing module now.

And for the swarm guys, you should be able to replace vty-windows and vty-unix dependencies with just vty-crossplatform now, just using the default mkVty function.

@jtdaugherty
Copy link

I've set a vty-windows upper bound on vty-crossplatform-0.3.0.0 and have also released vty-crossplatform-0.4.0.0 that uses vty-windows-0.2.0.0.

@byorgey
Copy link
Member Author

byorgey commented Nov 16, 2023

I don't see a vty-crossplatform-0.4.0.0 release on Hackage?

@jtdaugherty
Copy link

Whoops, forgot --publish. Fixed!

byorgey added a commit that referenced this issue Jan 13, 2024
Closes #1623.

Note that we still have to list both `vty-unix` and `vty-windows` in
`stack.yaml` but that's OK; it just means they are both made available
as extra dependencies, but only whichever is needed will actually be
installed.  In the codebase itself, we now get to avoid CPP on
imports, and the code is quite a bit simpler.
@mergify mergify bot closed this as completed in #1727 Jan 13, 2024
mergify bot pushed a commit that referenced this issue Jan 13, 2024
Closes #1623.

Note that we still have to list both `vty-unix` and `vty-windows` in `stack.yaml` but that's OK; it just means they are both made available as extra dependencies, but only whichever is needed will actually be installed.  In the codebase itself, we now get to avoid CPP on imports, and the code is quite a bit simpler.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-Linux Issue affecting the Linux platform. P-Windows Issue affecting the Windows platform. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. Z-Dependencies Issues related to packages we depend on, which versions or features we are using, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants