bpo-32124: Document C functions safe before init #4540
Conversation
Explicitly document C functions and C variables that can be set before Py_Initialize().
Change also how functions are listed: one per line.
Doc/c-api/init.rst
Outdated
|
|
||
| * :c:func:`PyMem_RawMalloc` | ||
| * :c:func:`PyMem_RawRealloc` | ||
| :c:func:`PyMem_RawCalloc` |
There was a problem hiding this comment.
Should there be a * in the beginning, like the rest?
Doc/c-api/init.rst
Outdated
| * :c:func:`PyObject_Calloc` | ||
| * :c:func:`PyObject_Free` | ||
|
|
||
| The following flags can also be set: |
There was a problem hiding this comment.
Is not this the all flags?
Doc/c-api/init.rst
Outdated
| be called before using any other Python/C API functions; with the exception of | ||
| a few functions and flags listed below. | ||
|
|
||
| The following functions can be safetely called before Python is initialized: |
There was a problem hiding this comment.
PyImport_AppendInittab(), PyImport_ExtendInittab(), PyInitFrozenExtensions().
Doc/c-api/init.rst
Outdated
| * :c:func:`PyObject_SetArenaAllocator` | ||
| * :c:func:`Py_DecodeLocale` | ||
| * :c:func:`Py_GetProgramName` | ||
| * :c:func:`Py_SetPath` |
There was a problem hiding this comment.
If Py_GetProgramName is included, shouldn't other Py_Get* functions be included? Py_GetPath(), Py_GetVersion(), and many others.
There was a problem hiding this comment.
Py_GetPath() must not be called before Py_Initialize().
I added other Py_GetXXX() functions.
Doc/c-api/init.rst
Outdated
|
|
||
| .. :c:data: Py_BytesWarningFlag | ||
|
|
||
| The :option:`-b` option. |
There was a problem hiding this comment.
Wrong indentation (should be 3 spaces).
Document what is the value if use -bb. And the same for -OO below.
Doc/c-api/init.rst
Outdated
| and options. By default, these flags are controlled by :ref:`command line | ||
| options <using-on-interface-options>`. | ||
|
|
||
| .. :c:data: Py_BytesWarningFlag |
There was a problem hiding this comment.
.. c:var:: Py_BytesWarningFlag
|
@Mariatta, @serhiy-storchaka: I updated my PR. Would you mind to review it again? |
Doc/c-api/init.rst
Outdated
|
|
||
| Private flag used by ``_freeze_importlib`` and ``frozenmain`` programs. | ||
|
|
||
| .. c:var:: Py_UseClassExceptionsFlag |
There was a problem hiding this comment.
I'm not sure that it's a good idea to document this flag, since I proposed to remove the flag https://bugs.python.org/issue32125
Doc/c-api/init.rst
Outdated
| be called before using any other Python/C API functions; with the exception of | ||
| a few functions and the :ref:`global configuration variables <global-conf-vars>`: | ||
|
|
||
| The following functions can be safetely called before Python is initialized: |
There was a problem hiding this comment.
Fixed.
Crap. "safetely" looked weird, so I used my spell check: "Google safetely", but I found results. Maybe there is an issue with my spell checker? :-)
Doc/c-api/init.rst
Outdated
|
|
||
| .. c:var:: Py_DebugFlag | ||
|
|
||
| Turn on parser debugging output (for wizards only, depending on compilation |
There was a problem hiding this comment.
I see the word wizards and thought of magicians :) Do you mean something like setup wizard?
Or only a real wizard/magician can use this flag? 😅
Maybe this is a common terminology and I'm just too new.. 🤷♀️
There was a problem hiding this comment.
I think Victor meant real wizards, like his and me. 🎩
There was a problem hiding this comment.
Ok in that case I suggest something less ambiguous, like "for expert" or "for experienced core devs" only.
This sentence also missed a period (.) at the end.
There was a problem hiding this comment.
This wording already is used two times in the documentation. In particularly in the documentation for the -d option. This is very old documentation.
There was a problem hiding this comment.
In sum, this option is only for those who understand this jargon. 😉
There was a problem hiding this comment.
I copied the doc from the -d option documentation.
Ok, I replaced "for wizards only" with "for expert only".
There was a problem hiding this comment.
I would keep the original wording. It is used also in the man page since the initial edition in 1994.
Doc/c-api/init.rst
Outdated
| Set to ``1`` if the :envvar:`PYTHONLEGACYWINDOWSFSENCODING` environment | ||
| variable is set to a non-empty string. | ||
|
|
||
| See also the :pep:`529`. |
There was a problem hiding this comment.
See also :pep:`529`.
(without "the")
There was a problem hiding this comment.
I picked: See :pep:529 for more details.
Doc/c-api/init.rst
Outdated
| Set to ``1`` if the :envvar:`PYTHONLEGACYWINDOWSSTDIO` environment | ||
| variable is set to a non-empty string. | ||
|
|
||
| See also the :pep:`528`. |
|
When you're done making the requested changes, leave the comment: |
Doc/c-api/init.rst
Outdated
| Set to ``1`` if the :envvar:`PYTHONLEGACYWINDOWSFSENCODING` environment | ||
| variable is set to a non-empty string. | ||
|
|
||
| See also the :pep:`529`. |
There was a problem hiding this comment.
See :pep:`529` for more details.
Doc/c-api/init.rst
Outdated
| * :c:func:`PyMem_SetupDebugHooks` | ||
| * :c:func:`PyObject_GetArenaAllocator` | ||
| * :c:func:`PyObject_SetArenaAllocator` | ||
| * :c:func:`Py_GetBuildInfo` |
There was a problem hiding this comment.
I would group Py_GetBuildInfo() etc as "informative functions".
Doc/c-api/init.rst
Outdated
|
|
||
| .. c:var:: Py_DebugFlag | ||
|
|
||
| Turn on parser debugging output (for wizards only, depending on compilation |
There was a problem hiding this comment.
I think Victor meant real wizards, like his and me. 🎩
Doc/c-api/init.rst
Outdated
| @@ -1092,7 +1315,7 @@ Python-level trace functions in previous versions. | |||
| +------------------------------+--------------------------------------+ | |||
|
|
|||
|
|
|||
| .. c:var:: int PyTrace_CALL | |||
| .. c:var::: int PyTrace_CALL | |||
There was a problem hiding this comment.
Isn't the third colon redundant?
There was a problem hiding this comment.
Oops, it's a mistake. Fixed.
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @Mariatta: please review the changes made to this pull request. |
|
Thanks @vstinner. |
|
This option is not for the general public. If change the wording, you should change also the man page. I think it is better to open a separate issue for changing wizards to experts. This change is not related to this issue. |
"expert only" doesn't mean "generic public", it's more the opposite.
Done. |
|
Thank you for your reviews @serhiy-storchaka and @Mariatta! I'm not confortable when I have to modify the documentation, so your feedback made me more confident in my change :-) |
|
|
||
| .. note:: | ||
|
|
||
| The following functions **should not be called** before |
There was a problem hiding this comment.
Would you mind putting these into a list?
|
|
||
| * :c:func:`PyImport_AppendInittab` | ||
| * :c:func:`PyImport_ExtendInittab` | ||
| * :c:func:`PyInitFrozenExtensions` |
There was a problem hiding this comment.
Why is PyInitFrozenExtensions() in this list?
There was a problem hiding this comment.
It was a request from @serhiy-storchaka. I don't know the rationale :-)
There was a problem hiding this comment.
It is called before Py_Initialize() in Python/frozenmain.c on Windows.
| * :c:func:`PyInitFrozenExtensions` | ||
| * :c:func:`PyMem_SetAllocator` | ||
| * :c:func:`PyMem_SetupDebugHooks` | ||
| * :c:func:`PyObject_SetArenaAllocator` |
There was a problem hiding this comment.
Why are we promising that PyObject_SetArenaAllocator() (and PyObject_GetArenaAllocator) is available before initialization? What do embedders need it for before init? Can we hold off adding it to this list until necessary?
There was a problem hiding this comment.
Same reason as for PyMem_SetAllocators: the embedding application needs to be able to specify how memory allocation during Py_Initialize() itself should be handled.
| * :c:func:`PyImport_AppendInittab` | ||
| * :c:func:`PyImport_ExtendInittab` | ||
| * :c:func:`PyInitFrozenExtensions` | ||
| * :c:func:`PyMem_SetAllocator` |
There was a problem hiding this comment.
How much of the PyMem_*() API do we really need to promise to embedders? AFAICS, we've only required them to use PyMem_RawFree() before initialization, due to Py_DecodeLocale(). The less we promise pre-init, the better.
Ideally we would provide equivalent pre-init functions, along with pre-init allocator state, that embedders could use if they needed.
If we do end up needing more, could we instead provide equivalent pre-init functions that take explicit allocator state that
There was a problem hiding this comment.
Setting the allocators pre-initialization is required, as that allows the embedding application to control exactly how CPython allocates memory during startup.
|
|
||
| .. _global-conf-vars: | ||
|
|
||
| Global configuration variables |
There was a problem hiding this comment.
In some ways, I'd rather we not advertise these flags as global variables, and particularly not as stable pre-init state. If the goal is PEP 432 then further publicizing this state helps to entrench and persist the API from which we're trying to move away.
On the other hand, PEP 432 and its API is not accepted (yet) and the status quo suggests that the documentation you've added here for existing state is useful.
From my biased position, I'd favor leaving this part out for now, but only if we're serious about PEP 432. :)
There was a problem hiding this comment.
For PEP 432, I want us to consider "Don't break backwards compatibility with the current embedding API" as a hard constraint. Part of that process is going to be reverse engineering what the current embedding API actually is in practice, since we don't have it fully documented, and we definitely don't have it fully tested.
So in a post-PEP-432 world, we'll end up with a situation where we have a "Legacy Embedding API" that we promise not to break (at least, not prior to Python 4.0 at the very earliest), and then a more fine-grained configuration-struct-based one.
There was a problem hiding this comment.
In some ways, I'd rather we not advertise these flags as global variables, and particularly not as stable pre-init state.
What do you mean by "not stable"? The inital state of these flags is well defined.
These flags are already actively used in the wild, it's lilely to only way to tune some Python options.
Not documenting them only makes Python harder to embed, it doesn't prevent users to use it.
Moreover, variables are exported as public symbols (prefixed with "Py", not "_Py").
|
Sorry I didn't get my review out sooner. |
| * :c:func:`PyImport_AppendInittab` | ||
| * :c:func:`PyImport_ExtendInittab` | ||
| * :c:func:`PyInitFrozenExtensions` | ||
| * :c:func:`PyMem_SetAllocator` |
There was a problem hiding this comment.
Setting the allocators pre-initialization is required, as that allows the embedding application to control exactly how CPython allocates memory during startup.
| * :c:func:`PyInitFrozenExtensions` | ||
| * :c:func:`PyMem_SetAllocator` | ||
| * :c:func:`PyMem_SetupDebugHooks` | ||
| * :c:func:`PyObject_SetArenaAllocator` |
There was a problem hiding this comment.
Same reason as for PyMem_SetAllocators: the embedding application needs to be able to specify how memory allocation during Py_Initialize() itself should be handled.
|
@ericsnowcurrently: I created the PR #4621 to change the formatting of the note. I also replied to your comments. Do you think that someone should be changed in the new PR #4621? Maybe comment there directly. |
Explicitly document C functions and C variables that can be set
before Py_Initialize().
https://bugs.python.org/issue32124