Conversation
Add "make check-capi-macros". Ignore existing macros which don't start with "Py", only prevent adding new names which don't respect this convention. Run "make check-capi-macros" in the GitHub Action "Check if generated files are up to date" job.
ZeroIntensity
left a comment
There was a problem hiding this comment.
I think this is a great idea. For clarity, do we already run similar checks for actual symbols? (For example, it should be impossible to add a new function to our headers that isn't prefixed with Py.)
We do already have So I'm not sure that it's useful to parse the public C API to check if PyAPI_FUNC() and PyAPI_DATA() use the Last year, I found exported symbols which doesn't with |
Yeah, as long as we have something, then I'm happy. |
|
I wrote a quick patch to test "make smelly": diff --git a/Include/pyerrors.h b/Include/pyerrors.h
index cfabbc5fe8d..f74534da7e6 100644
--- a/Include/pyerrors.h
+++ b/Include/pyerrors.h
@@ -326,6 +326,8 @@ PyAPI_FUNC(int) PyOS_snprintf(char *str, size_t size, const char *format, ...)
PyAPI_FUNC(int) PyOS_vsnprintf(char *str, size_t size, const char *format, va_list va)
Py_GCC_ATTRIBUTE((format(printf, 3, 0)));
+PyAPI_FUNC(int) test_dummy_func(void);
+
#ifndef Py_LIMITED_API
# define Py_CPYTHON_ERRORS_H
# include "cpython/pyerrors.h"
diff --git a/Python/errors.c b/Python/errors.c
index eb148998fc4..df7d3241dcc 100644
--- a/Python/errors.c
+++ b/Python/errors.c
@@ -2091,3 +2091,9 @@ PyErr_ProgramTextObject(PyObject *filename, int lineno)
{
return _PyErr_ProgramDecodedTextObject(filename, lineno, NULL);
}
+
+int
+test_dummy_func(void)
+{
+ return 4;
+}With this change, "make smelly" fails as expected with: |
| - name: Check for unsupported C global variables | ||
| if: github.event_name == 'pull_request' # $GITHUB_EVENT_NAME | ||
| run: make check-c-globals | ||
| - name: Check C ABI macros |
There was a problem hiding this comment.
Should this be "API" instead of "ABI"? I think it's weird to refer to the ABI when we're not actually dealing with exported symbols.
| - name: Check C ABI macros | |
| - name: Check C API macros |
| names.sort() | ||
|
|
||
| if not names: | ||
| print("OK: the Python C API only defines macros with name " |
There was a problem hiding this comment.
| print("OK: the Python C API only defines macros with name " | |
| print("OK: the Python C API only defines macros with names " |
| print('ERROR: the Python C API defines the following macros ' | ||
| 'with a name not starting with "Py":') | ||
| print() |
There was a problem hiding this comment.
Up to you, but it'd be nice to have colorful output with _colorize (the C API docs check script does this).
| names = [] | ||
| for filename in files: | ||
| parse_file(filename, names, ignored) |
There was a problem hiding this comment.
Consider using a set instead of a list here. Currently, this will contain duplicated names if a #define is used twice, such as in an #ifdef.
Add "make check-capi-macros". Ignore existing macros which don't start with "Py", only prevent adding new names which don't respect this convention.
Run "make check-capi-macros" in the GitHub Action "Check if generated files are up to date" job.
Py#157695