GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-107603: AC only includes pycore_gc.h if needed by vstinner · Pull Request #108715 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Aug 31, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Argument Clinic now only includes pycore_gc.h if PyGC_Head is needed, and only includes pycore_runtime.h if _Py_ID() is needed.

Add Py_ARGUMENT_CLINIC_AVOID_INTERNAL_CAPI macro to opt-out for the internal C API in Argument Clinic.

The following extensions avoids the internal C API by defining the macro Py_ARGUMENT_CLINIC_AVOID_INTERNAL_CAPI:

  • Modules/_csv.c
  • Modules/_multiprocessing/posixshmem.c
  • Modules/_testcapi/exceptions.c
  • Modules/grpmodule.c
  • Modules/syslogmodule.c
  • Issue: Argument Clinic includes internal headers in generated output unconditionally #107603

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner
vstinner requested review from a team, 1st1, AlexWaygood, Fidget-Spinner, JelleZijlstra, abalkin, asvetlov, berkerpeksag, brettcannon, corona10, ericsnowcurrently, erlend-aasland, ethanfurman, gvanrossum, kumaraditya303, ncoghlan, pganssle, rhettinger, tiran, warsaw and willingc as code owners August 31, 2023 11:52
@bedevere-bot bedevere-bot mentioned this pull request Aug 31, 2023
Argument Clinic includes internal headers in generated output unconditionally #107603
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Aug 31, 2023
@vstinner

vstinner commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

The following extensions avoids the internal C API by defining the macro Py_ARGUMENT_CLINIC_AVOID_INTERNAL_CAPI:
Modules/_csv.c
Modules/_multiprocessing/posixshmem.c
Modules/_testcapi/exceptions.c
Modules/grpmodule.c
Modules/syslogmodule.c

If we convert these modules to the limited C API, see issue #85283, the Py_ARGUMENT_CLINIC_AVOID_INTERNAL_CAPI macro can be removed (from AC and from these modules).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner force-pushed the clinic_includes2 branch from 65a4548 to e8ea3f9 Compare August 31, 2023 11:54
@vstinner

vstinner commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

Oh, global objects are outdated in the main branch, see PR #108714.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner added the skip news label Aug 31, 2023
@vstinner

vstinner commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

Hum. Windows fails to build Modules\_testinternalcapi.c:

Error: D:\a\cpython\cpython\Modules\clinic\_testinternalcapi.c.h(30,20): error C2099: initializer is not a constant [D:\a\cpython\cpython\PCbuild\_testinternalcapi.vcxproj]
Error: D:\a\cpython\cpython\Modules\clinic\_testinternalcapi.c.h(31,20): error C2099: initializer is not a constant [D:\a\cpython\cpython\PCbuild\_testinternalcapi.vcxproj]
Error: D:\a\cpython\cpython\Modules\clinic\_testinternalcapi.c.h(85,20): error C2099: initializer is not a constant [D:\a\cpython\cpython\PCbuild\_testinternalcapi.vcxproj]
Error: D:\a\cpython\cpython\Modules\clinic\_testinternalcapi.c.h(86,20): error C2099: initializer is not a constant [D:\a\cpython\cpython\PCbuild\_testinternalcapi.vcxproj]
Error: D:\a\cpython\cpython\Modules\clinic\_testinternalcapi.c.h(152,20): error C2099: initializer is not a constant [D:\a\cpython\cpython\PCbuild\_testinternalcapi.vcxproj]
Error: D:\a\cpython\cpython\Modules\clinic\_testinternalcapi.c.h(153,20): error C2099: initializer is not a constant [D:\a\cpython\cpython\PCbuild\_testinternalcapi.vcxproj]
Error: D:\a\cpython\cpython\Modules\clinic\_testinternalcapi.c.h(211,20): error C2099: initializer is not a constant [D:\a\cpython\cpython\PCbuild\_testinternalcapi.vcxproj]
Error: D:\a\cpython\cpython\Modules\clinic\_testinternalcapi.c.h(212,20): error C2099: initializer is not a constant [D:\a\cpython\cpython\PCbuild\_testinternalcapi.vcxproj]

Related code, I added error C2099: initializer is not a constant comments:

    static struct {
        PyGC_Head _this_is_not_used;
        PyObject_VAR_HEAD
        PyObject *ob_item[NUM_KEYWORDS];
    } _kwtuple = {
        .ob_base = PyVarObject_HEAD_INIT(&PyTuple_Type, NUM_KEYWORDS)  // error C2099: initializer is not a constant
        .ob_item = { &_Py_ID(doc), },  // error C2099: initializer is not a constant
    };
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner force-pushed the clinic_includes2 branch 2 times, most recently from b8efef7 to f989d08 Compare August 31, 2023 14:04
@vstinner
vstinner force-pushed the clinic_includes2 branch from f989d08 to 750b40b Compare August 31, 2023 14:12
@vstinner
pythongh-107603: AC only includes pycore_gc.h if needed
df2c754
Argument Clinic now only includes pycore_gc.h if PyGC_Head is needed,
and only includes pycore_runtime.h if _Py_ID() is needed.

Add Py_ARGUMENT_CLINIC_AVOID_INTERNAL_CAPI macro to opt-out for the
internal C API in Argument Clinic.

The following extensions avoids the internal C API by defining the
macro Py_ARGUMENT_CLINIC_AVOID_INTERNAL_CAPI:

* Modules/_csv.c
* Modules/_multiprocessing/posixshmem.c
* Modules/_testcapi/exceptions.c
* Modules/grpmodule.c
* Modules/syslogmodule.c
@vstinner
vstinner force-pushed the clinic_includes2 branch from 750b40b to df2c754 Compare August 31, 2023 14:24
@vstinner

vstinner commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

This approach doesn't work. I abandon this PR in favor of PR #108726 which is implemented differently.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner closed this Aug 31, 2023
@vstinner
vstinner deleted the clinic_includes2 branch August 31, 2023 15:57
@gvanrossum

gvanrossum commented Aug 31, 2023

Copy link
Copy Markdown
Member

Victor, next time can you use a draft PR for such experiments so 17 core devs aren't bombarded with review requests and internal monologue?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

Victor, next time can you use a draft PR for such experiments so 17 core devs aren't bombarded with review requests and internal monologue?

If I create a PR as draft, no notification is sent?

Well, I expected this PR to be ready, but then I saw that tests failed on Windows. I wrote a new PR from scratch with a different approach to avoid this.

I'm not sure why so many developers are requested for reviews, the vast majority of modified files are generated files (*/clinic/*.h files generated by Argument Clinic).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Aug 31, 2023

Copy link
Copy Markdown
Member

If I create a PR as draft, no notification is sent?

yes, code owners are only requested for review when you take it out of draft mode

I'm not sure why so many developers are requested for reviews, the vast majority of modified files are generated files (*/clinic/*.h files generated by Argument Clinic).

I don't think the GitHub CODEOWNERS feature takes any notice of whether files are generated or not when figuring out whether people should be pinged for review -- it just looks at whether the modified files match any of the globs provided in https://github.com/python/cpython/blob/main/.github/CODEOWNERS

vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner
@AlexWaygood AlexWaygood Awaiting requested review from AlexWaygood
@corona10 corona10 Awaiting requested review from corona10
@ethanfurman ethanfurman Awaiting requested review from ethanfurman
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner
@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra
@rhettinger rhettinger Awaiting requested review from rhettinger
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
@pganssle pganssle Awaiting requested review from pganssle
@abalkin abalkin Awaiting requested review from abalkin
@brettcannon brettcannon Awaiting requested review from brettcannon
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently
@ncoghlan ncoghlan Awaiting requested review from ncoghlan
@warsaw warsaw Awaiting requested review from warsaw
@tiran tiran Awaiting requested review from tiran
@1st1 1st1 Awaiting requested review from 1st1
@asvetlov asvetlov Awaiting requested review from asvetlov
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303
@willingc willingc Awaiting requested review from willingc

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review skip news

Projects

None yet

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.

4 participants

@vstinner @gvanrossum @AlexWaygood @bedevere-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.