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

Jump to bottom

Cross-build _curses failed if host ncurses headers and target ncurses headers have different layouts #72377

Open
yan12125 mannequin opened this issue Sep 17, 2016 · 23 comments
Open

Cross-build _curses failed if host ncurses headers and target ncurses headers have different layouts #72377

yan12125 mannequin opened this issue Sep 17, 2016 · 23 comments
Labels
3.7 (EOL) end of life build The build process and cross-build

Comments

@yan12125
Copy link
Mannequin

yan12125 mannequin commented Sep 17, 2016

BPO 28190
Nosy @doko42, @serhiy-storchaka, @ma8ma, @moreati, @yan12125
Files
  • ncurses.patch
  • cygwin-test_curses.log: ran without skip condition
  • ncurses-headers.patch: Patch version 2, autoreconf necessary
  • no-path-to-ncursesw.patch
  • no-path-to-ncursesw_2.patch
    		•

    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 = None
closed_at = None
created_at = <Date 2016-09-17.17:20:08.116>
labels = ['build', '3.7']
title = 'Cross-build _curses failed if host ncurses headers and target ncurses headers have different layouts'
updated_at = <Date 2019-12-10.08:03:17.734>
user = 'https://github.com/yan12125'

    bugs.python.org fields:

    activity = <Date 2019-12-10.08:03:17.734>
actor = 'xdegaye'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Cross-Build']
creation = <Date 2016-09-17.17:20:08.116>
creator = 'yan12125'
dependencies = []
files = ['44718', '44961', '45790', '45831', '45850']
hgrepos = []
issue_num = 28190
keywords = ['patch']
message_count = 23.0
messages = ['276806', '276807', '277430', '278051', '278052', '278060', '282647', '282698', '282782', '282806', '282822', '282838', '282839', '282840', '282844', '282848', '282912', '282915', '282921', '283111', '338884', '338889', '339591']
nosy_count = 6.0
nosy_names = ['doko', 'python-dev', 'serhiy.storchaka', 'masamoto', 'Alex.Willmer', 'yan12125']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'resolved'
status = 'open'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue28190'
versions = ['Python 3.6', 'Python 3.7']
    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Sep 17, 2016

    In changeset 919259054621 (from bpo-12567) and 707761d59a4a (from bpo-15268), /usr/include/ncursesw was added to include paths in setup.py and configure.ac. This is wrong and breaks cross-compiling. For example, if host has /usr/include/ncursesw/ncurses.h and target has $SYSROOT/include/ncurses.h, the build fails. An example can be found in [1].

    My patch removes all references to /usr/include/ncursesw and uses a robust detection, which is suitable for both native builds and cross builds.

    Added the authors of aforementioned changesets.

    [1] https://s3.amazonaws.com/archive.travis-ci.org/jobs/159936249/log.txt

    @yan12125 yan12125 mannequin added build The build process and cross-build 3.7 (EOL) end of life labels Sep 17, 2016
    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Sep 17, 2016

    Hmmm, I don't know why Rietveld failed to recognize changes in configure. Maybe it's because I've modified the patch file manually?

    @yan12125 yan12125 mannequin changed the title Cross-build _curses failed if host ncurses headers and target ncurses headers have different layouts Detect curses headers correctly for cross-compiling Sep 25, 2016
    @doko42
    Copy link
    Member

    doko42 commented Sep 26, 2016

    looks good to me, thanks for working on this.

    @ma8ma
    Copy link
    Mannequin

    ma8ma mannequin commented Oct 4, 2016

    Now, Cygwin platform is able to build core interpreter on default branch. But the curses module has been failed to build. Therefore I tried to build curses module on Cygwin (Vista x86) using this patch. And it has been succeeded.
    The patch effect at build time removes one compiler option "-I/usr/include/ncursesw". Maybe that modification position is setup.py:1352. I couldn't confirm why it has became success.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Oct 4, 2016

    Hmm it's surprising for me that an irrelevant patch fixes issues on Cygwin. Does test_curses pass?

    @ma8ma
    Copy link
    Mannequin

    ma8ma mannequin commented Oct 4, 2016

    test_curses has been skipped almost. the skip reason has been written "cygwin's curses mostly just hangs" in test case class.
    I removed the skip condition, and ran test. Tests was almost passed. The only, unget_wch was failed by edge case '\U0010FFFF', because the wchar_t size on windows platfrom is two bytes. I'll upload test log together.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Dec 7, 2016

    A clean patch without changes in ./configure. autoreconf necessary

    @doko42
    Copy link
    Member

    doko42 commented Dec 8, 2016

    the upstream ncurses has the ncursesw subdirinclude name, so you apparently can assume that this directory name is fixed *iff* it exists, however the ncursesw installation can be found in <prefix>/include instead. Can your changes cope with that?

    second issue is that you apparently don't do the changes for term.h (and possibly for other headers in other places as well, I only looked at the diff), so you mix ncursesw and ncurses headers. So you have to make these changes in all other places as well.

    Third issue is that you can't make these changes for third party extensions, if there are any relying on the ncurses/ncursesw distinction.

    fyi, "all" ncursesw headers are:

    /usr/include/ncursesw/curses.h
    /usr/include/ncursesw/cursesapp.h
    /usr/include/ncursesw/cursesf.h
    /usr/include/ncursesw/cursesm.h
    /usr/include/ncursesw/cursesp.h
    /usr/include/ncursesw/cursesw.h
    /usr/include/ncursesw/cursslk.h
    /usr/include/ncursesw/eti.h
    /usr/include/ncursesw/etip.h
    /usr/include/ncursesw/form.h
    /usr/include/ncursesw/menu.h
    /usr/include/ncursesw/nc_tparm.h
    /usr/include/ncursesw/ncurses_dll.h
    /usr/include/ncursesw/panel.h
    /usr/include/ncursesw/term.h
    /usr/include/ncursesw/term_entry.h
    /usr/include/ncursesw/termcap.h
    /usr/include/ncursesw/tic.h
    /usr/include/ncursesw/unctrl.h

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Dec 9, 2016

    second issue is that you apparently don't do the changes for term.h

    term.h is included only if ncurses is missing and the system (SysV) curses is used, so I didn't change it. See below:

    #ifdef __sgi
#include <term.h>
#endif

#ifdef HAVE_NCURSES_H
#include <ncurses.h>
#else
#include <curses.h>
#ifdef HAVE_TERM_H
/* for tigetstr, which is not declared in SysV curses */
#include <term.h>
#endif
#endif

    Third issue is that you can't make these changes for third party extensions

    Did you mean 3rd party extensions that include py_curses.h may get broken with this patch? IMO they shouldn't include this as it's CPython's implementation detail and shouldn't be used outside Modules/_cursesmodule.c and Modules/_curses_panel.c

    however the ncursesw installation can be found in <prefix>/include instead

    This is what I want to solve at the first place - make _curses compatible with different configurations. In configure.ac, there's a line:

    AC_CHECK_HEADERS(curses.h ncurses.h ncursesw/ncurses.h ncurses/ncurses.h panel.h ncursesw/panel.h ncurses/panel.h)

    so you apparently can assume that this directory name is fixed *iff* it exists

    Did you mean there's no need to check panel.h? I'll try to update the patch.

    There's something missing from this patch: if ncurses is built with --enable-reentrant, the library and include paths get an additional 't'. For example, the library name becomes libncursestw.so and includedir becomes $prefix/include/ncursestw. Note that 't' comes before 'w' as ncurses's configure.in handles --enable-widec before --enable-reentrant. I skip such cases as --enable-reentrant is not compatible with Modules/_cursesmodule.c yet (bpo-25720). I'd like to postpone it until bpo-25720 is resolved as it does not make sense to detect incompatible header paths.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Dec 9, 2016

    The only change that is needed here is to not include /usr/include/ncursesw in setup.py when cross compiling to ensure that the headers of the build platform are not included. When cross compiling Python, it is the responsability of the packager to set the appropriate CPPFLAGS and LDFLAGS upon invoking configure, so that the curses headers are included and the curses libraries are linked at build time.
    The same is true for the other extension modules, readline, openssl, libffi, etc...

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Dec 10, 2016

    The only change that is needed here is to not include /usr/include/ncursesw in setup.py when cross compiling

    No. Lots of codes in _cursesmodule.c need to know whether it's ncursesw, ncurses, or SysV's curses. For example: (segments below are from unpatched codebase)

    #if !defined(__hpux) || defined(HAVE_NCURSES_H)
    /* On HP/UX 11, these are of type cchar_t, which is not an
       integral type. If this is a problem on more platforms, a
       configure test should be added to determine whether ACS_S1
       is of integral type. */
    SetDictInt("ACS_S1",            (ACS_S1));
    SetDictInt("ACS_S9",            (ACS_S9));
    SetDictInt("ACS_DIAMOND",       (ACS_DIAMOND));
    SetDictInt("ACS_CKBOARD",       (ACS_CKBOARD));
    SetDictInt("ACS_DEGREE",        (ACS_DEGREE));
    SetDictInt("ACS_PLMINUS",       (ACS_PLMINUS));
    SetDictInt("ACS_BULLET",        (ACS_BULLET));
    SetDictInt("ACS_LARROW",        (ACS_LARROW));
    SetDictInt("ACS_RARROW",        (ACS_RARROW));
    SetDictInt("ACS_DARROW",        (ACS_DARROW));
    SetDictInt("ACS_UARROW",        (ACS_UARROW));
    SetDictInt("ACS_BOARD",         (ACS_BOARD));
    SetDictInt("ACS_LANTERN",       (ACS_LANTERN));
    SetDictInt("ACS_BLOCK",         (ACS_BLOCK));
#endif

    And

    static int
PyCurses_ConvertToCchar_t(PyCursesWindowObject *win, PyObject *obj,
                          chtype *ch
#ifdef HAVE_NCURSESW
                          , wchar_t *wch
#endif
                          )

    So detecting ncurses's actual include path is necessary.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Dec 10, 2016

    This patch does not add /usr/include/ncursesw to the include directories search paths when cross compiling.

    Restoring the original title.

    @Chi Hsuan Yen
    Please open a new issue for the detection of the curses paths. The current issue is about the failure to cross compile _curses when /usr/include/ncursesw is added to these paths. See the original post and the original title.

    @xdegaye xdegaye mannequin removed the build The build process and cross-build label Dec 10, 2016
    @xdegaye xdegaye mannequin self-assigned this Dec 10, 2016
    @xdegaye xdegaye mannequin changed the title Detect curses headers correctly for cross-compiling Cross-build _curses failed if host ncurses headers and target ncurses headers have different layouts Dec 10, 2016
    @doko42
    Copy link
    Member

    doko42 commented Dec 10, 2016

    The only change that is needed here is to not include
    /usr/include/ncursesw in setup.py when cross compiling

    no, this is a very wrong simplification. Both gcc and clang offer a method to search for header files and libraries in a target specific location. Please use these options.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Dec 10, 2016

    Both gcc and clang offer a method to search for header files and libraries in a target specific location.

    AFAIK there is no target specific location when cross compiling. What are those cross compilation target specific locations according to you ?

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Dec 10, 2016

    no, this is a very wrong simplification. Both gcc and clang offer a method to search for header files and libraries in a target specific location. Please use these options.

    Please open a new issue for the detection of the curses paths. The current issue is about the failure to cross compile _curses when /usr/include/ncursesw is added to these paths. See the original post and the original title.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Dec 10, 2016

    @xdegaye: no-path-to-ncursesw.patch fixes a problem yet introducing a new one. With that patch auto-detection of ncurses include files are broken. Now users have to specify the path manually even for native builds.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Dec 11, 2016

    Now users have to specify the path manually even for native builds.

    This does not make sense, the patch does not change anything for the native builds.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Dec 11, 2016

    Sorry I didn't read your patch carefully and it's surprising for me that you didn't remove/modify this line in configure.ac:

    CPPFLAGS="$CPPFLAGS -I/usr/include/ncursesw"

    With this line left there, feature detections for _curses are broken as ./configure check against native (/usr/include) headers, whose results make little sense for cross-builds.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Dec 11, 2016

    Hum, I did miss that line in configure.ac, thanks for pointing that out.
    New patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 13, 2016

    New changeset 8c78d844d6f0 by Xavier de Gaye in branch '3.6':
    Issue bpo-28190: Cross compiling the _curses module does not use anymore
    https://hg.python.org/cpython/rev/8c78d844d6f0

    New changeset bdf92b4e02f2 by Xavier de Gaye in branch 'default':
    Issue bpo-28190: Merge 3.6.
    https://hg.python.org/cpython/rev/bdf92b4e02f2

    @xdegaye xdegaye mannequin removed their assignment Dec 13, 2016
    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Mar 26, 2019

    As ncurses-related modules in Python alreadu build fine for Android, I consider the issue resolved.

    @yan12125 yan12125 mannequin closed this as completed Mar 26, 2019
    @doko42
    Copy link
    Member

    doko42 commented Mar 26, 2019

    no, please don't assume that if it builds for one cross build variant, that it builds for all. re-opening.

    @doko42 doko42 reopened this Mar 26, 2019
    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Apr 8, 2019

    I created #12587 as a preparation pull request to fix the __sgi issue mentioned in msg282782.

    @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
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life build The build process and cross-build
    Projects
    Curses issues
    Status: Build
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @doko42