Makes the build more robust and fixes compile errors and warnings
**UPDATE:** Part of this PR is now duplicated into https://github.com/jeaye/ncurses-rs/pull/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](https://github.com/jeaye/ncurses-rs/pull/201)(or, well, until [this](https://github.com/jeaye/ncurses-rs/commit/3aa22bc279e4929e3ab69d49f75a18eda3e431e9) 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: https://github.com/jeaye/ncurses-rs/pull/201#issuecomment-2045636905
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:
- [x] NixOS (via cmd `nix-shell` which uses ./`shell.nix` file)
- [x] Gentoo 2.15 default/linux/amd64/23.0/split-usr/no-multilib (stable)
- [x] Fedora 39
- ❌ NetBSD curses(ie. not ncurses) mentioned [here](https://github.com/jeaye/ncurses-rs/pull/201) - I can't do it
- [x] FreeBSD 14.0-RELEASE
- [x] without: `ncurses` & `pkgconf`
- [x] with both `ncurses` & `pkgconf` (ie. do `# pkg install ncurses pkgconf`)
- [x] OpenBSD 7.5 GENERIC
- [x] 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.
- [x] 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.
- [x] Ubuntu Desktop 22.04.4 LTS (needs `libncurses-dev`)
- [x] tested to work without `pkg-config` and `pkgconf` installed.
- [x] tested to work with `pkg-config` installed.
- [x] MacOS
- [x] 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](https://github.com/gyscos/cursive/wiki/Install-ncurses#macos))
- ❌ Sonoma v14.4 (won't boot after sys updating to this)
- [x] Windows 11 WSL1 [Arch Linux](https://github.com/yuk7/ArchWSL) 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/gyscos/cursive/pull/778)
* https://github.com/ihalila/pancurses with this PR [here](https://github.com/ihalila/pancurses/pull/93)
* https://github.com/mimblewimble/grin (indirectly via `cursive`) - needs a `cargo update` first and its dep `cursive_table_view` to be updated with [this](https://github.com/BonsaiDen/cursive_table_view/pull/41) PR first.
* https://github.com/BonsaiDen/cursive_table_view with [this](https://github.com/BonsaiDen/cursive_table_view/pull/41) PR.
Ideal TODO(s):
- [x] increment version <del>(left to be done by the maintainer of the repo, unless told otherwise)</del>
- [x] dedup some code in `build.rs`
- [x] could dedup more code in `build.rs`
- [ ] verify changed/added code correctness in `build.rs`
- [x] 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: https://github.com/jeaye/ncurses-rs/issues/127 - EDIT: <del>actually this isn't entirely true, `pancurses` is supposed to work on Windows and it uses `ncurses-rs`,</del>(eh, `pancurses` uses `pdcurses-sys` on Windows) however `build.rs` has some `cfg!(windows)` in code.
- [x] 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
- [x] 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`
- [x] run `rustfmt` on `build.rs` ?
- [x] ensure each env. var. is under `cargo:rerun-if-env-changed`
- [x] use `-L` and the path as separate `.arg()`s not merged into one.
- [x] 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.
- [x] path escaping seems [unnecessary](https://doc.rust-lang.org/std/process/struct.Command.html#method.arg)(kind of) unless the [compiler command](https://docs.rs/cc/1.0.92/cc/struct.Build.html#method.try_get_compiler) would be overridden(ie. changing `PATH` and having the same name as compiler executable there, because otherwise [doesn't seem to use](https://github.com/rust-lang/cc-rs/blob/f313ff54b86c97794f10e5058a6c123b5a50b575/src/lib.rs#L2706) 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.
- [x] 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.
- [x] see [what to do](https://github.com/jeaye/ncurses-rs/pull/218#issuecomment-2049572573) with these (which were there before this PR, but it's a v6 thing)
- [x] `warning: use of deprecated function printw: printw format support is disabled. Use addstr instead`
- [x] un-deprecate [this](https://github.com/jeaye/ncurses-rs/pull/218#issuecomment-2050990343)? this `printw` is a rust function accepting only 1 arg, therefore `format!()` would be used for formatting before calling it.
- [x] `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](https://github.com/jeaye/ncurses-rs/commit/bb767592e4c3e4b36a3d479d8b8c88ad9d78f9cd) `link_name` [doc](https://doc.rust-lang.org/reference/items/external-blocks.html#the-link_name-attribute) - 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.
- [x] I'll add `r#box` too for both rust and `ll`.
- [x] 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](https://github.com/jeaye/ncurses-rs/commit/71691f45b8eae14bd3cca967580bbf24440bb34b).
- ☑︎☐ 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
- [x] introduced by this PR
- [x] fix warning on NixOS: `warning: ncurses@6.0.0: 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 <del>wouldn't compile `ncurses-rs` for other reasons eg. `ERR` `OK`</del> 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.
- [x] fix warnings generated from examples (during `cargo test` though) - even though they happen due to feature not being active.
- [x] test that it builds with `cargo build --features wide`
- [x] 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.
- [x] maybe use `Build`'s [`.flag_if_supported()`](https://docs.rs/cc/1.0.92/cc/struct.Build.html#method.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).
- [x] then also use [try_flags_from_environment](https://docs.rs/cc/1.0.92/cc/struct.Build.html#method.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](https://github.com/rust-lang/cc-rs/blob/f313ff54b86c97794f10e5058a6c123b5a50b575/src/lib.rs#L3571-L3580) kind of breaking compatibility with the way it worked before, although this would be a better way, arguably.
- [x] `cargo test --all-features` compiles
- [x] fix warnings
- [x] `warning: unused doc comment`
- [x] `warning: unused `Result` that must be used` for functions that would Err due to any `\0` in &str when [`CString::new`](https://doc.rust-lang.org/std/ffi/struct.CString.html#method.new) processes it.
- [x] 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.
- [x] doing some `build.rs` revamping eg. [seen here](https://github.com/correabuscar/sandbox/commit/93048777277dfebf54edb882578cf429e133fe4f#diff-58157c65afc825cbc298b4aa3f8008ebf44d2a97bca1cd9d5c234cc10f0d9974)
- [x] I'm watching(github notifications All Activity) the entire repo for any future-reported breakage, to address it.
- [x] 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](https://github.com/jeaye/ncurses-rs/commit/daddcbb557169cfac03af9667ef7aefed19f9409) we're [trying](https://github.com/jeaye/ncurses-rs/blob/3aa22bc279e4929e3ab69d49f75a18eda3e431e9/build.rs#L25-L46) `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](https://github.com/jeaye/ncurses-rs/blob/3aa22bc279e4929e3ab69d49f75a18eda3e431e9/build.rs#L11-L18) 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.
- [x] 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.
- [x] link `tinfo(w)` as fallback in case it might exist(like on Gentoo) but won't affect NixOS which doesn't have it(<del>due to the `cargo`'s gonna ignore if it doesn't exist</del>**no 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. <del>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)</del>**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.
- [x] try `LC_CTYPE="en_US.UTF-8"` on OpenBSD
- [x] 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.
- [x] 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.
- [x] 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)
- [x] 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)
- [x] 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`.
- [x] <del>replace `OnceLock` in `build.rs` which requires MSRV `1.70.0` with something else maybe `AtomicBool`+`Mutex` which have MSRV `1.0.0`</del> 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.
- [x] set edition to 2021 (MSRV 1.56.0 required)
- [x] 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](https://github.com/jeaye/ncurses-rs/pull/218#issuecomment-2123881995).
- [x] re-test that everything builds/test (but don't try to run the examples)
- [x] re-test the build on native windows (which should still fail without any other errors)
合并状态:未合并 关闭于 2024-07-12 12 条评论