classification
Title: Make libedit support more generic; port readline / libedit to FreeBSD
Type: enhancement Stage: patch review
Components: Build, Extension Modules Versions: Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, donmez, emaste, haypo, koobs, martin.panter, merwok, ned.deily, ronaldoussoren, yaneurabeya
Priority: normal Keywords: patch

Created on 2011-11-29 15:19 by yaneurabeya, last changed 2016-06-02 06:31 by martin.panter.

Files
File name Uploaded Description Edit
python-port-readline-module-to-libedit-on-freebsd.patch yaneurabeya, 2011-11-29 15:18 review
editline.v2.patch martin.panter, 2015-05-24 09:46 Updated patch review
editline.v3.patch martin.panter, 2016-03-22 05:57 review
editline.v4.patch martin.panter, 2016-06-02 06:31 review
Messages (19)
msg148577 - (view) Author: NGie Cooper (yaneurabeya) Date: 2011-11-29 15:18
The attached patch enables libedit support in a generic way via configure.in, so I can pass in --with-readline=editline, --with-readline=readline, --with-readline=yes, or --with[out]-readline[=no] and it will do one of the following:

1. Enable editline support.
2. Enable readline support.
3. Enable default OS support (editline on OSX, readline otherwise).
4. Explicitly disable readline support.

Tested functional via cmd.py and with basic poking around via the readline module.

The attached patch was created against the 2.7 branch, but I'll produce a patch against 'trunk' sometime later on this week.

PS. The only quirk I found was the fact that FreeBSD 9's libedit lied when it reports the number of available history items. Hrmmm..
msg148804 - (view) Author: Éric Araujo (merwok) * (Python committer) Date: 2011-12-03 14:53
ISTM --with-readline=yes should just be --with-readline, and the =no forms should just be --without-readline.  That would be more in line with other options and less confusing (--without-readline=no ?!).

There is no trunk anymore now that we’ve switched away from Subversion; you should probably work on the Mercurial default branch (see devguide).
msg148805 - (view) Author: Éric Araujo (merwok) * (Python committer) Date: 2011-12-03 14:54
And if this is really two different requests (port readline module to FreeBSD i.e. change if __APPLE__ to if HAVE_EDITLINE and give more control about readline vs. editline in the configure script), then two reports should be opened.
msg148812 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2011-12-03 18:05
Without having yet done a detailed review of the patch and the configure options, I don't see a need to open a second issue.  The scope of this one is fine: generalizing the support of libedit to other platforms.
msg243973 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-05-24 09:46
Garrett Cooper: If you are still around, you might want to review the following three changes in this new patch. I suspect they were mistakes in your version, but I cannot be 100 percent sure.

* Restoring length - 1 subtraction in call_readline() at line 1239
* Change #if __APPLE__ back to #ifdef
* Gnu Readline requires <stdio.h> to be included first, so I fixed the configure.ac script

Other changes in this new patch:

* Merged with current code
* Remove outdated comment about using a runtime library check
* Couple minor corrections to comments, conditional variable declarations, etc
* Fixed wrong HIST_ENTRY pointer type used in a Gnu Readline conditional branch
* Changed double single quote sign back to single single quote. I am no Autocrap expert, but it screwed up the generated comment in pyconfig.h.in.
* Add ability to include with <editline/readline.h>

Tested on Arch Linux with both Gnu Readline 6.3.008 and Editline 20150325-3.1 available:

No readline argument: Uses Gnu Readline
--with-readline=editline: Uses Editline
--with-readline=readline: Gnu Readline
--with-readline=yes: Gnu Readline
--with-readline: Gnu Readline
--with-readline=no: No “readline” module
--without-readline: No “readline” module
--without-readline=no: error: invalid package name: readline=no

However after successfully compiling with Editline, there are a couple bugs with keystrokes or output not being synchronized. I do not think I will spent much more effort on it. But perhaps other people are interested in taking this further.
msg244882 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2015-06-06 01:58
See also suggested patch in Issue24388.
msg244883 - (view) Author: Ed Maste (emaste) Date: 2015-06-06 02:02
This issue causes the LLDB debugger to crash on FreeBSD (it uses Python as its embedded script interpreter).

What needs to be done to make some progress on this issue?
msg244885 - (view) Author: Ed Maste (emaste) Date: 2015-06-06 02:07
Note that the patch in Issue24388 is more a proof of concept. I'm not sure it's the "right" fix.

LLDB is a bit of a special case: LLDB links against libedit, but the Python libedit module is built as if readline is in use. It turns out this "magically" works out, presumably due to the runtime workaround detection. As far as I know this issue would affect Linux as well, but perhaps the version of libedit on common Linux distributions is one with the 0-based vs 1-based history fix?
msg244887 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2015-06-06 02:24
The suggested change to generalize support for libedit to other platforms is a new feature so, by default, it would first appear in a new feature release, e.g. 3.6.  There would probably have to be an exception granted to add it to 2.7.x or other 3.x releases, which are in maintenance mode.  I think the quickest workarounds are to either link Python with GNU readline or, if that is not acceptable, persuade FreeBSD to carry a patch similar to the one you suggested for their current versions of Python.  WRT Linux, it looks like Debian has both a libeditline0 package (presumably the old ABI) and a libedit2 package and their lldb package is linked with the latter.  (The Debian Pythons are always linked with GNU readline.)
msg245395 - (view) Author: Ed Maste (emaste) Date: 2015-06-15 17:14
It looks like rust developers hit the issue in Issue24388 with lldb on Ubuntu 15.04 as well: https://github.com/rust-lang/rust/issues/26297
msg245397 - (view) Author: Ed Maste (emaste) Date: 2015-06-15 17:23
Actually, in msg245395 I should claim the issue is with libedit / GNU readline compatibility and/or the workarounds in Python's readline module, not that it's specifically Issue24388.
msg245412 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-06-15 23:13
Maybe I am missing something, but is it possible to use a newer version of Editline (libedit) that fixes the compatibility bug, as mentioned in Issue 18458?
msg245420 - (view) Author: Ed Maste (emaste) Date: 2015-06-16 13:23
I believe the 0-based vs 1-based history is only one of a few different inconsistencies between libedit and readline. Workarounds will be necessary until a fixed libedit is deployed on all operating systems / distros of interest, but yes I agree that eventually they should not be needed.
msg260066 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-02-11 02:12
Martin, thanks for updating the patch. I've left some review comments on Rietveld.  After reviewing it, I think Garrett's original specification is correct: there is a need for four options to preserve current expected behavior although the default is slightly more complicated than stated.  The current behavior is: enable building readline with GNU readline if found on the search paths, possibly modified by CPPFLAGS and LDFLAGS, and, if not found and on OS X, use editline if found on the search paths.  Some build scripts depend on that behavior.
msg260067 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-02-11 02:17
That said, it *might* be OK to change the default behavior to just remove the "and on OS X" condition:  enable building readline with GNU readline if found on the search paths, possibly modified by CPPFLAGS and LDFLAGS, and, if not found, use editline if found on the search paths.  That's probably the desired behavior on FreeBSD systems where, AFAIK, GNU readline isn't shipped by default but BSD editline is.
msg260212 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-12 23:34
Thanks for the review Ned. I haven’t looked too closely but your comments sound reasonable in general. I don’t have a personal need to update this patch (Editline doesn’t work well for me on Linux), but I can try to update it if there is demand for this.

Regarding the leftover #ifdef __APPLE__ bits, I would have to double-check, but I presume I didn’t need to enable those for my version of Editline, so I presumed they were Apple-specific bugs.
msg262061 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-20 07:40
Quoting Ned: “The current behavior is: enable building readline with GNU readline if found on the search paths, . . . and, if not found and on OS X, use editline if found on the search paths.  Some build scripts depend on that behavior.”

Can you point out any logic (e.g. in configure.ac) that handles this? I suspect you might be mistaken. My understanding is that we always include <readline/readline.h> and always link against -lreadline, regardless of platform. My guess is that on OS X, the include file and library are actually pointers to Editline code, rather than Gnu code.

This is in contrast to my situation on Arch Linux, where I can install both the Gnu “readline” and the “libedit” packages without conflict. Gnu Readline is accessible via <readline/readline.h> and -lreadline, and Editline’s Readline compatibility is accessible via <editline/readline.h> and -ledit.

So I suspect there is no special on-OS-X condition to change or remove (if we ignore those #ifdef __APPLE__ bug workarounds for the moment). Maybe I should drop that DEFAULT_LIBREADLINE business from the patch.
msg262162 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-22 05:57
Patch v3 is an update taking into account Ned’s comments, and my own now deeper understanding:

* Reworked the --with-readline handling. Suggested options are --with-readline=editline, and --without-readline. The current behaviour (use -lreadline if possible) should still work by default.
* Adjust the style of the configure.ac syntax to match surrounding code.
* Restore rl_completion_suppress_append check, independent of rl_completion_append_character.
* Remove completion_matches() #ifdef madness, originally spawned by Issue 1703270, which makes no sense to me.
* Restored the runtime Editline checks protected by __APPLE__, but if WITH_EDITLINE is specified (e.g. on other platforms) the Editline support is always enabled and there are no runtime checks.
msg266877 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-02 06:31
Patch v4 resolves conflicts against the current code. I also used AC_CHECK_DECL invocations instead of AC_COMPILE_IFELSE in the configure script.

FTR I think my problems with the prompt and the terminal settings are caused by a line of code that is commented out in the Apple version. See <https://gnats.netbsd.org/48957> and <http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/readline.c.diff?r1=1.110&r2=1.111>. Also, the following test fails for me when using Editline:

======================================================================
ERROR: testHistoryUpdates (test.test_readline.TestHistoryManipulation)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/proj/python/cpython/Lib/test/test_readline.py", line 38, in testHistoryUpdates
    readline.replace_history_item(0, "replaced line")
ValueError: No history item at position 0

Third, test_zipimport passes, but messes up the terminal (staircase effect). Probably related to the Apple patch mentioned above.
History
Date User Action Args
2017-03-23 11:24:13martin.panterlinkissue20631 superseder
2016-06-02 06:31:11martin.pantersetfiles: + editline.v4.patch

messages: + msg266877
2016-03-22 05:57:57martin.pantersetfiles: + editline.v3.patch

messages: + msg262162
2016-03-20 07:40:49martin.pantersetmessages: + msg262061
2016-02-12 23:34:11martin.pantersetmessages: + msg260212
2016-02-11 02:17:37ned.deilysetmessages: + msg260067
2016-02-11 02:12:47ned.deilysetassignee: ned.deily ->
messages: + msg260066
2016-02-04 02:56:50berker.peksagsetnosy: + berker.peksag
2015-06-19 11:59:48donmezsetnosy: + donmez
2015-06-16 13:23:08emastesetmessages: + msg245420
2015-06-15 23:13:02martin.pantersetmessages: + msg245412
2015-06-15 17:26:45koobssetnosy: + koobs
2015-06-15 17:23:10emastesetmessages: + msg245397
2015-06-15 17:14:11emastesetmessages: + msg245395
2015-06-06 02:24:45ned.deilysetmessages: + msg244887
2015-06-06 02:07:53emastesetmessages: + msg244885
2015-06-06 02:02:12emastesetmessages: + msg244883
2015-06-06 01:58:45ned.deilysetnosy: + emaste

messages: + msg244882
stage: needs patch -> patch review
2015-06-06 01:57:22ned.deilylinkissue24388 superseder
2015-05-24 09:47:31martin.panterlinkissue18459 superseder
2015-05-24 09:46:19martin.pantersetfiles: + editline.v2.patch

components: + Extension Modules, - Library (Lib)
versions: + Python 3.6, - Python 3.3
nosy: + martin.panter

messages: + msg243973
stage: patch review -> needs patch
2011-12-03 18:05:08ned.deilysetmessages: + msg148812
2011-12-03 14:54:46merwoksetmessages: + msg148805
2011-12-03 14:53:32merwoksetnosy: + merwok
messages: + msg148804
2011-11-29 18:48:15hayposetnosy: + haypo
2011-11-29 18:35:59ned.deilysetassignee: ned.deily
2011-11-29 15:21:11pitrousetnosy: + ronaldoussoren, ned.deily
stage: patch review

components: + Build
versions: + Python 3.3, - Python 2.7, Python 3.4
2011-11-29 15:19:01yaneurabeyacreate