GeezerVison is showing github.com — gh-93649: Split vectorcall testing from _testcapimodule.c by encukou · Pull Request #94549 · python/cpython
Conversation
encukou
commented
Jul 4, 2022
•
edited by miss-islington
Loading
Uh oh!
There was an error while loading. Please reload this page.
The _testcapimodule.c file is getting too large to work with effectively.
This PR lays out a general structure of how tests can be split up, with more splitting to come later if the structure is OK.
Vectorcall tests aren't the biggest issue -- it's just an area I want to work on next, so I'm starting here.
An issue specific to vectorcall tests is that it wasn't clear that e.g. MethodDescriptor2 is related to testing vectorcall: the /* Test PEP 590 */ section had an ambiguous end. Separate file should make things like this much clearer.
OTOH, for some pieces it might not be clear where they should be -- I left meth_fastcall with tests of the other calling conventions. IMO, even with the ambiguity it's still worth it to split the huge file up.
I'm not sure about the buildsystem changes, hopefully CI will tell me what's wrong.
@vstinner, @markshannon: Do you think this is a good idea?
Automerge-Triggered-By: GH:encukou
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The _testcapimodule.c file is getting too large to work with effectively.
Vectorcall tests aren't the biggest issue -- it's just an area I want to work
on next, so I started there.
It does make it clear that MethodDescriptor2 is related to testing vectorcall,
which wasn't clear before (the /* Test PEP 590 */ section had an ambiguous end).
This PR lays out a general structure of how tests can be split up,
with more splitting to come later if the structure is OK.
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.
This is IMO a nice improvement. Thanks!
You might want to add Modules/_testcapi/testdcapimodule_parts.h to MODULE__TESTCAPI_DEPS in Makefile.pre.in.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
I suggest shorter file names:
Modules/_testcapi/test_vectorcall.c -> Modules/_testcapi/vectorcall.cModules/_testcapi/testcapimodule_parts.h -> Modules/_testcapi/parts.h
Would it be possible to move Modules/_testcapimodule.c to Modules/_testcapi/module.c?
|
erlend-aasland reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
@vstinner, @markshannon: Do you think this is a good idea?
Yes, I like the idea of splitting the long _testcapimodule.c into multiple shorter C files.
|
ambv reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Would it be possible to move Modules/_testcapimodule.c to Modules/_testcapi/module.c?
In a different PR, maybe. There are a few more files to go on that ride, and I'd like to limit this PR to one change.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
While _testcapimodule.c is built as a single C extension, I'm not convinced by the purpose of _testcapi/parts.h which only declares a single function. But it's more explicit for you, go for it.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
_testcapi/parts.h should list the init functions from all the parts. There'll be more :)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Status check is done, and it's a success ✅ .
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
The PR broke WASM build builts:
https://buildbot.python.org/all/#/builders/1050/builds/86
error: unable to open output file 'Modules/_testcapi/vectorcall.o': 'No such file or directory'
1 error generated.
emcc: error: 'ccache /opt/emsdk/upstream/bin/clang -target wasm32-unknown-emscripten -D__EMSCRIPTEN_SHARED_MEMORY__=1 -DEMSCRIPTEN -D__EMSCRIPTEN_major__=3 -D__EMSCRIPTEN_minor__=1 -D__EMSCRIPTEN_tiny__=15 -fignore-exceptions -mllvm -combiner-global-alias-analysis=false -mllvm -enable-emscripten-sjlj -mllvm -disable-lsr -Werror=implicit-function-declaration -Xclang -iwithsysroot/include/SDL --sysroot=/opt/buildbot/.emscripten_cache/sysroot -Xclang -iwithsysroot/include/compat -Wsign-compare -Wunreachable-code -DNDEBUG -g3 -fwrapv -O3 -Wall -pthread -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -fvisibility=hidden -I../../Include/internal -IObjects -IInclude -IPython -I. -I../../Include -DPy_BUILD_CORE_BUILTIN -c -matomics -mbulk-memory ../../Modules/_testcapi/vectorcall.c -o Modules/_testcapi/vectorcall.o' failed (returned 1)
make: *** [Makefile:2800: Modules/_testcapi/vectorcall.o] Error 1
make: *** Waiting for unfinished jobs....
emmake: error: 'make -j2 all' failed (returned 2)
gh-94695 should fix them
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
vstinner
vstinner approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|
vstinner
left a comment