Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-1635741 port _curses_panel to multi-phase init (PEP 489)#21986
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
koubaa commented Aug 28, 2020 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
koubaa commented Aug 29, 2020
@vstinner@shihai1991 this is ready for review |
vstinner left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Modules/signalmodule.c Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move SiginfoType to module state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
##[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
bedevere-bot commented Sep 7, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
|
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) ...
vstinner commented Dec 26, 2020 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
This change caused a regression: bpo-42694. |
https://bugs.python.org/issue1635741