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

make pancurses work with ncurses 6.0.1 #93

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

correabuscar
Copy link

@correabuscar correabuscar commented Apr 11, 2024

but first, ncurses-rs needs the changes in this PR : jeaye/ncurses-rs#220 (but the below were tested only with PR jeaye/ncurses-rs#218 which is a superset PR which handles more error cases/warnings and overall improves build, I'll have to retest depending on which PR, if ever, gets merged)
this got in jeaye/ncurses-rs#220 as v6.0.1 (already on crates.io), so I'm testing with this version.

Note: KEY_EVENT went away, though it wasn't used anywhere in pancurses.

Closes #92

  • compiles and passes cargo test
  • examples compile and run eg. cargo run --example...
  • examples appear to run correctly, except:
    • cargo run --example newtest, has text under the color box, was it the same with v5 ? yes, it was, overlayed the screens look identical on v5 and on v6 now.
    • cargo run --example newtest only shows the correct text in the other languages on NixOS, this appears to be due to ncursesw being selected there by pkg-config for some reason, not on Fedora/Gentoo which select ncurses - and I didn't force with any features like wide (if I do, it works on those 2 also: ie. cargo run --example newtest --features wide)
  • check if uses of mvprintw and printw by pancurses were using formatting before, since they can't anymore in v6. Ok, it wasn't possible to use formatting before, due to no extra args could've been supplied to fulfill the requested formatting; this got fixed in ncurses-rs so that the unformatted string that was used(as the only arg) won't segfault anymore if % was in it.
  • try features via cargo test
    • no feature specifier (eg. default?)
    • only wide
    • only show_menu
    • only disable_resize
    • all 3 at once

Legend:

  • ❌ = known not to work
  • = untested
  • = works

The above are checked to be true on:

  • Gentoo 2.15 default/linux/amd64/23.0/split-usr/no-multilib (stable)
  • NixOS (if PKG_CONFIG_PATH is set to the dir containing ncurses.pc file)
  • Fedora 39
  • Windows 11 x64 (cmd.exe) (must not use --all-features because then win32 and win32a will both be used and fail compilation)
  • FreeBSD 14.0-RELEASE
    • examples work with TERM=xterm-256color but are broken with TERM=xterm, via ssh using alacritty terminal (freebsd doesn't have alacritty in term database, so had to change it to can compile ncurses-rs aka ncurses crate).
  • OpenBSD 7.5 GENERIC
    • needs one of LC_CTYPE=en_US.UTF-8 or LANG=en_US.UTF-8 to be set otherwise it looks as if it doesn't have wide chars support so cursive and pancuses examples look pretty broken.
  • MacOS Mojave v10.14.6 (with this)
  • Windows 11 WSL1 Arch Linux Linux DESKTOP 4.4.0-22621-Microsoft #2506-Microsoft Fri Jan 01 08:00:00 PST 2016 x86_64 GNU/Linux

TODO:

  • bump the version of pancurses to reflect these changes, 0.18? (or leave this to the repo owner to do)
  • use exact ncurses-rs version in Cargo.toml after it gets published, ie. so it's not lower than that version, because then it would break compilation of pancurses
  • squash the first 3 commits into one, keep the rustfmt one after, separate.
  • run cargo clippy and fix:
    • errors
      • introduced due to this PR
      • pre this PR errors
        • error: this public function might dereference a raw pointer but is not marked unsafe
          • unsafe { curses::delscreen(screen) }
          • unsafe { curses::newterm(type_ptr, output, input) }
          • unsafe { curses::newterm(type_ptr, output, input) }
          • unsafe { curses::set_term(new) }
        • might be more errors but my limit is 5 due to -Z treat-err-as-bug=5
    • warnings
      • introduced due to this PR
      • pre this PR warnings
        • warning: name FILE contains a capitalized acronym
  • I'm watching(github notifications All Activity) the entire repo for any future-reported breakage, to address it.
  • KEY_EVENT went away, go-ncurses also commented it out.

@correabuscar correabuscar changed the title make it work with ncurses-rs v6 make pancurses work with ncurses-rs v6 Apr 11, 2024
@ihalila
Copy link
Owner

ihalila commented Apr 15, 2024

Thanks for the PR. I'll look through this as soon as I find a bit of time.

@correabuscar
Copy link
Author

correabuscar commented Apr 15, 2024

No worries, it's still in draft phase until the ncurses-rs one gets in(if ever), then I'll mark it ready for review, no need to look at it before then, I think.

@correabuscar

This comment was marked as outdated.

@correabuscar
Copy link
Author

Reopening because jeaye/ncurses-rs#220 got in, so this PR might work now, bare with me while I re-test everything in OP...

@correabuscar correabuscar reopened this Jul 12, 2024
@correabuscar correabuscar changed the title make pancurses work with ncurses-rs v6 make pancurses work with ncurses 6.0.1 Jul 12, 2024
... created due to using -Z treat-err-as-bug=5 and there are more than 5
compile errors,

eg. rustc-ice-2024-04-11T10_19_07-85702.txt
@correabuscar
Copy link
Author

correabuscar commented Jul 12, 2024

I forgot that I had deleted the virtual machines, so testing was now only done on Gentoo... but otherwise the PR is ready. Note that the ncurses crate had already published version 6.0.1 which is required for this PR to work (already enforced).

EDIT: technically only this 1 of the 3 commits is needed. Can cherry-pick it, or tell me whether or not to remove the other 2 and force-push ?

@correabuscar correabuscar marked this pull request as ready for review July 12, 2024 18:35
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.

Update to ncurses 6.0.0
2 participants