Fix curses module compilation with ncurses6

[donmez]

BPO	25720
Nosy	@Yhg1s, @mdickinson, @serhiy-storchaka, @ma8ma, @yan12125
PRs	bpo-25720: curses: Fix check for whether WINDOW is a pad #1689 bpo-25720: Fix the method for checking pad state of curses WINDOW #4164 [3.6] bpo-25720: Fix the method for checking pad state of curses WINDOW (GH-4164) #4212 [2.7] bpo-25720: Fix the method for checking pad state of curses WINDOW (GH-4164) #4213
Dependencies	bpo-31891: Make curses compiling on NetBSD 7.1 and tests passing
Files	curses-ncurses6.patch curses-is_pad.patch join-test-issue28190-issue25720.patch: for testing

^{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 = <Date 2017-11-01.12:38:50.867>
created_at = <Date 2015-11-24.11:39:07.962>
labels = ['extension-modules', 'build', '3.7']
title = 'Fix curses module compilation with ncurses6'
updated_at = <Date 2017-11-01.12:38:50.865>
user = 'https://bugs.python.org/donmez'

bugs.python.org fields:

activity = <Date 2017-11-01.12:38:50.865>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2017-11-01.12:38:50.867>
closer = 'serhiy.storchaka'
components = ['Extension Modules']
creation = <Date 2015-11-24.11:39:07.962>
creator = 'donmez'
dependencies = ['31891']
files = ['41148', '45002', '45037']
hgrepos = []
issue_num = 25720
keywords = ['patch']
message_count = 27.0
messages = ['255261', '258136', '258137', '258142', '261920', '261923', '278050', '278054', '278128', '278140', '278241', '278242', '278245', '278248', '278303', '278353', '278360', '278363', '287757', '294071', '296750', '305178', '305181', '305370', '305372', '305373', '305374']
nosy_count = 6.0
nosy_names = ['twouters', 'mark.dickinson', 'donmez', 'serhiy.storchaka', 'masamoto', 'yan12125']
pr_nums = ['1689', '4164', '4212', '4213']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue25720'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7']

[donmez]

ncurses6 turned on NCURSES_OPAQUE, so now you have to use some helper functions instead of accessing the structs directly. This _should_ be compatible with ncurses5 though I didn't test it.

Original patch is from openSUSE.

[donmez]

Any patch review/comment ?

[serhiy-storchaka]

configure.ac directly uses w->_flags in a check. It looks that this check always fails with ncurses6.

[donmez]

Thats not an issue for ncurses because Include/py_curses.h does:

  #ifdef HAVE_NCURSES_H
  /* configure was checking <curses.h>, but we will
     use <ncurses.h>, which has all these features. */
  #ifndef WINDOW_HAS_FLAGS
  #define WINDOW_HAS_FLAGS 1
  #endif
  #ifndef MVWDELCH_IS_EXPRESSION
  #define MVWDELCH_IS_EXPRESSION 1
  #endif
  #endif

So it overrides WINDOW_HAS_FLAGS for ncurses case.

[donmez]

ping?

[serhiy-storchaka]

I suspect that the patch can break build with non-ncurses implementations or with old ncurses (when is_pad() was added?). Needed more direct feature check.

[yan12125]

is_pad is added in ncurses 5.7-20090906 [1]. At least Mac OS X still ships ancient ncurses 5.7-20081102 [2], so an check in configure.ac is necessary. I'm trying it out.

[1] http://invisible-island.net/ncurses/NEWS.html#t20090906
[2] http://opensource.apple.com//source/ncurses/ncurses-46/ncurses/NEWS

[serhiy-storchaka]

Mark, do you have relation to this?

[ma8ma]

I tried to build curses module on Cygwin (Vista x86) using bpo-25720 patch. And it has been succeeded.
When test_curses ran without skip condition, it was same result as msg278060 (bpo-28190).

I found out build success reasons for cases of applying patch:
bpo-25720 -- implementation of WINDOW is opaque (but WINDOW_HAS_FLAGS is defined at Include/py_curses.h:61 ). However, curses module build went well to cover the _flags field from source code by is_pad.
bpo-14598 -- implementation of WINDOW is not opaque (WINDOWS_HAS_FLAGS is defined at configure script). Therefore, curses module build went well because WINDOW has the _flags field.
bpo-28190 -- implementation of WINDOW is opaque (WINDOW_HAS_FLAGS isn't defined: py_curses.h has been cleaned by patch). Hence, curses module build went well to remove the _flags field from source code at preprocessing.

All case tests on Cygwin have failed at unget_wch.

[mdickinson]

Mark, do you have relation to this?

Sorry, no. Whatever ncurses knowledge I may once have had has long since vanished.

[ma8ma]

I updated the patch that add configuration check for is_pad. the is_pad is wrapped into py_is_pad at Modules/_cursesmodule.c:932 by either of three ways.

Case one -- is_pad is found: Define the macro that is simple wrapping.
Case two -- is_pad is not found, however WINDOW has _flags field: Define the macro using _flags field.
Case three -- is_pad is not found, and WINDOW doesn't have _flags field: Define the macro that is always preprocessed to FALSE.

I succeeded to build curses module on Cygwin (Vista x86) using this patch.
This patch doesn't include that undo fixes for specific platforms.

[donmez]

@masamoto thank you!

[serhiy-storchaka]

Added a comment on Rietveld.

[yan12125]

Thanks masamoto! There's another minor issue about this patch: if there's no curses.h (ncurses built with --without-curses-h), the detection always fails.

[ma8ma]

Added comment in review.

Yen, I tried to build without curses.h file (overwrite to ncurses.h). It was failed on checking header. Hence, I confirmed curses headers on Cygwin and Ubuntu.

I found out that curses.h includes the directive "#include <unctrl.h>". And unctrl.h has "#include <curses.h>". Therefore, I think that some platforms require curses.h.
Would you confirm your platform curses library?

[yan12125]

headers.sh in ncurses changes "#include <curses.h>" to the actual path. For example here's a line in my unctrl.h:

#include <ncursesw/ncurses.h>

[ma8ma]

Thank you for confirming, Yen :)
In this case, It seems necessary that resolves headers. I think missing headers issue maybe solve by bpo-28190. I wrote a join test patch for bpo-28190 and bpo-25720.
Would you be able to resolve headers using this?

[yan12125]

Thanks, building is fine here.

By the way, testing is broken due to other bugs (/tmp not available on Android). It's unrelated and I'll open a new issue for that.

[donmez]

What's the status on this? Can you please create a pull request on Github so we can continue there?

[ma8ma]

Hi, I finished various things and tackle the issue again, I opened PR 1689 at last.

Changes from previous patch:

• If ncurses doesn't have both is_pad function and _flags field of WINDOW, NCURSES_OPAQUE is defined as zero to make WINDOW to non-opaque type before including ncurses.h.
• The conditional compile on function definition are kept, it replaces WINDOW_HAS_FLAGS with py_is_pad macro itself.
• Unindent the blocks that places after section of the conditional compile on function definition.

[ma8ma]

Ping. I updated PR a bit: macOS is joined to new compile condition and remove platform-specific condition.

[ma8ma]

I opened PR 4164 to improve the is_pad configure check and previous PR was closed.

[serhiy-storchaka]

I'll try to test this on NetBSD after fixing curses on NetBSD. It uses a different implementation of curses which don't support is_pad.

[serhiy-storchaka]

New changeset 8bc7d63 by Serhiy Storchaka (Masayuki Yamamoto) in branch 'master':
bpo-25720: Fix the method for checking pad state of curses WINDOW (bpo-4164)
8bc7d63

[serhiy-storchaka]

New changeset ff6ae4d by Serhiy Storchaka (Miss Islington (bot)) in branch '3.6':
bpo-25720: Fix the method for checking pad state of curses WINDOW (GH-4164) (bpo-4212)
ff6ae4d

[serhiy-storchaka]

New changeset 6ba0b58 by Serhiy Storchaka (Miss Islington (bot)) in branch '2.7':
bpo-25720: Fix the method for checking pad state of curses WINDOW (GH-4164) (bpo-4213)
6ba0b58

[serhiy-storchaka]

Thank you for your contribution Masayuki!