Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-93649: Split vectorcall testing from _testcapimodule.c#94549

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
miss-islington merged 5 commits intopython:mainfromencukou:testcapi-split
Jul 8, 2022

Conversation

@encukou
Copy link
Member

@encukouencukou commentedJul 4, 2022•
edited by miss-islington
Loading

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 leftmeth_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

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 workon 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.
Copy link
Contributor

@erlend-aaslanderlend-aasland left a 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 addModules/_testcapi/testdcapimodule_parts.h toMODULE__TESTCAPI_DEPS inMakefile.pre.in.

@vstinner
Copy link
Member

I suggest shorter file names:

  • Modules/_testcapi/test_vectorcall.c ->Modules/_testcapi/vectorcall.c
  • Modules/_testcapi/testcapimodule_parts.h ->Modules/_testcapi/parts.h

Would it be possible to moveModules/_testcapimodule.c toModules/_testcapi/module.c?

erlend-aasland reacted with thumbs up emoji

@vstinner
Copy link
Member

@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

@encukou
Copy link
MemberAuthor

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.

Copy link
Member

@vstinnervstinner left a 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.

Co-authored-by: Victor Stinner <vstinner@python.org>
@encukou
Copy link
MemberAuthor

Thanks!

@encukou
Copy link
MemberAuthor

_testcapi/parts.h should list the init functions from all the parts. There'll be more :)

@miss-islington
Copy link
Contributor

Status check is done, and it's a success ✅ .

@miss-islingtonmiss-islington merged commitbe862b4 intopython:mainJul 8, 2022
@encukouencukou deleted the testcapi-split branchJuly 8, 2022 15:56
@tiran
Copy link
Member

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 1make: *** Waiting for unfinished jobs....emmake: error: 'make -j2 all' failed (returned 2)

gh-94695 should fix them

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@erlend-aaslanderlend-aaslanderlend-aasland left review comments

@vstinnervstinnervstinner approved these changes

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@encukou@vstinner@miss-islington@tiran@erlend-aasland@bedevere-bot
[8]ページ先頭
©2009-2025 Movatter.jp