| Message ID | 20190916124203.31380-1-jeremy@azazel.net |
|---|---|
| Headers | show |
| Series | Add Linenoise support to the CLI. | expand |
On Mon, Sep 16, 2019 at 01:41:59PM +0100, Jeremy Sowden wrote: > Sebastian Priebe [0] requested Linenoise support for the CLI as an > alternative to Readline, so I thought I'd have a go at providing it. > Linenoise is a minimal, zero-config, BSD licensed, Readline replacement > used in Redis, MongoDB, and Android [1]. > > 0 - https://lore.kernel.org/netfilter-devel/4df20614cd10434b9f91080d0862dd0c@de.sii.group/ > 1 - https://github.com/antirez/linenoise/ > > The upstream repo doesn't contain the infrastructure for building or > installing libraries. I've taken a look at how Redis and MongoDB handle > this, and they both include the upstream sources in their trees (MongoDB > actually uses a C++ fork, Linenoise NG [2]), so I've done the same. > > 2 - https://github.com/arangodb/linenoise-ng > > Initially, I added the Linenoise header to include/ and the source to > src/, but the compiler warning flags used by upstream differ from those > used by nftables, which meant that the compiler emitted warnings when > compiling the Linenoise source and I had to edit it to fix them. Could you silent these warnings via CFLAGS just like we do with mini-gmp.{c,h}? We already cache a copy of mini-gmp.c in the tree, this would follow the same approach, just the source under src/ and the header in include/. > Since they were benign and editing the source would make it more > complicated to update from upstream in the future, I have, instead, > chosen to put everything in a separate linenoise/ directory with its > own Makefile.am and the same warning flags as upstream. > > By default, the CLI continues to be build using Readline, but passing > `with-cli=linenoise` instead causes Linenoise to be used instead. Probably good if you can also update 'nft -v' to display that nft is compiled with/without mini-gmp and also with either libreadline/linenoise. > The first two patches do a bit of tidying; the third patch adds the > Linenoise sources; the last adds Linenoise support to the CLI. No objections, please update tests/build/ to check for this new ./configure option. Thanks.
On 2019-09-20, at 12:15:20 +0200, Pablo Neira Ayuso wrote: > On Mon, Sep 16, 2019 at 01:41:59PM +0100, Jeremy Sowden wrote: > > Sebastian Priebe [0] requested Linenoise support for the CLI as an > > alternative to Readline, so I thought I'd have a go at providing it. > > Linenoise is a minimal, zero-config, BSD licensed, Readline > > replacement used in Redis, MongoDB, and Android [1]. > > > > 0 - https://lore.kernel.org/netfilter-devel/4df20614cd10434b9f91080d0862dd0c@de.sii.group/ > > 1 - https://github.com/antirez/linenoise/ > > > > The upstream repo doesn't contain the infrastructure for building or > > installing libraries. I've taken a look at how Redis and MongoDB > > handle this, and they both include the upstream sources in their > > trees (MongoDB actually uses a C++ fork, Linenoise NG [2]), so I've > > done the same. > > > > 2 - https://github.com/arangodb/linenoise-ng > > > > Initially, I added the Linenoise header to include/ and the source > > to src/, but the compiler warning flags used by upstream differ from > > those used by nftables, which meant that the compiler emitted > > warnings when compiling the Linenoise source and I had to edit it to > > fix them. > > Could you silent these warnings via CFLAGS just like we do with > mini-gmp.{c,h}? We already cache a copy of mini-gmp.c in the tree, > this would follow the same approach, just the source under src/ and > the header in include/. Ah, yes, thanks for the pointer. > > Since they were benign and editing the source would make it more > > complicated to update from upstream in the future, I have, instead, > > chosen to put everything in a separate linenoise/ directory with its > > own Makefile.am and the same warning flags as upstream. > > > > By default, the CLI continues to be build using Readline, but > > passing `with-cli=linenoise` instead causes Linenoise to be used > > instead. > > Probably good if you can also update 'nft -v' to display that nft is > compiled with/without mini-gmp and also with either > libreadline/linenoise. Will do. > > The first two patches do a bit of tidying; the third patch adds the > > Linenoise sources; the last adds Linenoise support to the CLI. > > No objections, please update tests/build/ to check for this new > ./configure option. Will do. J.