Makes the build more robust and fixes compile errors and warnings

[correabuscar]

UPDATE: Part of this PR is now duplicated into #220 (for the case when the backwards-compatible(iirc) improvements here aren't wanted, or seem too much to review) but in this PR I've kept the commits that #220 has, for now, so merging either one or both should be possible.

This PR preserves the changes that were already done in v6 here(or, well, until this point) and attempts to "fix" some of the missing things... while also making sure compilation doesn't fail on a bunch of target environments and if it does fail, that the user trying to build it has plenty of helpful information about it. Before this PR it would build on Fedora and Ubuntu (and OpenBSD/FreeBSD but fail to link ex_5 with --features=menu) and fail to build on NixOS or Gentoo.

This PR was initially spawned from: #201 (comment)

The following were used successfully:

• cargo build
• cargo test (though there are 0 tests)
• cargo build --all-features
• cargo test --all-features
• cargo build --all-targets
• cargo test --all-targets
• cargo build --all-targets --all-features
• cargo test --all-targets --all-features

... on each of these target environments:

• NixOS (via cmd nix-shell which uses ./shell.nix file)
• Gentoo 2.15 default/linux/amd64/23.0/split-usr/no-multilib (stable)
• Fedora 39
• ❌ NetBSD curses(ie. not ncurses) mentioned here - I can't do it
• FreeBSD 14.0-RELEASE
  • without: ncurses & pkgconf
  • with both ncurses & pkgconf (ie. do # pkg install ncurses pkgconf)
• OpenBSD 7.5 GENERIC
  • with pkg-config(which it doesn't find ncurses) and no explicit ncurses installed(but base75 and comp75 has the libs respectively the headers) - it builds and passes all tests(even of cursive and pancurses) but the examples look broken/unusable in cursive (TERM=vt220) - in ncurses-rs doesn't detect F1 to exit from ex_4 and ex_5 - this depends on TERM env.var, for example if it's TERM=alacritty they work.
  • 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.
• Ubuntu Desktop 22.04.4 LTS (needs libncurses-dev)
  • tested to work without pkg-config and pkgconf installed.
  • tested to work with pkg-config installed.
• MacOS
  • Mojave v10.14.6 (fails to link ex_5(ld: symbol(s) not found for architecture x86_64) unless you brew ncurses and set PKG_CONFIG_PATH like said here)
  • ❌ Sonoma v14.4 (won't boot after sys updating to 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

... and on the following repos that use ncurses-rs (if they applied their own patches for which they've PR already waiting in draft mode, to transition from using ncurses-rs v5 to v6 after this PR is in):

• https://github.com/gyscos/cursive with this PR here
• https://github.com/ihalila/pancurses with this PR here
• https://github.com/mimblewimble/grin (indirectly via cursive) - needs a cargo update first and its dep cursive_table_view to be updated with this PR first.
• https://github.com/BonsaiDen/cursive_table_view with this PR.

Ideal TODO(s):

• increment version (left to be done by the maintainer of the repo, unless told otherwise)
• dedup some code in build.rs
• could dedup more code in build.rs
• verify changed/added code correctness in build.rs
• re-check that it still compiles/tests/runs_examples correctly after all changes.
• ❌ test on windows target (ie.x86_64-pc-windows-gnu) (compile,test, maybe also run examples)
  • would build if LC_MESSAGES were removed (it's in two places) - no idea what about them but it's probably no good to do this.
• ❌ test on Windows (I can't do) - well, it's not supposed to work, it doesn't support Windows: Build issue; permission denied to open file #127 - EDIT: actually this isn't entirely true, pancurses is supposed to work on Windows and it uses ncurses-rs,(eh, pancurses uses pdcurses-sys on Windows) however build.rs has some cfg!(windows) in code.
  • test that pancurses compiles with this PR, on Windows. (it's unaffected)
• ❌ test on Windows WSL (i can't do Ubuntu/Debian/etc. only Arch Linux)
• ❌ something else I didn't think of
• fix ex_5 linking which fails on NixOS when cargo test --all-features is used (is fine on Fedora and Gentoo though) - unclear why it can't find the menu lib unless specified like: RUSTFLAGS='-l menu' cargo test --features=menu // fixed by ensuring pkgs.pkg-config existed in the nix-shell
• run rustfmt on build.rs ?
• ensure each env. var. is under cargo:rerun-if-env-changed
• use -L and the path as separate .arg()s not merged into one.
• are Path/PathBuf handled correctly? (see https://gist.github.com/ssokolow/0d9f5c5e4a8a37a962875af205bcc723 ) I'll just use .display(), we definitely don't want \0 to be part of the path i believe.
• path escaping seems unnecessary(kind of) unless the compiler command would be overridden(ie. changing PATH and having the same name as compiler executable there, because otherwise doesn't seem to use an env var to set the executable for compiler to use EDIT: it does actually use CC env.var.) to use a script thus paths would be passed to a script acting as the compiler. Defaulting to using .display() on Path/PathBuf just like previous code did. But this would be bad if compiler is a script, but it's not something we can control or pre-escape from here really.
• ensure each commit will not break compilation, eg. if part of a git bisect; this means, squashing the commits before merge wouldn't be necessary. For this reason decided not to break that big build.rs commit into smaller commits anymore, but also because it's doing lots of refactoring.
• see what to do with these (which were there before this PR, but it's a v6 thing)
  • warning: use of deprecated function printw: printw format support is disabled. Use addstr instead
    • un-deprecate this? this printw is a rust function accepting only 1 arg, therefore format!() would be used for formatting before calling it.
  • warning: attribute should be applied to a foreign function or static #[link_name="box"] pub fn box_(w: WINDOW, v: chtype, h: chtype) -> i3 not a foreign function or static UNCLEAR link_name doc - ok i think i understand the intention now: we wanted to use ncurses::box_ in rust but the ncurses::ll:box_ one must map to one named box from the ncurses lib. But I'm not sure why this was done instead of just using box from the start, ah I see expected identifier, found reserved keyword but now r#box could be used, except that box_ would also have to be used for compatibility.
    • I'll add r#box too for both rust and ll.
  • fix warning reported with newer rust version: warning: creating a shared reference to mutable static is discouraged
&wrapped::acs_map as *const chtype shared reference to mutable static - apparently nothing now uses this anymore, in this crate, but was added by this commit.
• ☑︎☐ run cargo clippy and fix:
  • ❌ errors
    • ❌ pre this PR errors
      • ❌ error: this public function might dereference a raw pointer but is not marked unsafe
        • ❌ str::from_utf8_unchecked(CStr::from_ptr(ptr).to_bytes()).to_owned()
        • ❌ ll::copywin(src_win, dest_win, src_min_row, src_min_col,
        • ❌ and lots more like that
  • ☑︎☐ warnings
    • ❌ pre this PR warnings
      • ❌ too many
    • introduced by this PR
• fix warning on NixOS: warning: [email protected]: 414 | # warning _FORTIFY_SOURCE requires compiling with optimization (-O)
• ❌ retry with NetBSD curses (lost cause), and ncurses on NetBSD(? or is it on sabotage linux?) - either way, I failed on both before (on sabotage linux wouldn't compile ncurses-rs for other reasons eg. ERR OK this was due to TERM=linux instead of TERM=xterm now compiles but ex_7 won't link; and I couldn't get NetBSD to see its own packages, sees 0)
  • worst case, setting NCURSES_RS_RUSTC_LINK_LIB=curses should work, I estimate.
• fix warnings generated from examples (during cargo test though) - even though they happen due to feature not being active.
• test that it builds with cargo build --features wide
• it's easier on NixOS to just run nix-shell in repo dir (I didn't notice shell.nix existed until late) than having pkgs.ncurses installed system-wide and then setting PKG_CONFIG_PATH before running cargo build in repo dir. Both work.
• maybe use Build's .flag_if_supported() (or just .flag()) to add compiler flags instead of (the old way)directly via the compiler that was gotten via .try_get_compiler(), I just kept the old way there so far (maybe there's a good reason for it, unsure).
  • then also use try_flags_from_environment for adding flags from the environment var NCURSES_RS_CFLAGS - well that method will try search for an environment variable with appropriate target prefixes first, so a bunch of differently named env. vars will be checked first kind of breaking compatibility with the way it worked before, although this would be a better way, arguably.
• cargo test --all-features compiles
  • fix warnings
    • warning: unused doc comment
    • warning: unused Result that must be used for functions that would Err due to any \0 in &str when CString::new processes it.
• don't elide errors in .unwrap_or_else(|_| but show them instead, so start with .unwrap_or_else(|e| ... and show e - this was introduced by a commit in this PR, but now force pushed as fixed.
• doing some build.rs revamping eg. seen here
• I'm watching(github notifications All Activity) the entire repo for any future-reported breakage, to address it.
• add fallback lib names for 'menu' and 'panel' too, else linking eg. ex_5 will fail on FreeBSD unless it has pkgconf installed (ie. pkg install pkgconf)
• figure out why we're trying ncurses5 then ncurses since the latter is version 6(or presumably latest so at least 5 or more), don't we want to use the latest version? Was ncurses ever lower version than ncurses5 in the past? Since we stop if we find ncurses5, this means we'd be using 5 instead of 6 if somehow both are present. This was the behavior before this PR. If this is checked-marked then I've switched it to: try ncurses(w) then ncurses(w)5, stop on first find; same for 'menu' and 'panel' I left this as it was because I've no way to test that it broke before and isn't now.
• fix ex_6 in FreeBSD, see why it's black screen except the bottom shows <-Press Space-> - that being said, comments in vim are very dark blue which are unreadable! - Ok it has TERM=xterm in text console, but works well with TERM=vt100 or TERM=vt220, tho no colors; with TERM=xterm-256color is that dark blue from vim comments mostly, but still better than that black one from before. Gonna say it's FreeBSD at fault here because vim colors are also weird.
• link tinfo(w) as fallback in case it might exist(like on Gentoo) but won't affect NixOS which doesn't have it(due to the cargo's gonna ignore if it doesn't existno it's not!); so, when pkg-config(/pkgconf) doesn't exist or i've set NCURSES_NO_PKG_CONFIG=1 and NCURSESW_NO_PKG_CONFIG=1 (and ncurses and ncursesw5 don't exist on system) which emulates pkg-config not existing, then, at least on Gentoo, libtinfo(w) doesn't get linked in, and so for example ex_5 it will fail to link because we have no fallback for linking tinfo lib too, fallback was there only for ncurses lib. And the good thing is if tinfo isn't needed, such as on NixOS, and we could still tell cargo via println!("cargo:rustc-link-lib=tinfo");(or tinfow) to link it, cargo will gracefully ignore it if it doesn't exist.(or so I hear, must test)it doesn't it's just like adding -ltinfo to linker, fails if it doesn't exist, like on NixOS - must find a way here... I've an idea to try.
• try LC_CTYPE="en_US.UTF-8" on OpenBSD
• compilation breaks if the TERM is unknown on the system, errors like: error[E0432]: unresolved import constants::TRUE - with this fixed, I can definitely go back to trying sabotage linux's netbsd-curses then, which was failing like this before.
• libmenu/libpanel both require linking with libncurses explicitly, which the normal cargo build does, but not try_link()(which this PR added in build.rs) which would fail to find libmenu but say it would still tell cargo to link it, so it worked anyway. Fixed it now to try linking with ncurses if without it fails, thus detecting that libmenu links successfully and thus not show the wrong warning.
• detect if none of LC_ALL, LC_CTYPE or LANG are set to anything with utf-8 (case insensitive) in them, then cargo warn that wide feature won't display correctly, but only if wide feature is enabled. OpenBSD 7.5 has none of these set, by default (maybe I didn't follow the install instructions, else I would've set them manually, but they're for sure not set automatically)
  • warn if not UTF-8 exactly on OpenBSD, else it won't work. However en or en_US or even ``(empty) can be used in any case (we don't check for these though), so even LANG=.UTF-8 works (but not on Gentoo for example)
• MSRV is 1.57.0 now; outdated info follows://determine and set MSRV, 1.70.0, but could go as low as 1.57.0 with --features=not_OnceLock and pinning build dep cc="=1.0.92 and changing rust-version="1.57.0" in Cargo.toml.
  • ❌ see if can get MSRV even lower
    • cc="=1.0.92" passes cargo build with MSRV 1.53.0 but not cargo test unless MSRV is 1.63.0
    • cc="=1.0.95" needs 1.63.0
    • cc="=1.0.18"(the pre-PR value) has MSRV 1.19.0 (for cargo build) but it doesn't pass cargo test with any rust version.
    • cc="=1.0.40"(randomly picked) passes cargo test for MSRV 1.40.0 (coincidence) and cargo build MSRV is 1.19.0.
• replace OnceLock in build.rs which requires MSRV 1.70.0 with something else maybe AtomicBool+Mutex which have MSRV 1.0.0 remove OnceLock and just don't care about repetitions for cargo:rerun-if-env-changed, MSRV is now 1.57.0, but remember that cc is 1.0.95 at the moment (Cargo.lock not being in the repo) thus MSRV is 1.63.0 just like cc needs.
• set edition to 2021 (MSRV 1.56.0 required)
• remove cargo build --features=test_build_rs_of_ncurses_rs which was testing only some of the build.rs internal code. This testing would've only be useful for devs of ncurses-rs basically, but it wasn't testing enough of the code anyway. Did help during the PR though. Removed as per this.
  • re-test that everything builds/test (but don't try to run the examples)
  • re-test the build on native windows (which should still fail without any other errors)

[vwbusguy]

I don't really know what to do about these 2 warnings, which were there before this PR, so I'm gonna delegate them to the next person :)

   Compiling ncurses v6.0.0 (/home/user/1tmp/ncurses-rs)
warning: attribute should be applied to a foreign function or static
   --> /home/user/1tmp/ncurses-rs/src/lib.rs:161:1
    |
161 |   #[link_name="box"] pub fn box_(w: WINDOW, v: chtype, h: chtype) -> i32
    |  _^^^^^^^^^^^^^^^^^^_-
162 | | { wborder(w, v, v, h, h, 0, 0, 0, 0) }
    | |______________________________________- not a foreign function or static
    |
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: `#[warn(unused_attributes)]` on by default

warning: use of deprecated function `printw`: printw format support is disabled. Use addstr instead
   --> /home/user/1tmp/ncurses-rs/src/lib.rs:822:3
    |
822 |   printw(s)
    |   ^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

warning: `ncurses` (lib) generated 2 warnings
    Finished dev [unoptimized + debuginfo] target(s) in 1.97s

Arguably the latter could just get a #[allow(deprecated)] on top of that printw(s) call.

These appear unrelated to the PR to me.

printw() and addstr() do similar things. I looked through recent release notes for ncurses upstream and couldn't find a reference for printw() being deprecated, just vwprintw (without the underscore vw_).

https://tldp.org/HOWTO/NCURSES-Programming-HOWTO/printw.html
https://invisible-island.net/ncurses/NEWS.html

[correabuscar]

As a reminder, worst case, you could just consider(ie. cherry pick) only the last 3 commits from the day of April 11th, seen here at the top: https://github.com/jeaye/ncurses-rs/pull/218/commits (EDIT: these¹) and ignore the rest(ie. from april 18th and onwards) or even forget about the whole rest of PR, because those 3 commits are the only ones needed to get the other projects (cursive, pancurses) to work (and PRs for them are already prepared). Meanwhile, I could just take another look to see if indeed those 3 commits are indeed enough. EDIT: I've done the checking: they compile/test on wsl(archlinux), ubuntu 22.04.4, fedora 39. Fails to build(or link examples) on nixos,Gentoo,OpenBSD 7.5, FreeBSD 14.0 though, it would need some later changes(related to build) to not fail, no doubt.

I could also just remove the useless feature test_build_rs_of_ncurses_rs

I'm tempted to say we should do this[...]
[...]
So I'm not sure if this sort of thing is commonplace these days.

that testing of build.rs code, is definitely not common place at all, I haven't seen it used personally, I've just made that up for this just to make sure the code I changed/added didn't break(which it did several times), but it's not testing enough of the build.rs anyway (I wanted to add more tests, but procrastinated as I felt it would be removed anyway), and there doesn't seem to be an idiomatic way(or any?) of testing build.rs, but I may be wrong. I'm a rust beginner actually and while I've had no intention of using anything with ncurses, I can still monitor this repo to address any possible fallout from these changes.

I can just make a commit to remove that testing build.rs functionality shortly... EDIT: done (341 less lines).

Back when I was used to Rust, cargo did all of the building magically, rather than needing a 1700 line build.rs

the code could probably be minimized more I guess, but it's mainly this much because it's meant to handle many build cases with better reporting in case of build failures, otherwise whoever's building it might be spending more time trying to figure out what went wrong which is something I was trying to spare/avoid. This is arguably not commonplace...

Footnotes

1. those 3 commits are:
   https://github.com/jeaye/ncurses-rs/pull/218/commits/34310c5546a27b822552ee9b37f6727405ffaac2
   https://github.com/jeaye/ncurses-rs/pull/218/commits/ab8fd269fd37ce8e8970406bbc7d803b5be2e8bc
   https://github.com/jeaye/ncurses-rs/pull/218/commits/e4a1b4fffe14f88bb91181638273003c00d59ac3
   they're the last 3 seen for day Apr. 11 in this view. ↩

[ThomasHabets]

I'm out of my depth, here.

[vwbusguy]

I'm out of my depth, here.

Ditto. :-)

[correabuscar]

I (somehow lol)realize now that I've been too stubborn about it and that @vwbusguy was right about this PR needing to be split into two others. So I'll attempt to do this next, so this PR will remain the build improvement one and a new PR will do ONLY the needed stuff for cursive and pancurses to use ncurses-rs v6 without compile errors.

[correabuscar]

Closing this to allow someone else in the future to come along and do things better, less verbose and more maintainable(!), and so they won't feel deterred by the existence of this PR to make it their own way :)
But more importantly, I may or may not be here in the future to provide support for these changes, as I had originally intended, which would be unpleasant for someone else to have to maintain, I can imagine, so I'd recommend not merging this because of that.