bpo-1635741 port _curses_panel to multi-phase init (PEP 489) #21986
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@vstinner @shihai1991 this is ready for review |
vstinner
reviewed
Aug 29, 2020
koubaa
force-pushed
the
bpo-1635741-simple
branch
2 times, most recently
from
1e92854 to
8b134da
Compare
@vstinner I pulled some out into another PR. |
IMHO, a single module change in a PR is more reasonable. |
shihai1991
reviewed
Aug 30, 2020
Modules/signalmodule.c
Outdated
| #if defined(HAVE_SIGWAITINFO) || defined(HAVE_SIGTIMEDWAIT) | ||
| if (!initialized) { | ||
| if (PyStructSequence_InitType2(&SiginfoType, &struct_siginfo_desc) < 0) | ||
| return NULL; | ||
| return -1; | ||
| } | ||
| Py_INCREF((PyObject*) &SiginfoType); |
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.
I sympathize with this, I can certainly do this going forward. Please let me know and I can split this PR if required. |
victor have give an suggestion, so we can follow victor's advice in this PR ;) |
Yeah, I still don't want to review a single PR converting three unrelated extension modules to multiphase init. |
koubaa
force-pushed
the
bpo-1635741-simple
branch
from
f3d2905 to
95081a7
Compare
No problem at all. I am splitting each of these in its own PR |
corona10
requested changes
Sep 2, 2020
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 :)
|
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 |
|
@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) |
|
@corona10 I removed PyState_FindModule issue, please try again now |
koubaa
changed the title
vstinner
reviewed
Sep 2, 2020
There was a problem hiding this comment.
Please fix compiler warnings that you can see at https://github.com/python/cpython/pull/21986/files
vstinner
reviewed
Sep 3, 2020
vstinner
reviewed
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.. |
|
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. |
|
Fixed |
vstinner
reviewed
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. |
vstinner
reviewed
Sep 7, 2020
vstinner
reviewed
Sep 7, 2020
vstinner
approved these changes
Sep 7, 2020
|
test_asyncio failed on Windows (x86), but this test is not reliable. I ignore the failure. |
|
|
I created https://bugs.python.org/issue41739 "test_logging: threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 2)". |
shihai1991
added a commit
to shihai1991/cpython
that referenced
this pull request
Sep 10, 2020
* 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) ...
xzy3
pushed a commit
to xzy3/cpython
that referenced
this pull request
Oct 18, 2020
|
This change caused a regression: bpo-42694. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://bugs.python.org/issue1635741