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

linenoise embeds non-free ConvertUTF{.cpp,.h} #2014

Closed
infinity0 opened this issue Mar 27, 2018 · 17 comments · Fixed by #2228
Closed

linenoise embeds non-free ConvertUTF{.cpp,.h} #2014

infinity0 opened this issue Mar 27, 2018 · 17 comments · Fixed by #2228
Assignees

Comments

@infinity0
Copy link

Hi, in the process of Debian packaging this piece of software we noticed that the source code of nix includes non-free (and buggy, unmaintained, obsolete) code from Unicode Inc, specifically in the linenoise embedded library.

The solution as suggested by Debian (and adopted by Ghostscript) is to rewrite the code to use libicu instead.

@dtzWill
Copy link
Member

dtzWill commented Mar 27, 2018

Hmm well that's unfortunate. Following your links (thanks!) I found that many projects include this, to the dismay of Debian packaging teams I'm sure. One project jumped at me, LLVM:

https://lintian.debian.org/full/[email protected]#llvm-toolchain-6.0_1_x3a6.0~_x2brc2-1

Which is interesting because:

ICU seems an awfully heavy dependency, I'm not even sure what Nix's official stance on UTF8 is anyway --perhaps removing utf8 support from our REPL is best short-term solution?

@shlevy
Copy link
Member

shlevy commented Mar 27, 2018

👍 for removing UTF8 support for now. ICU is pretty damn heavy... Maybe some day we can have it enabled optionally at runtime if it's available.

@dtzWill
Copy link
Member

dtzWill commented Mar 28, 2018

(Perhaps moving to editline would be worth (re-)considering?)

@shlevy
Copy link
Member

shlevy commented Mar 28, 2018

@dtzWill How hard would it be to quickly remove utf-8 support while we investigate longer-term solutions?

@edolstra
Copy link
Member

Just revert 21f5157? What was the reason for switching to linenoise-ng anyway?

@dtzWill
Copy link
Member

dtzWill commented Mar 28, 2018

It doesn't look very easy to remove. We could also revert to linenoise (not linenoise-ng) although that'd be unfortunate functionality-wise.

@dtzWill
Copy link
Member

dtzWill commented Mar 28, 2018

Since I was curious what it would take / act like, here's a quick port to using editline (no ncurses): https://github.com/dtzWill/nix/tree/feature/editline

Using linenoise-ng was done so that completions were listed instead of only being available one-at-a-time. See #1703 .

@edolstra
Copy link
Member

@dtzWill Switching to editline seems good to me in principle.

@infinity0
Copy link
Author

The issue was reported to LLVM upstream and they have it marked as "P release blocker" for nearly a year now, sigh.

editline sounds good to me, removing an embedded library is an extra bonus IMO.

@dtzWill
Copy link
Member

dtzWill commented Mar 28, 2018

Reverting to linenoise (no -ng) and picking up the contents of a PR that optionally list completions:

https://github.com/dtzWill/nix/tree/experimental/linenoise-list-all

(I just grabbed the code from the PR, we may wish to merge that with linenoise master for fixes?)

Not sure what is best way to go, but if not editline then modifying linenoise (as in that branch) looks pretty easy--much easier than de-UTF8'ing linenoise-ng.

Hope this helps!

@shlevy
Copy link
Member

shlevy commented Mar 29, 2018

@dtzWill I'd prefer to defer to your technical opinion here, given that we're not OK embedding ConvertUTF. Should we do linenoise or editline? Can you open a PR for whichever?

@dtzWill
Copy link
Member

dtzWill commented Mar 29, 2018

Okay! I probably won't be able to get to this for a few days at least, but if that's alright I'll put together a PR for folks to review/test out.

@cstrahan
Copy link
Contributor

FWIW, I've opened an issue upstream (and in a fork that looks nice):

@KaiHa
Copy link
Contributor

KaiHa commented Jun 13, 2018

Hi @dtzWill, @shlevy, are you still planning to work on this issue. It would be great if we can get this solved.

@dtzWill
Copy link
Member

dtzWill commented Jun 13, 2018

rain1's linenoise fork looks promising: https://github.com/rain-1/linenoise .

Listing more than one completion is moderately important to me. Looks like rain1's fork has that change under review. Not sure what the problem with editline was.

@dtzWill
Copy link
Member

dtzWill commented Jun 13, 2018

I'm still not sure why using libeditline wasn't the clear winner, haha. Maybe because I didn't have a good way to vendor it?

Poking at the branch I linked above, seems good to me?
https://github.com/dtzWill/nix/tree/feature/editline

I fear I'm missing something, but for now I'll create a PR for it so we can poke at it and see if it works as we'd like.

@nixos-discourse
Copy link

This issue 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 a pull request may close this issue.

7 participants