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

feat: migrate to vty-crossplatform and brick-2 #2128

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

ShrykeWindgrace
Copy link

@ShrykeWindgrace ShrykeWindgrace commented Dec 8, 2023

Addresses #2112

Incidentally, this adds support for hledger-ui for windows in Windows Terminal.

Current state of support of various features in different terminal emulators on Windows is tracked here: chhackett/vty-windows#15 with visualization here: https://github.com/chhackett/vty-windows/wiki/TerminalSupport (WIP). Rule of thumb - if emulator is mintty-based (defaults for cygwin, git bash, msys2), the app will fail. If it is conhost-based (Windows-Terminal, Wezterm) the app will work. If its a conhost-based-but-opinionated (Alacritty), almost everything will work.

I have not yet started proper documentation for hledger-ui, but I think that in most cases it will be safe to redirect them to vty-windows outright.

Also I have not touched CI/releases/packaging. The builds should pass for sufficiently recent GHC (I think ghc-9.4.4 should be enough. ghc-8.* most probably won't work).

@ShrykeWindgrace ShrykeWindgrace changed the title Migrate to vty-crossplatform and brick-2 feat: migrate to vty-crossplatform and brick-2 Dec 8, 2023
@simonmichael
Copy link
Owner

Very cool !

I see vty-windows is requiring GHC 9.4.4+. I'm thinking about this for hledger. It would be pleasant, but a big jump from 8.10 which I think would hurt our packageability ? Debian stable has 9.0 but Debian unstable has 9.4. No Ubuntu version has a GHC newer than 9.0. But I guess it wouldn't be the end of the world. 8.10 is 3.5y old (2020) and 9.4 is 1y old (2022).

@simonmichael simonmichael mentioned this pull request Dec 8, 2023
@ShrykeWindgrace
Copy link
Author

ShrykeWindgrace commented Dec 8, 2023

@simonmichael I did not take that aspect into account, to be honest. I know that scoop and chocolatey for windows just grab the latest released executables from github, so they do not care. Nix and its derivatives most probably compile on whatever default ghc they have in nixpkgs (was 9.4.8 yesterday morning). For more mainstream *nix distros (Ubuntu, Debian, Arch, ...) I have zero idea 😥

Come to think of that, it appears you don't have to worry about ghc-9.4.4 on *nix OSes at all. vty-crossplatform/vty-unix/vty all require base-4.8 (ghc-7-ish), and brick-2 requires base-4.9 (ghc-8-ish), so you should not hit the dependency on vty-windows with its ghc-9.4.4 requirements.

And on windows builders you are free to do whatever you want.

Copy link
Owner

@simonmichael simonmichael left a comment

Choose a reason for hiding this comment

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

What docs changes do you think are needed ? I think not much (I can help with that if needed).

I'm willing to require GHC 9.4 if the poll I just sent out doesn't turn up problems.

Looking good !

- vty >=5.15 && <6
- unix
- brick >=2.1.1 && <3
- vty >=6.1 && <7
Copy link
Owner

@simonmichael simonmichael Dec 8, 2023

Choose a reason for hiding this comment

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

API changes are not unknown in these packages, I think we'd better put tighter upper bounds (<2.2 <2.3, <6.2) even though it's a hassle.

Copy link
Owner

Choose a reason for hiding this comment

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

Still need these tighter upper bounds, though I can do it as a fixup.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least on Arch, brick 2.2 also works, so perhaps a bound of < 2.3 is also appropriate for brick. On Stackage, it's available in Stackage Nightly 2023-12-19.

Copy link
Author

Choose a reason for hiding this comment

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

brick-2.2 is an enhancement over 2.1; judging by the changelog, there are no API changes.

Copy link
Author

@ShrykeWindgrace ShrykeWindgrace Dec 19, 2023

Choose a reason for hiding this comment

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

API changes are not unknown in these packages, I think we'd better put tighter upper bounds (<2.2 <2.3, <6.2) even though it's a hassle.

Already did that for 2.2/6.2, can bump brick to <2.3 if you want.

edit, I think I got it - I most probably changed only yaml files, not cabal. Done!

hledger-ui/package.yaml Show resolved Hide resolved
stack.yaml Show resolved Hide resolved
@ShrykeWindgrace
Copy link
Author

What docs changes do you think are needed ? I think not much (I can help with that if needed).

Just mention that hledger-ui would not work in all terminal emulators people use on Windows.

I'm willing to require GHC 9.4 if the poll I just sent out doesn't turn up problems.

I edited my previous comment. It seems that would not be necessary for packaging on non-windows OSes.

Looking good !

Thanks! I'll iron out the extra-deps and dependencies for non-windows OSes soon(ish)=)

@simonmichael
Copy link
Owner

Come to think of that, it appears you don't have to worry about ghc-9.4.4 on *nix OSes at all.

Darn!

@simonmichael
Copy link
Owner

It looks like the right fix might be to add the newer packages in stack9.4.yaml.?

@Vekhir
Copy link
Contributor

Vekhir commented Dec 19, 2023

Hi, I'm in the process of updating the Arch packages to GHC 9.6 and hledger-ui is one of them. May I ask what is holding up this PR?

For my part, I can confirm that this patch works for Arch. I also tested with brick 2.2, which was released yesterday, and that works too. We are not using stack though.

-- Vekhir

@ShrykeWindgrace
Copy link
Author

Hi, I'm in the process of updating the Arch packages to GHC 9.6 and hledger-ui is one of them. May I ask what is holding up this PR?

Hi! Thanks for maintaining that package=) From my part - nothing software-related, just plain old lack of time because of end-of-the-year festivities and day job workload=) I'll try to move the PR forward this week.

Introduced or bumped dependencies:

- brick-2.1.1
- vty-6.1
- vty-crossplatform-0.4.0.0
- vty-windows-0.2.0.1 conditionally on windows (current version of
  vty-crossplatform has 0.2.0.0 as a lower bound, need to put a newer version explcitly;
  once we get a newer vty-crossplatform, we will be able to drop this conditional)
- vty-unix (indirectly via vty-crossplatform)
@ShrykeWindgrace
Copy link
Author

It looks like the right fix might be to add the newer packages in stack9.4.yaml.?

Just did that=)

Copy link
Owner

@simonmichael simonmichael left a comment

Choose a reason for hiding this comment

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

@ShrykeWindgrace looking good, let's add the tighter upper bounds requested, and let's clarify the situation with minimum GHC requirement: I think we found requiring GHC 9.4+ isn't strictly necessary, but as it stands this PR makes hledger require GHC 9.4+, and I'm ok with trying this, but I wonder what's the experience now when you try to build with an older GHC. Shouldn't we bump the lower base bound in the hledger packages also to make our GHC requirement explicit.

@simonmichael
Copy link
Owner

https://gitlab.haskell.org/ghc/ghc/-/wikis/commentary/libraries/version-history has the GHC/base versions. base 4.17.0.0 allows GHC 9.4.0-4, we could require base 4.17.1.0 (GHC 9.4.5+) if it ensures a clearer build failure for installers.

@ShrykeWindgrace
Copy link
Author

@ShrykeWindgrace looking good,

Thanks!

let's add the tighter upper bounds requested,

I thought I did that. What did I miss?

and let's clarify the situation with minimum GHC requirement: I think we found requiring GHC 9.4+ isn't strictly necessary, but as it stands this PR makes hledger require GHC 9.4+, and I'm ok with trying this, but I wonder what's the experience now when you try to build with an older GHC.

If I understand correctly, this PR makes a ghc-9.4+ a requirement only on windows. Other OSes should not be impacted.

Shouldn't we bump the lower base bound in the hledger packages also to make our GHC requirement explicit.

I do not have a good vision on how people build and package hledger on different OSes and platforms. On windows they usually grab an executable from the latest GH release (and hence don't care about GHC), on nix they build with their current GHC (9.4.8 at the moment); you mentioned that Ubuntus/Debians are lagging behind in their packaging (current Debian testing is 9.4.7, https://packages.debian.org/search?keywords=ghc), but then again, I don't know whether people use their pacman/yum/apt/brew/whatever or build from source to get a hledger executable. Personally, I will not feel this GHC version change at all; other people - I do not have enough info. Mailing list did not bring much objection, if I memory serves me right. Sorry of being no help here=)

@simonmichael
Copy link
Owner

Ok, I'm always trying to smooth the haskell build experience as much as we can, but I'm just going to merge this and we'll see how it shakes out. Thank you very much !

@simonmichael simonmichael merged commit ce02f20 into simonmichael:master Dec 19, 2023
1 check passed
@Vekhir Vekhir mentioned this pull request Dec 19, 2023
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.

None yet

3 participants