Skip to content
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

gh-107603: AC only includes pycore_gc.h if needed #108715

Closed
wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Aug 31, 2023

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
Copy link
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).

@vstinner
Copy link
Member Author

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

@vstinner
Copy link
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
    };

@vstinner vstinner force-pushed the clinic_includes2 branch 2 times, most recently from b8efef7 to f989d08 Compare August 31, 2023 14:04
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
Copy link
Member Author

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

@vstinner vstinner closed this Aug 31, 2023
@vstinner vstinner deleted the clinic_includes2 branch August 31, 2023 15:57
@gvanrossum
Copy link
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?

@vstinner
Copy link
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).

@AlexWaygood
Copy link
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

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

Successfully merging this pull request may close these issues.

4 participants