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

use editline instead of linenoise(-ng) #2228

Merged
merged 3 commits into from
Nov 13, 2018

Conversation

dtzWill
Copy link
Member

@dtzWill dtzWill commented Jun 13, 2018

I marked the commit as "WIP" but I don't remember
what problems or concerns I had with this.

Poking at it, things seem to work nicely?

Not sure if utf8 is supported, don't think this is needed
for anything we have in nixpkgs.

Maybe I was just unhappy about adding another dependency?

Fixes #2014 .
May be good to review recent completion bugs and look for regressions.

@dtzWill
Copy link
Member Author

dtzWill commented Jun 13, 2018

Observations so far:

  • Avoids the worst part of "nix repl" cannot tab-complete names that contain a dash ('-') #1756 (although the fix I suggest there I've used daily and seems to work great, sorry for not pushing upstream)
  • Doesn't auto-fill shared prefix of completion choices (although does complete when only 1 option available) (fixed in follow-up commit)
  • control-c during completion seems to cause some sort of queued exit
    • Update: looks like editline (configure-time option) handles sigint itself, we may want to disable this or disable our handling of it, it's likely the combination that's causing this behavior.
  • Doesn't do anything silly if you try piping to nix repl (:|nix repl)-- this was an issue previously.

@dtzWill
Copy link
Member Author

dtzWill commented Jun 14, 2018

Apparently editline is not already in upstream Nixpkgs so perhaps this is unwise? (apparently no other programs we package use it?)

@edolstra
Copy link
Member

Unwise for what reason?

@dtzWill
Copy link
Member Author

dtzWill commented Jun 14, 2018

General dependency "health", I suppose. If we are the only user it's ours to maintain/update. Sometimes there is a reason no one else uses a component, but I think we're safe here if it's what Debian uses.

@Lassulus
Copy link
Member

any progress here? The commit message still says WIP

@dtzWill
Copy link
Member Author

dtzWill commented Jun 26, 2018

I don't remember what is "wip" about it, sorry (shame on me for not making a better note of this at at the time!).
I might have just been defensive since I grafted most of this, as I recall, from the "original" readline-based repl code and am not confident in the API and our usage of it personally.
I'll try using it as my daily-driver for a bit to see how that goes...
FWIW just pushed editline to 18.03 as well so it's available if we want it without too much trouble.

I feel we'd be better off using libedit but I think that'll take non-trivial effort since it uses a different API, but presumably nothing entirely too radically different :).

@dtzWill
Copy link
Member Author

dtzWill commented Jun 26, 2018

FWIW I don't think working on this (either testing/improving what is started here, or exploring using libedit or something) requires much knowledge of anything Nix-internal, so it may be a good project for a wider audience of developers :).

@dtzWill
Copy link
Member Author

dtzWill commented Jun 26, 2018

Not inserting a shared "prefix" among completions was making me sad, so put together a commit to provide that behavior. Pushing here since I think it makes editline more usable but I'm okay if would rather merge without.

@dtzWill dtzWill force-pushed the feature/editline-pr branch from 9b98e3d to af4f014 Compare June 26, 2018 19:20
@dtzWill
Copy link
Member Author

dtzWill commented Jun 26, 2018

rebased (not quite to master due to #2249) and replaced the prefix completion with a slightly better approach.

@dtzWill
Copy link
Member Author

dtzWill commented Jul 3, 2018

Might act a bit differently when editing long lines that wrap, especially as interacting with terminal resizing?

@KaiHa
Copy link
Contributor

KaiHa commented Aug 11, 2018

I might have just been defensive since I grafted most of this, as I recall, from the "original" readline-based repl code and am not confident in the API and our usage of it personally.

[SNIP]

I feel we'd be better off using libedit but I think that'll take non-trivial effort since it uses a different API, but presumably nothing entirely too radically different :).

Hi @dtzWill, may I ask what is the reason for not using readline? From the perspective of packaging Nix for Debian it would be a huge step forward, because readline is already available as a package in Debian.

libedit (http://thrysoee.dk/editline/) would also be available in Debian.

editline (http://troglobit.com/projects/editline/) would be slightly more complicated, because it is not available in Debian (although there is a package by the name editline but it seems to be not related to the one you are using).

@edolstra
Copy link
Member

We used to use readline, but got rid of it because it doesn't depend on ncurses (c5f23f1).

@KaiHa
Copy link
Contributor

KaiHa commented Aug 25, 2018

I have created a commit on this branch that replaces editline by readline. As far as I can see this does not pull in ncurses as a dependency. [1] TAB completion (with a list of all matches) is working, and it seems to work as expected.

Would it make sense to create a pull request from it or do you see other problems when using readline?

[1]: It does not pull in ncurses as a dependency on Debian, but readline does indeed have ncurses in its closure on Nix (maybe as a termcap lib).

src/nix/repl.cc Outdated
// and read brand new input.
linenoiseHistoryAdd(input.c_str());
// We handled the current input fully, so we should clear it and read brand new input.
// TODO: history_add ?
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this TODO still needs to be done to get proper history.

@dtzWill
Copy link
Member Author

dtzWill commented Oct 29, 2018 via email

@dtzWill
Copy link
Member Author

dtzWill commented Oct 29, 2018

Conflicts resolved (merge, not rebase eep): https://github.com/dtzWill/nix/tree/feature/editline-pr-merge

Which also contains #2485 so it builds against 18.09 for testing. Will tackle a rebase and drop the TODO in a few...

@dtzWill dtzWill force-pushed the feature/editline-pr branch from af4f014 to 9f99809 Compare October 29, 2018 13:46
Bump fallback editline expression to latest in nixpkgs.
@edolstra edolstra merged commit 2a8bdfd into NixOS:master Nov 13, 2018
@edolstra
Copy link
Member

Thanks, merged!

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-repl-and-readline/31655/2

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.

linenoise embeds non-free ConvertUTF{.cpp,.h}
5 participants