Skip to content

gh-87175: Port curses capi pointer array to a struct. #24304

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

shihai1991
Copy link
Member

@shihai1991 shihai1991 commented Jan 23, 2021

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, this change is backward incompatible. I don't know how to proceed. Is this API used outside Python, in 3rd party projects?

@encukou @serhiy-storchaka @methane: Is this change reasonable?

Note: I asked @shihai1991 to write this change ;-)

@methane
Copy link
Member

methane commented Jan 26, 2021

@vstinner
Copy link
Member

https://foss.heptapod.net/pypy/cffi/-/blob/cea8f1260587a4cf6ad230a00148cabea0f61572/demo/_curses.py#L972

It's a demo and it's commented, so it seems ok.

Moreover, the demo uses PyCursesInitialised; which is not affected by this change.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document the incompatible change with a NEWS entry (blurb) and document in C API > Porting to Python 3.10 of:
https://docs.python.org/dev/whatsnew/3.10.html#id2

@vstinner
Copy link
Member

The only user I found is this.
https://foss.heptapod.net/pypy/cffi/-/blob/cea8f1260587a4cf6ad230a00148cabea0f61572/demo/_curses.py#L972

@methane: Did you check for PyCurses_API and PyCurses_API_pointers usage?

@shihai1991
Copy link
Member Author

The only user I found is this.
https://foss.heptapod.net/pypy/cffi/-/blob/cea8f1260587a4cf6ad230a00148cabea0f61572/demo/_curses.py#L972

@methane: Did you check for PyCurses_API and PyCurses_API_pointers usage?

I use https://searchcode.com/ to check who will use it. But I am not find the useful info in here.

@encukou
Copy link
Member

encukou commented Jan 26, 2021

Hum, this change is backward incompatible. I don't know how to proceed.

Well, PEP 387 is quite clear on this one, I think. PyCurses_API is obviously an API, so it can't be changed without a deprecation warning period.
IMO, this should be done by adding a new capsule and deprecating the old static one.

@shihai1991
Copy link
Member Author

Hum, this change is backward incompatible. I don't know how to proceed.

Well, PEP 387 is quite clear on this one, I think. PyCurses_API is obviously an API, so it can't be changed without a deprecation warning period.
IMO, this should be done by adding a new capsule and deprecating the old static one.

Hm, it's more friendly to users. What's your suggestion? victor :) @vstinner

@serhiy-storchaka
Copy link
Member

Why do you want to do this?

If we will do that, I think it is safe to replace an array with a structure as long as offsets of structure fields matches offsets of array items. I believe it is true on all supported platforms, because all fields are pointers. PyCurses_API_pointers should be kept for compatibility (but deprecated).

@vstinner
Copy link
Member

Why do you want to do this?

I proposed this change (and @shihai1991 implemented it) since I don't see a raw array of pointers as an "API".

If we will do that, I think it is safe to replace an array with a structure as long as offsets of structure fields matches offsets of array items. I believe it is true on all supported platforms, because all fields are pointers. PyCurses_API_pointers should be kept for compatibility (but deprecated).

Even if PyCurses_API_pointers (4) is part of the public Include/py_curses.h header file (and it doesn't exclude anything from the limited C API), it seems like the API is mostly used through these 4 macros:

#define PyCursesWindow_Type (*(PyTypeObject *) PyCurses_API[0])
#define PyCursesSetupTermCalled  {if (! ((int (*)(void))PyCurses_API[1]) () ) return NULL;}
#define PyCursesInitialised      {if (! ((int (*)(void))PyCurses_API[2]) () ) return NULL;}
#define PyCursesInitialisedColor {if (! ((int (*)(void))PyCurses_API[3]) () ) return NULL;}

Not directly through static void **PyCurses_API;.

Maybe this API was only exposed for the _curses_panel module and is not intended to be used outside Python.

@vstinner
Copy link
Member

Note: Include/py_curses.h is not included by Python.h.

@vstinner vstinner closed this Jan 27, 2021
@vstinner vstinner reopened this Jan 27, 2021
@vstinner
Copy link
Member

(oops, I closed the PR by mistake.)

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 27, 2021
@vstinner
Copy link
Member

vstinner commented May 9, 2023

The capsule object and the related C API is consumed by _curses_panel.c. It's a way to expose an API in Modules/_cursesmodule.c and use it in Modules/_curses_panel.c. The _curses_panel extension is separated since it's linked to the panel library (ex: libpanelw.so.6 on Linux), whereas the _curses extension is linked to curses (ex: libncursesw.so.6 on Linux).

I'm not sure what it was good idea at the beginning to make such API public. But well, when it was added 23 years ago (commit 0c7a0bd), we cared less about what should be public or private. The C API was mostly consumed by CPython itself.

I suggest to make the capsule object private and move the related API to the internal C API.

In Python 3.10, I directly removed the ucnhash_CAPI attribute of the unicodedata extension: I made it private (unicodedata._ucnhash_CAPI) and moved its C API to the internal C API. This work was related to convert the unicodedata to the multi-phase initialization and convert static types to heap types: see #86323 I would expect that there are more unicodedata users than _curses users, for their C API.

@vstinner
Copy link
Member

vstinner commented May 9, 2023

These APIs are not tested, not documented, and only used by _cursesmodule.c and _curses_panel.c (in CPython):

  • PyCursesWindow_Type
  • PyCursesSetupTermCalled
  • PyCursesInitialised
  • PyCursesInitialisedColor

CPython:

$ git grep -l -E '(PyCursesWindow_Type|PyCursesSetupTermCalled|PyCursesInitialised|PyCursesInitialisedColor)'
Include/py_curses.h
Modules/_curses_panel.c
Modules/_cursesmodule.c
Modules/clinic/_curses_panel.c.h
Modules/clinic/_cursesmodule.c.h
Tools/c-analyzer/cpython/globals-to-fix.tsv

They are not used by any of the PyPI top 5000 projects. cffi has a reference but only in a comment:

$ ./search_pypi_top.py PYPI-2023-04-13/ '(PyCursesWindow_Type|PyCursesSetupTermCalled|PyCursesInitialised|PyCursesInitialisedColor)' -o curses -q
PYPI-2023-04-13/cffi-1.15.1.tar.gz: cffi-1.15.1/demo/_curses.py: #     PyCursesInitialised;
PYPI-2023-04-13/cffi-1.15.1.tar.gz: cffi-1.15.1/demo/_curses.py: #     PyCursesInitialised;

Found 2 matching lines in 1 projects

@shihai1991
Copy link
Member Author

Thanks for you comment. I am amazing that Victor checked the details again after two years :)
This not exposed by python.h and the outside user can use the macros to get the field by using the macros in the py_curses.h if they need it. Or removed it into internal CAPI is OK if we haven't the outside user(I am not sure).

I'm not sure what it was good idea at the beginning to make such API public. But well, when it was added 23 years ago (commit 0c7a0bd), we cared less about
what should be public or private. The C API was mostly consumed by CPython itself.

Maybe we can bother this PR's creator to make sure the original purpose. @akuchling

I will rebase it tomorrow.

@arhadthedev arhadthedev changed the title bpo-43009: Port curses capi pointer array to a struct. gh-87175: Port curses capi pointer array to a struct. May 12, 2023
@arhadthedev
Copy link
Member

I will rebase it tomorrow.

Could you target Doc/whatsnew/3.13.rst, please? 3.12 no longer accepts new features because of final preparations for 3.12b1.

@vstinner
Copy link
Member

The least risky approach is to:

  • Add a new internal C API, maybe using a struct rather than an array.
  • Deprecate the old C API for 2 Python releases.

@shihai1991
Copy link
Member Author

I will rebase it tomorrow.

Could you target Doc/whatsnew/3.13.rst, please? 3.12 no longer accepts new features because of final preparations for 3.12b1.

So 3.12 is prepare to release? I missed a lot of things recently.

@arhadthedev
Copy link
Member

So 3.12 is prepare to release? I missed a lot of things recently.

Moreover, its release date was moved (python/peps#3139) from this Monday to May 22.

@erlend-aasland
Copy link
Contributor

So 3.12 is prepare to release? I missed a lot of things recently.

No, only the beta (feature freeze). The release is not until this fall.

Anyway, I'm not sure how PEP-387 plays with stuff that's not exposed by Python.h.

Conflicts:
	Doc/whatsnew/3.10.rst
	Include/py_curses.h
@shihai1991 shihai1991 requested a review from a team as a code owner May 16, 2023 19:03
@shihai1991
Copy link
Member Author

So 3.12 is prepare to release? I missed a lot of things recently.

Moreover, its release date was moved (python/peps#3139) from this Monday to May 22.

I will rebase it tomorrow.

Could you target Doc/whatsnew/3.13.rst, please? 3.12 no longer accepts new features because of final preparations for 3.12b1.

Hi, I updated this PR(add the internal APIs) and the old APIs in py_curses.h hasn't been removed. So I think I add a note in 3.12 will be ok?

@arhadthedev
Copy link
Member

arhadthedev commented May 17, 2023

So I think I add a note in 3.12 will be ok?

Probably yes, however, it's up to the core developers to decide.

Tests / Check if generated files are up to date (pull_request) Failing after 7m

checking analysis results...
Include/internal/pycore_curses.h   	-                                  	PyCurses_C_API                          	> not supported (mutable)           	static PyCurses_CAPI*
-------------------------
total failures: 1
done checking
Categorized by storage:


static-module-global
  Include/internal/pycore_curses.h   	-                                  	PyCurses_C_API
subtotal: 1

@erlend-aasland Should we store the new PyCurses_CAPI in the module state or the interpreter state?

@shihai1991
Copy link
Member Author

@erlend-aasland Should we store the new PyCurses_CAPI in the module state or the interpreter state?

It's not common variable in interpreter(just the curses_panel extension module use this Capsule API), so saved in interpreter state is OK.

@erlend-aasland
Copy link
Contributor

It's not common variable in interpreter(just the curses_panel extension module use this Capsule API), so saved in interpreter state is OK.

I assume you meant module state, no?

@shihai1991
Copy link
Member Author

It's not common variable in interpreter(just the curses_panel extension module use this Capsule API), so saved in interpreter state is OK.

I assume you meant module state, no?

Oh, thanks for correcting my wrong writing.

In addition, Use the ignored.csv to ignore the warning maybe OK in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.