classification
Title: Cross-build _curses failed if host ncurses headers and target ncurses headers have different layouts
Type: compile error Stage: patch review
Components: Cross-Build Versions: Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Alex.Willmer, Chi Hsuan Yen, doko, masamoto, python-dev, serhiy.storchaka, xdegaye
Priority: normal Keywords: patch

Created on 2016-09-17 17:20 by Chi Hsuan Yen, last changed 2016-12-13 15:07 by xdegaye.

Files
File name Uploaded Description Edit
ncurses.patch Chi Hsuan Yen, 2016-09-17 17:20 review
cygwin-test_curses.log masamoto, 2016-10-04 16:47 ran without skip condition
ncurses-headers.patch Chi Hsuan Yen, 2016-12-07 18:02 Patch version 2, autoreconf necessary
no-path-to-ncursesw.patch xdegaye, 2016-12-10 09:08 review
no-path-to-ncursesw_2.patch xdegaye, 2016-12-11 14:27 review
Messages (20)
msg276806 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-09-17 17:20
In changeset 919259054621 (from issue12567) and 707761d59a4a (from issue15268), /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
msg276807 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-09-17 17:29
Hmmm, I don't know why Rietveld failed to recognize changes in configure. Maybe it's because I've modified the patch file manually?
msg277430 - (view) Author: Matthias Klose (doko) * (Python committer) Date: 2016-09-26 15:59
looks good to me, thanks for working on this.
msg278051 - (view) Author: Masayuki Yamamoto (masamoto) * Date: 2016-10-04 15:46
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.
msg278052 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-10-04 15:53
Hmm it's surprising for me that an irrelevant patch fixes issues on Cygwin. Does test_curses pass?
msg278060 - (view) Author: Masayuki Yamamoto (masamoto) * Date: 2016-10-04 16:47
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.
msg282647 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-12-07 18:02
A clean patch without changes in ./configure. autoreconf necessary
msg282698 - (view) Author: Matthias Klose (doko) * (Python committer) Date: 2016-12-08 08:39
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
msg282782 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-12-09 13:44
> 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 (issue25720). I'd like to postpone it until issue25720 is resolved as it does not make sense to detect incompatible header paths.
msg282806 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-12-09 21:10
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...
msg282822 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-12-10 03:41
> 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.
msg282838 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-12-10 09:08
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.
msg282839 - (view) Author: Matthias Klose (doko) * (Python committer) Date: 2016-12-10 09:10
> 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.
msg282840 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-12-10 09:26
> 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 ?
msg282844 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-12-10 09:50
> 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.
msg282848 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-12-10 12:20
@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.
msg282912 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-12-11 09:35
> 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.
msg282915 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-12-11 11:24
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.
msg282921 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-12-11 14:27
Hum, I did miss that line in configure.ac, thanks for pointing that out.
New patch.
msg283111 - (view) Author: Roundup Robot (python-dev) Date: 2016-12-13 15:06
New changeset 8c78d844d6f0 by Xavier de Gaye in branch '3.6':
Issue #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 #28190: Merge 3.6.
https://hg.python.org/cpython/rev/bdf92b4e02f2
History
Date User Action Args
2016-12-13 15:11:44xdegayeunlinkissue26865 dependencies
2016-12-13 15:07:58xdegayesetassignee: xdegaye ->
2016-12-13 15:06:29python-devsetnosy: + python-dev
messages: + msg283111
2016-12-11 14:27:10xdegayesetfiles: + no-path-to-ncursesw_2.patch

messages: + msg282921
2016-12-11 11:24:49Chi Hsuan Yensetmessages: + msg282915
2016-12-11 09:35:39xdegayesetmessages: + msg282912
2016-12-10 21:23:05hayposetnosy: - haypo
2016-12-10 12:20:33Chi Hsuan Yensetmessages: + msg282848
2016-12-10 09:50:48xdegayesetmessages: + msg282844
2016-12-10 09:26:57xdegayesetmessages: + msg282840
2016-12-10 09:10:43dokosetmessages: + msg282839
2016-12-10 09:08:33xdegayesetfiles: + no-path-to-ncursesw.patch

assignee: xdegaye
components: - Build
title: Detect curses headers correctly for cross-compiling -> Cross-build _curses failed if host ncurses headers and target ncurses headers have different layouts
versions: + Python 3.6
messages: + msg282838
stage: patch review
2016-12-10 03:41:23Chi Hsuan Yensetmessages: + msg282822
2016-12-09 21:10:24xdegayesetmessages: + msg282806
2016-12-09 13:44:55Chi Hsuan Yensetmessages: + msg282782
2016-12-08 08:39:30dokosetmessages: + msg282698
2016-12-07 20:30:50xdegayelinkissue26865 dependencies
2016-12-07 18:02:20Chi Hsuan Yensetfiles: + ncurses-headers.patch

messages: + msg282647
2016-10-04 16:47:48masamotosetfiles: + cygwin-test_curses.log

messages: + msg278060
2016-10-04 15:53:04Chi Hsuan Yensetmessages: + msg278052
2016-10-04 15:46:04masamotosetnosy: + masamoto
messages: + msg278051
2016-09-30 15:30:32xdegayesetnosy: + xdegaye
2016-09-26 15:59:25dokosetmessages: + msg277430
2016-09-25 10:42:00serhiy.storchakasetnosy: + serhiy.storchaka
2016-09-25 09:35:49Chi Hsuan Yensettitle: Cross-build _curses failed if host ncurses headers and target ncurses headers have different layouts -> Detect curses headers correctly for cross-compiling
2016-09-17 17:29:00Chi Hsuan Yensetmessages: + msg276807
2016-09-17 17:20:08Chi Hsuan Yencreate