8000 Remove (n)curses dependency by faho · Pull Request #10269 · fish-shell/fish-shell · GitHub
[go: up one dir, main page]

Skip to content

Remove (n)curses dependency#10269

Merged
faho merged 7 commits intomasterfrom
uncurse
Feb 22, 2024
Merged

Remove (n)curses dependency#10269
faho merged 7 commits intomasterfrom
uncurse

Conversation

@faho
Copy link
Member
@faho faho commented Jan 27, 2024

Description

Note: This PR is an exploration. It's to show that this is possible and to see if it would be workable across a variety of systems.

Here's what this does:

It replaces our (very few) uses of curses the C library with the rust-terminfo crate, which reads the terminfo database on its own.

That means we no longer link against curses, we don't need to find curses, which simplifies the build quite a bit.

It also removes quite a bit of C-interoperability gunk - there is no more "cur_term" pointer that can be null and needs to be released or we get a memory leak. It also removes the pain of "what argument does tputs take" because there no longer is a tputs.

Some open questions:

  • The default search path for terminfo does not include /usr/local/share/terminfo, where FreeBSD would place it. I have hacked the tests to set $TERMINFO_DIRS and opened Add FreeBSD default search paths meh/rust-terminfo#40.
  • I have replaced "tputs" with write(tparm(foo)) - tputs did some weird locking around its output and required a function pointer and it was all a bit messy. Was there an actual reason for that?
  • This requires a "non-hashed" terminfo database, meaning there is apparently a way to build (n)curses so it can't read it. I have never seen systems like that. On FreeBSD you install terminfo-db and it's done
  • Are there any systems that do not have terminfo at all? I have not found any so far, FreeBSD 14 defaults to using terminfo (from the release notes: "The ncurses(3X) library is now able to use terminfo(5) as well as termcap(5), and uses terminfo preferentially") and FreeBSD 13 also has it available

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

@faho faho added this to the fish next-3.x milestone Jan 27, 2024
@faho
Copy link
Member Author
faho commented Jan 27, 2024

One possibility of making fish work on awkward systems without a working terminfo is to hard-code a fallback Term object, which could even just include the values of xterm-256color we use.

@faho
Copy link
Member Author
faho commented Jan 27, 2024

FreeBSD generally works, and the PR to add the standard directory has been accepted.

NetBSD has its own format, stored in /usr/share/misc/terminfo.cdb, which file calls a "NetBSD Constant Database, version 1, for 'NetBSD terminfo'".

You can install ncurses via pkgin install ncurses, and find the database for that in /usr/pkg/share/terminfo. Running fish with TERMINFO_DIRS=/usr/pkg/share/terminfo fish works.

@n1000
Copy link
n1000 commented Jan 28, 2024

I tried this branch out on OpenBSD 7.4 as well, it seemed to work fine there also (at least I did not notice any obvious issue). I had to additionally bump the libc crate version up to 0.2.152 to bring in some missing libc posix_spawn* declarations to fix the compilation on OpenBSD.

@meowcoder
Copy link

Can't compile without libncurses-dev installed, can't find term.h.
Had to remove the include line from libc.c to fix this.

@faho faho force-pushed the uncurse branch 2 times, most recently from b3c732e to 830fe77 Compare January 28, 2024 18:05
@mqudsi
Copy link
Contributor
mqudsi commented Jan 28, 2024

I am against replacing a package literally available everywhere (curses) with dependencies that might need to be manually installed (even if via the package manager) on various platforms. My FreeBSD installation has been upgraded from version to version since 2009 or so, is heavily abused, contains a ton of non-portable stuff, and yet still doesn't have terminfo-db installed.

@faho
Copy link
Member Author
faho commented Jan 28, 2024

dependencies that might need to be manually installed (even if via the package manager) on various platforms

Keep in mind that most people would either already have it installed (e.g. on non-BSD) or just get them with the fish package (FreeBSD's fish package would depend on it).

So it's only those that are on a platform where this isn't already the default and decide to install fish outside of their distro package. And for those people, it's an 800K download.

Of course I recognize that it will hit you personally because you'll want to compile fish yourself, but even then you'll just have to install a package. (unfortunately FreeBSD 14's ncurses will prefer a terminfo database if it's available, but it isn't a dependency)

In return we get an easier build with some pretty gnarly interoperability code removed. That trade-off sounds attractive to me.

Still, like I said, this is an experiment, we don't have to take it. Maybe we'll find another replacement for curses - which we barely use, so we really need just the db lookup.

@faho
Copy link
Member Author
faho commented Jan 28, 2024

I'm also absolutely not above just including a few terminfo entries ourselves.

This already includes xterm-256color, which should be the most widely used single entry. If we added ~10 more we could probably get it so most people never actually need their terminfo database. E.g. my fresh FreeBSD VM apparently defaults to TERM=xterm.

And it's not like these things change constantly - and if they do, you'll get terminals like alacritty shipping their own, which this also just picks up.

@mqudsi
Copy link
Contributor
mqudsi commented Jan 28, 2024

For what it's worth, I've read more about it and terminfo-db was introduced in FreeBSD 13 (con: if we depend on it, we will be permanently icing out FreeBSD 12 and below users, but that just hit EOL) and currently is built from the ncurses sources (it's a build dependency). But the plan is to invert the dependency tree in FreeBSD 14 so that ncurses depends on terminfo-db, which means it'll basically be installed everywhere by default in that case.

In light of this, I take back a good part of my resistance to this change, though I continue to have concerns about the availability of terminfo on non-mainstream platforms (where curses is almost always available).


Suggestion: We could use terminfo and fall back to termcap. I think that would cover all sources, but at the expense of needing a (thin) layer of abstraction (in the absence of terminfo, forward 2-char capabilities to termcap and return None for the rest).

@faho
Copy link
Member Author
faho commented Jan 29, 2024

We could use terminfo and fall back to termcap.

I am not opposed to adding termcap, but only if that is ever used.

I have looked, and so far I have not found a single platform that isn't EOL that doesn't ship a readable terminfo database as a package but has a termcap database.

  • NetBSD has a terminfo database by default, but in a separate format. ncurses terminfo is available as a package.
    It does not have a separate termcap database (see https://man.netbsd.org/termcap.5, "the termcap database file is no longer shipped with NetBSD" - the page is from 2012)
  • FreeBSD 14 will include ncurses terminfo, it's available as a package in 13+.
  • OpenBSD apparently ships terminfo according to Remove (n)curses dependency #10269 (comment)
  • Any linux distro I know has ncurses terminfo.
  • macOS has terminfo
  • Haiku has an ncurses package, no idea about the default state
  • Illumos appears to have terminfo

Can you name a system that has termcap but doesn't have terminfo? What about one where you can't easily get terminfo as a package?

Because it seems to me like adding support for NetBSD's terminfo format would help more than adding termcap.

in the absence of terminfo, forward 2-char capabilities to termcap and return None for the rest

This part is easy, we already only use termcap names.

@mqudsi
Copy link
Contributor
mqudsi commented Jan 29, 2024

I'm assuming where you say "terminfo" or "ncurses" in your last comment, you specifically mean a terminfo directory and not just terminfo via ncurses?

I am surprised that the list is that comprehensive; FreeBSD usually isn't the last to bring in newer tech. But I just checked and even my dated Solaris 11 and OpenBSD 6.8 virtual machines have terminfo databases (at /usr/share/lib/terminfo and /usr/share/terminfo, respectively).

After thinking about it some, another option to would be to ship with our own "fallback terminfo db" containing just an entry for xterm and xterm-256 and we would be ok even on platforms without terminfo support at all?

@faho
Copy link
Member Author
faho commented Jan 29, 2024

I'm assuming where you say "terminfo" or "ncurses" in your last comment, you specifically mean a terminfo directory and not just terminfo via ncurses?

Yes. There is a "hashed database" format that ncurses introduced pretty late, which isn't used much from what I can see. I believe FreeBSD used to, but they have terminfo-db now.

OpenBSD explicitly dropped the hashed format in 2015.

rust-terminfo cannot "currently" read the hashed format.

(tbh I find this a pretty pointless optimization nowadays - reminds me of how fortune ships those ".dat" files to point to the actual fortunes, when it's 2MB of text and you can just read it directly)

After thinking about it some, another option to would be to ship with our own "fallback terminfo db" containing just an entry for xterm and xterm-256 and we would be ok even on platforms without terminfo support at all?

I had a similar idea: c845eb3 adds xterm-256color, and uses it if all else fails.

We would probably want to use that if $TERM is xterm-256color and reading it from disk failed (and probably without an error message at all). If we want to make it semi-comprehensive we could add "xterm", "screen" and "tmux" plus their -256color variants.

@faho
Copy link
Member Author
faho commented Jan 31, 2024

Alright I added a test that compares the hardcoded xterm-256color I added (from my ncurses install) to the system's, and we see two differences:

  1. macOS' is outdated. We know this, that's why we hack around it to add italics and such.
  2. FreeBSD's entry has backspace and delete swapped.

That latter one is annoying, but probably not fixable - it's a decades old confusion that terminals make configurable. So if we do want to add a fallback definition, we would have to live with this. (I don't believe #cfging it out really helps)

@mqudsi
Copy link
Contributor
mqudsi commented Feb 1, 2024

Can you expand on this:

FreeBSD's entry has backspace and delete swapped.

How does that contrast to the current behavior when retrieving the value from curses under FreeBSD?

@faho
Copy link
Member Author
faho commented Feb 1, 2024

How does that contrast to the current behavior when retrieving the value from curses under FreeBSD?

Sorry, it's not delete-the-key, it's the ascii "DEL" character. It's the backspace key and ctrl-backspace that are swapped, because one of them sends DEL and the other the "backspace" character. It only affects if bind -k backspace triggers for backspace or ctrl-backspace.

The good news is that we bind both the same in our bindings because this is a common confusion between terminals, so it's not a huge deal. It is also configurable in a lot of them (because of course it is), so you can't actually rely on terminfo to begin with.


The useless details

My archlinux system says "kbs=^?", i.e. ctrl-? (aka \x7f, aka "DEL")

The FreeBSD terminfo says "kbs=^H", i.e. ctrl-H

When I run unconfigured and unmodified actual xterm on my arch machine and press backspace I get ctrl-H, when I press ctrl-backspace I get ctrl-?.

That means fish_key_reader (reading from curses) locally on my linux machine says pressing actual backspace in xterm is \b and pressing ctrl-backspace is -k backspace and \x7f. Connecting to FreeBSD via ssh says backspace is -k backspace and \b and ctrl-backspace is \x7f.

In addition, gnome terminal, konsole and wezterm, all claiming to be xterm-256color (and I would guess much more popular than crusty old actual xterm), agree with arch's definition by default (as do the alacritty, kitty and tmux terminfos), so I get -k backspace for backspace and \b for ctrl-backspace locally.

There is a whole mess of history behind this, I read it years ago but tbh I don't think it really matters. The current situation is terrible and I would like for things to improve somewhat.

@faho
Copy link
Member Author
faho commented Feb 20, 2024

Alright, I believe this is pretty successful.

It gets us around linking to (n)curses, which was always one of the more awkward parts of our build. Which also means it fixes #10316, and makes building any form of self-contained package like appimages easier (because including ncurses there was a crapshoot), see #6475 (comment). And it does that by making the code simpler (no more cur_term!) and making a nonexistent curses less of a problem.

The downside is that the BSD packages will have to depend on their terminfo database, because some of them don't install it by default - but even that will mostly work because xterm-256color is the single most common $TERM value.

So I would like to see if there is any appetite to merge this.

@zanchey
Copy link
Member
zanchey commented Feb 20, 2024

I think it's a good idea. rust-terminfo looks fine (supports home directory databases and TERMINFO_DIRS).

src/curses.rs Outdated
};
return Err(x);
};
let mut path = PathBuf::from("/usr/pkg/share/terminfo");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is a NetBSD-specific workaround then maybe enable it only there?
Also, maybe this should use env!("PREFIX") instead of hardcoding /usr?
The only place where we hardcode /usr is in the DEFAULT_PATH but PREFIX comes first there.

Copy link
Member Author
@faho faho Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, maybe this should use env!("PREFIX") instead of hardcoding /usr?

This path is not dependent on where fish is installed, so using PREFIX would be wrong. E.g. if you installed fish to ~/.local/, you would still want to read terminfo from /usr/pkg/share/terminfo.

I am unsure if pkgsrc can be set to use a different path, but this is the default one.

if this is a NetBSD-specific workaround then maybe enable it only there?

This is the path used on NetBSD if you install from pkgsrc (the default package manager there), but pkgsrc itself can also be used on other platforms.

More than that, this is just one more path to search, if we find a valid terminfo entry there we should use it regardless of platform.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok that sounds fine. Would be nice to get rid of this workaround eventually

Copy link
Member Author
@faho faho Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, of course, also propose this upstream.

Unlike the FreeBSD path, I didn't because this one is a bit less generic (tho I see now they also have what I believe is the Haiku path?) and I wanted to see what looking up custom paths would look like, in case it turned out that we would have to add e.g. build options to add more search paths.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or propose a method to terminfo that allows passing a list of directories to search.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

man terminfo says that after checking TERMINFO_DIRS etc it does:

• Finally, ncurses searches these compiled-in locations:
• a list of directories (no default value), and
• the system terminfo directory, /usr/share/terminfo (the compiled-in default).

I guess "(no default value)" means it's platform specific and we have to reimplement that?

Copy link
Member Author
@faho faho Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would assume that is referring to the --with-terminfo-dirs ncurses build option.

Technically, distributions could set that one, I doubt many do. (the man page should reflect the compiled settings of your version, that "/usr/share/terminfo" is also configurable)

We already have checked a bunch of systems here, and we already have them covered, so it does not seem useful in practice.

If we did want to support this option generally, we would do it by adding a build option of our own and expecting distributions to simply apply the same setting.

Edit: Yup it is

Copy link
Member Author
@faho faho Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Debian uses --with-terminfo-dirs, but to add "/etc/terminfo:/lib/terminfo:/usr/share/terminfo", all of which are already checked by rust-terminfo. Likewise for fedora. NixOS adds "/run/current-system/sw/share/terminfo" (tho that seems to only compile it in for static binaries?).


I am inclined to merge this as-is, with the search path it has, and if anyone needs other paths we either add them or, worst case, add a build option to add more paths.

Basically we ship the typical paths that are used in practice, and if there is a need in practice for weirder setups let people supply them at build, just like they supply them to curses (they can already set $TERMINFO_DIRS at runtime).

}
pub fn setup_fallback_term() -> Arc<Term> {
let mut global_term = TERM.lock().expect("Mutex poisoned!");
// These values extracted from xterm-256color from ncurses 6.4
Copy link
Contributor
@krobelus krobelus Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sometimes send a TERM that doesn't support colors over SSH, to disable color.
But in my case the remote system has the corresponding terminfo entry, so that's fine.


Looks fine to me overall.
I don't have a strong opinion on curses versus terminfo but smaller sounds good.

I think it's generally a good idea to cut a new release before adding/bumping a dependency but we already added Rust so I guess both changes can go into the same release

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we already try xterm-256color if $TERM could not be found.

What this changes is:

  1. If TERM is xterm-256color and could not be found, we use our fallback xterm-256color, without an error message or trying the other fallbacks
  2. If we can't find any of of $TERM or our fallback TERM values, we use xterm-256color. Before, we would go on without any terminfo, and a lot of things would just break.

Both of these would be a good idea even if we kept using curses.

They are, however, more helpful with the rest of this, because it is now a bit more likely that we don't get a terminfo database (because you need to install curses to build against it, and because curses would also read the hashed database or awkward locations for that database).

faho added 4 commits February 22, 2024 17:37
This allows us to get the terminfo information without linking against curses.

That means we can get by without a bunch of awkward C-API trickery.

There is no global "cur_term" kept by a library for us that we need to invalidate.

Note that it still requires a "unhashed terminfo database", and I don't know how well it handles termcap.

I am not actually sure if there are systems that *can't* have terminfo, everything I looked at
has the ncurses terminfo available to install at least.
And use it if $TERM is xterm-256color and could not be found, *without* warning.

These barely change, especially in the parts we use.
faho added 3 commits February 22, 2024 17:40
We don't need to know that it tried these five before finally getting
one, the list is *right there*.

It is also very unlikely that someone has "xterm" or "ansi" but not "xterm-256color"

For xterm-256color, we don't warn *at all* because we have that one hardcoded.
These seems weird to add upstream, and we might want to read
more here.
@faho
Copy link
Member Author
faho commented Feb 22, 2024

Alright, I've now gone through most of https://repology.org/project/ncurses/versions, and I haven't seen any more interesting directories, and the only mention of hashed-db I found was in the Suse package, but it was conditional and downloading an actual package showed it wasn't used.

Hashed-db is also basically unusable because, as far as I can tell, it still wants berkeley db, and that has been oracle'd all over (it's AGPL and has been unmaintained since 2020).

So I'm going to merge this now, if anything is missing anywhere we can add it.


Note that writing this has made me even less enamoured with the terminfo system - I've checked what terminfo actually adds in practice and it was even less than I thought.

If we just used xterm-256color everywhere (and kept our heuristics for truecolor and cursor shaping), we would lose almost nothing. Most of the differences either don't matter (if a key sequence like khome is set to NULL we don't actually care - we won't ever get it then) or are just actually wrong (the smacs sequence for tmux-256color does not work in tmux, use the one for xterm-256color in tmux and it does) or unnecessary (tmux understands xterm's exit_attribute_mode (set_color reset) and cursor_up perfectly well).

In turn you get a ton of features that aren't in terminfo, or that take literal decades to get in there. I recently found out that there is a bracketed-paste capability. It was added in December 2022, because Bram Moolenaar (the vim author) asked. Bracketed paste was added to xterm in 2005. And I lied, it's not one capability, it's four, because of course technically a terminal could decide to support bracketed paste but with slightly different escape sequences (but otherwise exactly the same semantics because those aren't expressable in terminfo). And I have not found a single mention of these in the man pages, I only found them in the actual source.

I want away from this system. I want it to be replaced with something usable, and I want that to be a hell of a lot simpler.

@faho faho merged commit c408342 into master Feb 22, 2024
@faho faho deleted the uncurse branch February 22, 2024 19:10
@krobelus
Copy link
Contributor

xterm everywhere sounds worth trying.
One problem with terminal databases is that inside SSH or containers they may not match the client terminal.
Assuming xterm could solve some of these problems. I'm not sure what if anything would get worse.

I want away from this system. I want it to be replaced with something usable, and I want that to be a hell of a lot simpler.

Maybe XTGETTCAP which talks to the terminal directly.

@faho
Copy link
Member Author
faho commented Feb 23, 2024

Maybe XTGETTCAP which talks to the terminal directly.

I don't like waiting for a response. Especially over ssh that's not a great idea.

I want a system where most sequences are just usable. For most things we communicate to the terminal, we do not actually need a response, and we don't even need the terminal to do anything in particular. It just can't break and e.g. barf half the sequence onto the screen.

For example if we send the cursor shape or OSC-7 PWD we don't actually care if the terminal changes the cursor shape or does anything with the PWD.

And for any kind of query, a sufficiently terrible terminal can just barf the query instead of giving a response. And sufficiently terrible terminals unfortunately exist, so it isn't helpful.

What I want is a mode where the terminal just sets "$GOODTERM=1", and that means we can just use any OSC etc sequences and it will either support or ignore them. Unfortunately this requires terminal authors to talk to each other and come to an agreement and in our current reality I do not see that happening.


What I would do in practice is to simply downplay terminfo errors and fall back to our included xterm-256color without much fuss.

I would like to check where in practical use that one gets any big errors, and if we don't find anything remove the scary warnings. Unfortunately that only gets us up to what terminfo defines.

@faho faho mentioned this pull request Feb 23, 2024
@faho faho mentioned this pull request Mar 3, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

0