Uh oh!
There was an error while loading. Please reload this page.
bpo-1635741 port _curses_panel to multi-phase init (PEP 489) - #21986
Conversation
koubaa
commented
Aug 29, 2020
@vstinner@shihai1991 this is ready for review |
vstinner
left a comment
There was a problem hiding this comment.
I don't want to review this PR since it's too large. If you want me to review it, please cut it in half (create a second PR for half of changed modules.)
1e92854 to
8b134daComparekoubaa
commented
Aug 29, 2020
@vstinner I pulled some out into another PR. |
shihai1991
commented
Aug 30, 2020
IMHO, a single module change in a PR is more reasonable. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Can we move SiginfoType to module state?
There was a problem hiding this comment.
@shihai1991 I'm not familiar with PyStructSequence_InitType2. Are there any special considerations when converting a type like this to a heap type and adding it to module state?
There was a problem hiding this comment.
ops, I am not familiar with PyStructSequence_InitType2() too : (. This function looks like just do some copy operation.
koubaa
commented
Aug 30, 2020
I sympathize with this, I can certainly do this going forward. Please let me know and I can split this PR if required. |
shihai1991
commented
Aug 30, 2020
victor have give an suggestion, so we can follow victor's advice in this PR ;) |
vstinner
commented
Sep 1, 2020
Yeah, I still don't want to review a single PR converting three unrelated extension modules to multiphase init. |
f3d2905 to
95081a7Comparekoubaa
commented
Sep 1, 2020
No problem at all. I am splitting each of these in its own PR |
corona10
left a comment
There was a problem hiding this comment.
##[error]/home/runner/work/cpython/cpython/Modules/_curses_panel.c:629:7: error: request for member ‘PyCursesPanel_Type’ in something not a structure or union
st->PyCursesPanel_Type = PyType_FromSpec(&PyCursesPanel_Type_spec);
^~
##[error]/home/runner/work/cpython/cpython/Modules/_curses_panel.c:630:11: error: request for member ‘PyCursesPanel_Type’ in something not a structure or union
if (st->PyCursesPanel_Type == NULL)
^~
##[error]/home/runner/work/cpython/cpython/Modules/_curses_panel.c:633:31: error: request for member ‘PyCursesPanel_Type’ in something not a structure or union
if (PyModule_AddType(m, st->PyCursesPanel_Type) < 0)
^~
##[error]/home/runner/work/cpython/cpython/Modules/_curses_panel.c:642:7: error: request for member ‘PyCursesError’ in something not a structure or union
st->PyCursesError = PyErr_NewException("_curses_panel.error", NULL, NULL);
^~
##[error]/home/runner/work/cpython/cpython/Modules/_curses_panel.c:643:40: error: request for member ‘PyCursesError’ in something not a structure or union
PyDict_SetItemString(d, "error", st->PyCursesError);
Please check compile error :)
bedevere-bot
commented
Sep 2, 2020
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
koubaa
commented
Sep 2, 2020
@corona10 I don't have compile errors on windows, but I am now noticing that there is a PyState_FindModule which isn't compatible with multi-phase initialization. I'll fix that, and try a test build on linux too (odd that the CI passed if you hit a compile error) |
koubaa
commented
Sep 2, 2020
@corona10 I removed PyState_FindModule issue, please try again now |
vstinner
left a comment
There was a problem hiding this comment.
Please fix compiler warnings that you can see at https://github.com/python/cpython/pull/21986/files
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner
left a comment
There was a problem hiding this comment.
They are still compiler errors, like:
unknown type name ‘_curses_panel_state’; did you mean ‘_curses_panelstate’?
What is your OS? Did you try to build Python locally before pushing your change?
koubaa
commented
Sep 6, 2020
@vstinner I am on windows and I don't have any errors when I build locally. I think I need to depend on github to find them.. |
koubaa
commented
Sep 7, 2020
I think I fixed all the compiler warnings. The CI fails due to this:
I think this is due to adding cls:defining_class to the argument clinic (which converts the method type to METH_METHOD|METH_FASTCALL|METH_KEYWORDS. I'm not sure exactly what I need to do to fix it. |
koubaa
commented
Sep 7, 2020
Fixed |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner
commented
Sep 7, 2020
Oh ok. I'm fine with you using the CI to check for errors. The module is not built on Windows, since it's specific to Unix. curses is not available on Windows. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner
left a comment
There was a problem hiding this comment.
LGTM, thanks for the multiple updates, the code is now much better!
vstinner
commented
Sep 7, 2020
test_asyncio failed on Windows (x86), but this test is not reliable. I ignore the failure. |
|
vstinner
commented
Sep 7, 2020
I created https://bugs.python.org/issue41739 "test_logging: threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 2)". |
* origin/master: (1373 commits) bpo-1635741: Port mashal module to multi-phase init (python#22149) bpo-1635741: Port _string module to multi-phase init (pythonGH-22148) bpo-1635741: Convert _sha256 types to heap types (pythonGH-22134) bpo-1635741: Port the termios to multi-phase init (PEP 489) (pythonGH-22139) bpo-41732: add iterator to memoryview (pythonGH-22119) bpo-40744: Drop support for SQLite pre 3.7.3 (pythonGH-20909) bpo-41316: Make tarfile follow specs for FNAME (pythonGH-21511) bpo-41720: Add "return NotImplemented" in turtle.Vec2D.__rmul__(). (pythonGH-22092) bpo-1635741 port _curses_panel to multi-phase init (PEP 489) (pythonGH-21986) bpo-1635741: Port _overlapped module to multi-phase init (pythonGH-22051) bpo-1635741: Port _opcode module to multi-phase init (PEP 489) (pythonGH-22050) bpo-1635741 port zlib module to multi-phase init (pythonGH-21995) [doc] Add link to Generic in typing (pythonGH-22125) bpo-41513: Expand comments and add references for a better understanding (pythonGH-22123) bpo-1635741: Port _sha1, _sha512, _md5 to multiphase init (pythonGH-21818) closes bpo-41723: Fix an error in the py_compile documentation. (pythonGH-22110) [doc] Fix padding in some typing definitions (pythonGH-22114) Fix documented Python version for venv --upgrade-deps (pythonGH-22113) bpo-40318: Migrate to SQLite3 trace v2 API (pythonGH-19581) bpo-41687: Fix sendfile implementation to work with Solaris (python#22040) ...
This change caused a regression: bpo-42694. |
https://bugs.python.org/issue1635741