jeaye / ncurses-rs

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

Makes the build more robust and fixes compile errors and warnings #218

Closed correabuscar closed 4 months ago

correabuscar commented 7 months ago

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(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: https://github.com/jeaye/ncurses-rs/pull/201#issuecomment-2045636905

The following were used successfully:

... on each of these target environments:

... 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):

Ideal TODO(s):

correabuscar commented 7 months ago

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.

EDIT: Regarding the other warning, here's what ChatGPT 3.5 says (better than I could):

The warning message indicates that there's an attribute #[link_name="box"] applied to a function named box_, which is likely intended to alias the function box_ to a symbol named "box" in the resulting binary.

However, the warning suggests that this attribute is being applied to a function (box_) that is not a foreign function or static, which means it's being used incorrectly according to the Rust compiler. This warning is letting the developer know that while this usage was previously allowed by the compiler, it's being phased out and will eventually become a hard error in future releases of Rust.

To resolve this warning, the developer might need to review the purpose of the #[link_name] attribute and ensure it's being applied correctly, possibly to a foreign function or static item as intended. If the intention is to rename the function box_ to "box" in the resulting binary, they might need to adjust the usage of the attribute or find an alternative approach that aligns with Rust's conventions.

It's unclear to me whether this actually worked at all before, and is it still working now while it's a warning, but won't when it becomes an error.

Here's the doc for link_name

vwbusguy commented 7 months ago

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 commented 7 months ago

Indeed, the warnings were there before this PR, however I felt I should've addressed them in this PR since it's about making v6 usable by v5 users of it like cursive and pancurses.

ncurses::printw(the rust version of it) was deprecated before(but for a different reason), however now ll::printw(the c version, what your link points to) can do formatted strings via ncurses-rs(previously it couldn't)

I was wondering whether ncurses::printw should still be deprecated because now that it got fixed, and it still only accepts one argument, thus can't accept formatted strings(ie. you'd need extra args), it's practically working as before (but now it doesn't crash anymore due to stray say %s), however maybe users of it expect it to act like the c version and accept formatted strings(but this is what ll::printw is for instead). But maybe I'm missing something and that's why keeping it deprecated makes sense. Either way, users of ncurses::printw would pass rust's format!() strings to it, if formatting was desired.

correabuscar commented 7 months ago

I'm gonna say the PR is as ready as it can be, otherwise there's always gonna be something to change/improve on it each day :)

Alternatively, if I don't change anything for like 2 days, it's probably because I've nothing else that I'm willing to do on it(but rest assured there's plenty more to do on it, I just have to be in the right mood to do it)
vwbusguy commented 7 months ago

You're already at 50 commits and over 1700 LOC. It feels like a lot of scope creep happened here. My concern with that is getting the CVE patch rolled out in the other libraries now depends on @jeaye reviewing and merging this massive PR. I very much appreciate your work here, but perhaps splitting this off into separate PRs for some of the other quality of life fixes might help make these changes easier to grok and review.

correabuscar commented 7 months ago

You're already at 50 commits and over 1700 LOC. It feels like a lot of scope creep happened here. My concern with that is getting the CVE patch rolled out in the other libraries now depends on @jeaye reviewing and merging this massive PR. I very much appreciate your work here, but perhaps splitting this off into separate PRs for some of the other quality of life fixes might help make these changes easier to grok and review.

I guess the last 3 commits from April 11th can be cherry picked to make v6 work with cursive/pancurses(but they still need their own patches), the rest of the PR can be reviewed later(if ever).

EDIT: I could squash all the (50) commits into one or two, so to avoid the need to review every[^1] commit. But otherwise each commit is made so that the build doesn't fail to compile, especially during a git bisect. That being said, some builds will fail to compile because their fixes happened in some subsequent commit(s).

I could also just remove the useless feature test_build_rs_of_ncurses_rs, as it's not gonna be used by anything, it's just a cargo test equivalent for some of build.rs QOL code. That would remove 250+ LOC.

Lots of lines are just comments, but comments can be ignored, and I think it's better to have them than not at all, even if they may even be wrong or outdated.

laterEDIT: @vwbusguy regarding your stated concern, if you wish to accelerate that, please by all means feel free to submit a new PR with only the relevant changes needed (you can either use my commits(those 3 I mentioned) or even submit new ones as your own even if it ends up being the same content, I honestly don't mind about credit, you've all my blessings) if you hope it would be accepted sooner... I'll then adapt my PR after yours gets accepted to include the changes(eg. rebase on master). Ie. if you want to do that do it, don't let the existence of this PR stop you from doing it, I honestly won't mind at all. I don't do it myself because perfectionism(sux) :) otherwise I would've just left this PR alone as it was initially, and no scope creep would've happened.

[^1]: which ensures no evil code sneaks in and gets run by builds during a git bisect.

jeaye commented 6 months ago

Wow. This is a lot of work. Thank you for doing it! I missed your comment about being done, since there were so many commit emails coming in that I stopped looking at them for a while. :facepalm:

I could also just remove the useless feature test_build_rs_of_ncurses_rs, as it's not gonna be used by anything, it's just a cargo test equivalent for some of build.rs QOL code. That would remove 250+ LOC.

I'm tempted to say we should do this, but what I'd really love is to get someone who knows the current state of Rust better than I do. I haven't been keeping up much with Rust in the last several years (working on my own lang instead). Back when I was used to Rust, cargo did all of the building magically, rather than needing a 1700 line build.rs. So I'm not sure if this sort of thing is commonplace these days.

With that said, I have gone through the PR and I don't have any overall concerns beyond getting more eyes on this before it gets merged. As @vwbusguy said, if we want to unblock other projects from upgrading to fix that CVE sooner, we can separate that work. But if I could get one or two other folks to go through this, that'd increase my confidence in it.

@vwbusguy, @Ella-0, or @ThomasHabets: Anyone up to give this a look as well?

correabuscar commented 6 months ago

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[^1]) 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...

[^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 commented 5 months ago

I'm out of my depth, here.

vwbusguy commented 5 months ago

I'm out of my depth, here.

Ditto. :-)

correabuscar commented 5 months ago

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 commented 4 months ago

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.