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

Make libedit support more generic; port readline / libedit to FreeBSD #57710

Closed
ngie-eign mannequin opened this issue Nov 29, 2011 · 39 comments
Closed

Make libedit support more generic; port readline / libedit to FreeBSD #57710

ngie-eign mannequin opened this issue Nov 29, 2011 · 39 comments
Assignees
Labels
3.9 only security fixes 3.10 only security fixes build The build process and cross-build extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@ngie-eign
Copy link
Mannequin

ngie-eign mannequin commented Nov 29, 2011

BPO 13501
Nosy @gpshead, @ronaldoussoren, @ned-deily, @merwok, @ngie-eign, @berkerpeksag, @vadmium, @koobs, @Romain-Geissler-1A, @kellinm, @rohieb, @tnir
PRs
  • bpo-13501 Build or disable readline with Editline #12076
  • bpo-38634: Allow non-apple build to cope with libedit #16986
  • bpo-13501: allow choosing between readline and libedit #24189
  • [3.9] bpo-13501: allow choosing between readline and libedit #25420
  • Files
  • python-port-readline-module-to-libedit-on-freebsd.patch
  • editline.v2.patch: Updated patch
  • editline.v3.patch
  • editline.v4.patch
  • 0001-Build-or-disable-readline-module-with-Editline.patch: Rebase to Master
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/gpshead'
    closed_at = <Date 2021-02-09.01:08:37.505>
    created_at = <Date 2011-11-29.15:19:01.712>
    labels = ['extension-modules', '3.10', 'type-feature', '3.9', 'build']
    title = 'Make libedit support more generic; port readline / libedit to FreeBSD'
    updated_at = <Date 2021-04-15.06:36:36.529>
    user = 'https://github.com/ngie-eign'

    bugs.python.org fields:

    activity = <Date 2021-04-15.06:36:36.529>
    actor = 'tnir'
    assignee = 'gregory.p.smith'
    closed = True
    closed_date = <Date 2021-02-09.01:08:37.505>
    closer = 'gregory.p.smith'
    components = ['Build', 'Extension Modules']
    creation = <Date 2011-11-29.15:19:01.712>
    creator = 'ngie'
    dependencies = []
    files = ['23806', '39480', '42241', '43096', '48175']
    hgrepos = []
    issue_num = 13501
    keywords = ['patch']
    message_count = 39.0
    messages = ['148577', '148804', '148805', '148812', '243973', '244882', '244883', '244885', '244887', '245395', '245397', '245412', '245420', '260066', '260067', '260212', '262061', '262162', '266877', '323760', '328724', '328726', '328737', '328770', '336697', '336780', '339302', '344991', '349574', '379407', '380120', '382947', '382948', '384780', '386675', '386677', '386679', '386684', '386694']
    nosy_count = 16.0
    nosy_names = ['gregory.p.smith', 'ronaldoussoren', 'donmez', 'ned.deily', 'eric.araujo', 'ngie', 'egaudry', 'berker.peksag', 'martin.panter', 'koobs', 'emaste', 'Mark Harfouche', 'RomainGeissler1A', 'Robert Marshall', 'rhi', 'tnir']
    pr_nums = ['12076', '16986', '24189', '25420']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'commit review'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue13501'
    versions = ['Python 3.9', 'Python 3.10']

    @ngie-eign
    Copy link
    Mannequin Author

    ngie-eign mannequin commented Nov 29, 2011

    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..

    @ngie-eign ngie-eign mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Nov 29, 2011
    @pitrou pitrou added the build The build process and cross-build label Nov 29, 2011
    @ned-deily ned-deily self-assigned this Nov 29, 2011
    @merwok
    Copy link
    Member

    merwok commented Dec 3, 2011

    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).

    @merwok
    Copy link
    Member

    merwok commented Dec 3, 2011

    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.

    @ned-deily
    Copy link
    Member

    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.

    @vadmium
    Copy link
    Member

    vadmium commented May 24, 2015

    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.

    @vadmium vadmium added extension-modules C modules in the Modules dir and removed stdlib Python modules in the Lib dir labels May 24, 2015
    @ned-deily
    Copy link
    Member

    See also suggested patch in bpo-24388.

    @emaste
    Copy link
    Mannequin

    emaste mannequin commented Jun 6, 2015

    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?

    @emaste
    Copy link
    Mannequin

    emaste mannequin commented Jun 6, 2015

    Note that the patch in bpo-24388 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?

    @ned-deily
    Copy link
    Member

    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.)

    @emaste
    Copy link
    Mannequin

    emaste mannequin commented Jun 15, 2015

    It looks like rust developers hit the issue in bpo-24388 with lldb on Ubuntu 15.04 as well: rust-lang/rust#26297

    @emaste
    Copy link
    Mannequin

    emaste mannequin commented Jun 15, 2015

    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 bpo-24388.

    @vadmium
    Copy link
    Member

    vadmium commented Jun 15, 2015

    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 bpo-18458?

    @emaste
    Copy link
    Mannequin

    emaste mannequin commented Jun 16, 2015

    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.

    @ned-deily
    Copy link
    Member

    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.

    @ned-deily ned-deily removed their assignment Feb 11, 2016
    @ned-deily
    Copy link
    Member

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 12, 2016

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Mar 20, 2016

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Mar 22, 2016

    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 bpo-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.

    @vadmium
    Copy link
    Member

    vadmium commented Jun 2, 2016

    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.

    @MarkHarfouche
    Copy link
    Mannequin

    MarkHarfouche mannequin commented Aug 19, 2018

    Is there still interest in this?

    @Romain-Geissler-1A
    Copy link
    Mannequin

    Romain-Geissler-1A mannequin commented Oct 28, 2018

    Hi,

    Yes this bug report is still valid. I just tried compiling Python 3.7.1 on linux with libedit installed (https://thrysoee.dk/editline/) but not libreadline, and indeed the configure script fails to detect I have it installed and falls back on the python readline emulation.

    Note: I am using Linux, not Mac OS.

    Cheers,
    Romain

    @MarkHarfouche
    Copy link
    Mannequin

    MarkHarfouche mannequin commented Oct 28, 2018

    My workaround has been to use neither readline nor libedit.

    On Sun, Oct 28, 2018 at 2:40 PM Romain Geissler <report@bugs.python.org>
    wrote:

    Romain Geissler <romain.geissler@amadeus.com> added the comment:

    Hi,

    Yes this bug report is still valid. I just tried compiling Python 3.7.1 on
    linux with libedit installed (https://thrysoee.dk/editline/) but not
    libreadline, and indeed the configure script fails to detect I have it
    installed and falls back on the python readline emulation.

    Note: I am using Linux, not Mac OS.

    Cheers,
    Romain

    ----------
    nosy: +RomainGeissler1A


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue13501\>


    @Romain-Geissler-1A
    Copy link
    Mannequin

    Romain-Geissler-1A mannequin commented Oct 28, 2018

    Yes in my case the build works, but then I have a resulting python interpreter with limited readline functionality. That's why I have some interest in having this patch rebased and included into trunk.

    @MarkHarfouche
    Copy link
    Mannequin

    MarkHarfouche mannequin commented Oct 28, 2018

    Right, but when do you actually use the bare python interpreter? I'm always
    on ipython or something that uses that as the backend.

    On Sun, Oct 28, 2018 at 4:12 PM Romain Geissler <report@bugs.python.org>
    wrote:

    Romain Geissler <romain.geissler@amadeus.com> added the comment:

    Yes in my case the build works, but then I have a resulting python
    interpreter with limited readline functionality. That's why I have some
    interest in having this patch rebased and included into trunk.

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue13501\>


    @ngie-eign
    Copy link
    Mannequin Author

    ngie-eign mannequin commented Feb 26, 2019

    I'll try to rebase Martin's changes, as they don't apply to the master branch on GitHub today.

    @ngie-eign ngie-eign mannequin added 3.7 (EOL) end of life 3.8 only security fixes labels Feb 26, 2019
    @kellinm
    Copy link
    Mannequin

    kellinm mannequin commented Feb 27, 2019

    I have rebased this to recent master hash 53b9e1a and submitted a pull request.

    It built in my CI environment, so seems to be OK as far as compiling.

    I'll run the test suite against it later this evening when I have time to stand up a quick build enviroment with the full dependency list.

    @kellinm
    Copy link
    Mannequin

    kellinm mannequin commented Apr 1, 2019

    The pull request is now ready for a review.

    Apologies for the delay - took me a bit longer to circle back on this than I would have liked.

    @gpshead
    Copy link
    Member

    gpshead commented Jun 7, 2019

    In PR it you refer to -ledit and to editline. libedit and libeditline appear to be two different libraries.

    I was under the impression that libedit is the desirable one (it's what *BSD uses - coming originally from NetBSD if I understand correctly - which is how it wound up being macOS's choice). I could also be wrong.

    Is there a relationship between libedit and libeditline? They both look like useful non-GPL readline alternatives.

    We're building CPython internally at Google with libedit instead of readline of late and our initial patch to do this looks similar (though simpler) than your PR's readline.c changes. The #ifdef __APPLE__ checks are replaced with a #ifdef HAVE_LIBEDIT check which we set in our pyconfig.h.

    I'm interested in seeing this stuff get in one way or another, there's no reason we should tie people to GNU readline. :)

    @gpshead gpshead self-assigned this Jun 7, 2019
    @egaudry
    Copy link
    Mannequin

    egaudry mannequin commented Aug 13, 2019

    Hi, I think that there are some issues in the last patch (0001-Build-or-disable-readline-module-with-Editline.patch) made available.

    For instance, the using_libedit_emulation variable should be protected by DETECT_EDITLINE ifdef (and not SUPPORT_EDITLINE). And the setup.py file mentioned a 'readline_lib' string instead of the readline_lib variable.

    I am interested by being able to build python without readline but libedit instead too.

    @gpshead gpshead added 3.9 only security fixes and removed 3.7 (EOL) end of life labels Dec 9, 2019
    @gpshead gpshead added 3.10 only security fixes and removed 3.8 only security fixes labels Oct 20, 2020
    @ngie-eign
    Copy link
    Mannequin Author

    ngie-eign mannequin commented Oct 23, 2020

    Is there something I can do to help move this change along? 8 years is a long time for an issue to be open :(.

    @gpshead
    Copy link
    Member

    gpshead commented Nov 1, 2020

    at a glance, it looks like the PR needs updating.

    @rohieb
    Copy link
    Mannequin

    rohieb mannequin commented Dec 14, 2020

    What's the status of this patch? Is it still needed after 68669ef, which went into v3.8.1?

    @rohieb
    Copy link
    Mannequin

    rohieb mannequin commented Dec 14, 2020

    (That was meant to be 68669ef for the autolinking feature)

    @rohieb
    Copy link
    Mannequin

    rohieb mannequin commented Jan 10, 2021

    Okay, I've had a look at this, and it seems that all the heavy lifting is already included in 68669ef (via bpo-38634, PR 16986). The selection of libedit vs. readline still needs to be ported. I've prepared (and tested) PR 24189 for the rebase of this patch to the current master.

    @gpshead
    Copy link
    Member

    gpshead commented Feb 9, 2021

    New changeset e1f7769 by Roland Hieber in branch 'master':
    bpo-13501: allow choosing between readline and libedit (GH-24189)
    e1f7769

    @gpshead
    Copy link
    Member

    gpshead commented Feb 9, 2021

    I'm closing this as I believe everything we need done is done at this point. Open new issues if there are remaining libedit vs libreadline things to take care of.

    Thanks everyone!

    @gpshead gpshead closed this as completed Feb 9, 2021
    @koobs
    Copy link

    koobs commented Feb 9, 2021

    Would we like the freebsd worker CI configurations to be modified to use ports/package versions of libedit (or readline, or whatever) to ensure converage?

    Right now the defaults are used (whatever edit library that entails)

    @gpshead
    Copy link
    Member

    gpshead commented Feb 9, 2021

    That seems like a good idea to prevent regressions if anyone knows how to do that.

    For python.org/dev/'s buildbot fleet, the configuration of what configure and make flags happens via https://github.com/python/buildmaster-config/tree/master/master/custom - note the 'def setup' overriding that needs to be done in factories.py so that older branches {'3.7', '3.8', '3.9'} are not configured using the flag.

    Coordinate with the owner of a particular buildbot to make sure the system has the requisite installed libraries and headers.

    I manually tested a Linux build against libedit, it builds and works. But test_readline has failures. tracking those in https://bugs.python.org/issue43172

    @koobs
    Copy link

    koobs commented Feb 9, 2021

    @gregory I'm the worker owner and can coordinate buildbot worker changes with Zach or Victor

    FreeBSD has:

    editline (in FreeBSD base, installed in /usr/ )
    libedit in ports/packages (installs in /usr/local)
    readline in ports/packages (installs in /usr/local)

    Which combination would get us the best coverage?

    Also, if both libedit and readline are installed from ports, does configure have a preference/default or will it barf?

    Primary motivation behind the questions is that the Python build system is notoriously flaky with respect to third party libs in various locations (we've had issues with gettext, ssl, expat, among a few others)

    I'm happy to install whatever you'd like on the workers at your convenience to the tests can be run as a custom build (if they haven't been already)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes 3.10 only security fixes build The build process and cross-build extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants