search

jeaye / ncurses-rs

A low-level ncurses wrapper for Rust
Other
685 stars 99 forks source link

Makes `cursive` and `pancurses` compile with v6 of `ncurses-rs` #220

Closed correabuscar closed 4 months ago

correabuscar commented 5 months ago

This was originally made in PR https://github.com/jeaye/ncurses-rs/pull/218 but now it's split off[^1] for easier reviewing and merging, because that PR also has a ton of build improvements too and it's too difficult to review.

With this PR, cursive and pancurses will compile afterwards but only if they also accept their own PRs which are https://github.com/gyscos/cursive/pull/778 respectively https://github.com/ihalila/pancurses/pull/93

Note, perhaps obvious to me, that currently cursive and pancurses cannot compile (without errors) with version 6.0.0 of ncurses-rs due to regressions that v6 introduced and also some incompatible changes, both of which this PR addresses, so they're stuck using version 5.101.0

[^1]: technically that PR still contains everything this PR, so merging that one doesn't require this one as well. Ideally, that one would be merged instead of this one, but hey, the option to not merge that one(at all, ever) is now available, if we only care about making cursive&pancurses use our version 6 as soon as possible(well, 2 months late anyway :D)

correabuscar commented 4 months ago

Closing in favor of #218 , because these changes here aren't good enough, at the very least they're incomplete, they just do the bare minimum(so things compile, in most used OS-es like Fedora/Ubuntu), but also to allow someone else in the future to come along and not feel deterred to do things better in their own new PR. But more importantly, I may or may not be here in the future to provide support for these changes, as I had originally intended.

jeaye commented 4 months ago

Ah, thanks for the ping on this one. My apologies for being terrible about merging these things. :facepalm: It's not your fault I've taken so long; just bad prioritization on my part.

If you're ok with this, I'm good to reopen this one and merge it without any change.

correabuscar commented 4 months ago

It's up to you really, but if this gets merged I'll reopen the ones from cursive and pancurses as they'll need it.

correabuscar commented 4 months ago

whoops, I reopened it myself, by mistake, just wanted to comment :) reopening is not up to me

jeaye commented 4 months ago

I've published this now. I also cherry-picked 7fd51d9056147746e121ee3f41f70a6152700a99, since it allowed me to build on NixOS. :wink:

You've done a lot of great work with improving portability, in #220; the problem is that it's too much at once. But when I ran into issues building on NixOS, that PR was the first place I looked to see what you did to get NixOS working and if I could just cherry-pick it.

Similarly, if we're able to break it down further, and if you're interested, I think that'll make it easier to digest, review, and merge.

At any rate, thank you so much for both PRs and for the time you've put into this.

correabuscar commented 4 months ago

Ah you meant #218 . Can you please push your current repo state so that it reflects the published v6.0.1 crate on crates.io, the cherry pick isn't yet on github as far as I can tell, and I was wondering why did pancurses just compile for me on Gentoo when using v6.0.1 from crates.io, and that cherry pick explains it.

the problem is that it's too much at once. But when I ran into issues building on NixOS, that PR was the first place I looked to see what you did to get NixOS working and if I could just cherry-pick it. Similarly, if we're able to break it down further, and if you're interested, I think that'll make it easier to digest, review, and merge.

Good feedback, now I'll know this for the future, also @ vwbusguy told me something similar in this comment but I had already done most of the work by that time and it kinda felt too interconnected to just split into PRs, better to know just what to remove or refactor at that point. I'll see if and what I can do about whatever was left, to only make tiny PRs that can be easy, can't promise that I will do anything though.(atm, it's best to assume that I won't)

For NixOS there were later changes too, for sure the one with the shell.nix file fixed a bunch of stuff, I don't even remember at the moment.

jeaye commented 4 months ago

You're right, I meant #218. Everything's been pushed now!

vwbusguy commented 4 months ago

Thanks @correabuscar and @jeaye !!