bpo-32124: Document C functions safe before init by vstinner · Pull Request #4540 · python/cpython

vstinner · 2017-11-24T13:22:19Z

Explicitly document C functions and C variables that can be set
before Py_Initialize().

https://bugs.python.org/issue32124

Explicitly document C functions and C variables that can be set before Py_Initialize().

Change also how functions are listed: one per line.

Mariatta · 2017-11-24T13:46:07Z

Doc/c-api/init.rst

+
+  * :c:func:`PyMem_RawMalloc`
+  * :c:func:`PyMem_RawRealloc`
+    :c:func:`PyMem_RawCalloc`


Should there be a * in the beginning, like the rest?

Typo, fixed

serhiy-storchaka · 2017-11-24T14:10:50Z

Doc/c-api/init.rst

+  * :c:func:`PyObject_Calloc`
+  * :c:func:`PyObject_Free`
+
+The following flags can also be set:


Is not this the all flags?

I changed the text.

serhiy-storchaka · 2017-11-24T14:12:15Z

Doc/c-api/init.rst

+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:


PyImport_AppendInittab(), PyImport_ExtendInittab(), PyInitFrozenExtensions().

serhiy-storchaka · 2017-11-24T14:17:09Z

Doc/c-api/init.rst

+  * :c:func:`PyObject_SetArenaAllocator`
+  * :c:func:`Py_DecodeLocale`
+  * :c:func:`Py_GetProgramName`
+  * :c:func:`Py_SetPath`


If Py_GetProgramName is included, shouldn't other Py_Get* functions be included? Py_GetPath(), Py_GetVersion(), and many others.

Py_GetPath() must not be called before Py_Initialize().

I added other Py_GetXXX() functions.

serhiy-storchaka · 2017-11-24T14:18:38Z

Doc/c-api/init.rst

+
+.. :c:data: Py_BytesWarningFlag
+
+  The :option:`-b` option.


Wrong indentation (should be 3 spaces).

Document what is the value if use -bb. And the same for -OO below.

serhiy-storchaka · 2017-11-24T14:21:08Z

Doc/c-api/init.rst

+and options. By default, these flags are controlled by :ref:`command line
+options <using-on-interface-options>`.
+
+.. :c:data: Py_BytesWarningFlag


.. c:var:: Py_BytesWarningFlag

vstinner · 2017-11-24T15:59:34Z

@Mariatta, @serhiy-storchaka: I updated my PR. Would you mind to review it again?

vstinner · 2017-11-24T16:09:44Z

Doc/c-api/init.rst

+
+   Private flag used by ``_freeze_importlib`` and ``frozenmain`` programs.
+
+.. c:var:: Py_UseClassExceptionsFlag


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

Mariatta

Noticed a typo and grammar issues :)

Mariatta · 2017-11-24T17:30:42Z

Doc/c-api/init.rst

+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:


Typo :) safetely -> safely

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? :-)

Mariatta · 2017-11-24T18:05:54Z

Doc/c-api/init.rst

+
+.. c:var:: Py_DebugFlag
+
+   Turn on parser debugging output (for wizards only, depending on compilation


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.. 🤷‍♀️

I think Victor meant real wizards, like his and me. 🎩

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.

This wording already is used two times in the documentation. In particularly in the documentation for the -d option. This is very old documentation.

In sum, this option is only for those who understand this jargon. 😉

I copied the doc from the -d option documentation.

Ok, I replaced "for wizards only" with "for expert only".

I would keep the original wording. It is used also in the man page since the initial edition in 1994.

Mariatta · 2017-11-24T18:08:56Z

Doc/c-api/init.rst

+   Set to ``1`` if the :envvar:`PYTHONLEGACYWINDOWSFSENCODING` environment
+   variable is set to a non-empty string.
+
+   See also the :pep:`529`.


See also :pep:`529`.

(without "the")

I picked: See :pep:529 for more details.

Mariatta · 2017-11-24T18:09:28Z

Doc/c-api/init.rst

+   Set to ``1`` if the :envvar:`PYTHONLEGACYWINDOWSSTDIO` environment
+   variable is set to a non-empty string.
+
+   See also the :pep:`528`.


Remove "the".

bedevere-bot · 2017-11-24T18:11:29Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

serhiy-storchaka

What about adding an explicit note in the description of all of these functions that they can be called before Py_Initialize()?

serhiy-storchaka · 2017-11-24T17:32:03Z

Doc/c-api/init.rst

+   Set to ``1`` if the :envvar:`PYTHONLEGACYWINDOWSFSENCODING` environment
+   variable is set to a non-empty string.
+
+   See also the :pep:`529`.


See :pep:`529` for more details.

serhiy-storchaka · 2017-11-24T17:38:27Z

Doc/c-api/init.rst

+  * :c:func:`PyMem_SetupDebugHooks`
+  * :c:func:`PyObject_GetArenaAllocator`
+  * :c:func:`PyObject_SetArenaAllocator`
+  * :c:func:`Py_GetBuildInfo`


I would group Py_GetBuildInfo() etc as "informative functions".

serhiy-storchaka · 2017-11-24T18:17:17Z

Doc/c-api/init.rst

+
+.. c:var:: Py_DebugFlag
+
+   Turn on parser debugging output (for wizards only, depending on compilation


I think Victor meant real wizards, like his and me. 🎩

serhiy-storchaka · 2017-11-24T18:18:24Z

Doc/c-api/init.rst



-.. c:var:: int PyTrace_CALL
+.. c:var::: int PyTrace_CALL


Isn't the third colon redundant?

Oops, it's a mistake. Fixed.

vstinner · 2017-11-24T20:21:37Z

I have made the requested changes; please review again.

bedevere-bot · 2017-11-24T20:21:39Z

Thanks for making the requested changes!

@Mariatta: please review the changes made to this pull request.

Mariatta · 2017-11-24T20:57:13Z

Thanks @vstinner.
I personally prefer the word "experts" over "wizards". It's not a jargon or idiom, something the general public will understand :)
Thanks for changing it.

serhiy-storchaka · 2017-11-24T21:19:01Z

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.

vstinner · 2017-11-24T21:24:46Z

This option is not for the general public.

"expert only" doesn't mean "generic public", it's more the opposite.

If change the wording, you should change also the man page.

Done.

vstinner · 2017-11-24T21:31:03Z

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 :-)

ericsnowcurrently · 2017-11-24T21:18:17Z

Doc/c-api/init.rst

+
+.. note::
+
+   The following functions **should not be called** before


Would you mind putting these into a list?

ericsnowcurrently · 2017-11-24T21:24:19Z

Doc/c-api/init.rst

+
+  * :c:func:`PyImport_AppendInittab`
+  * :c:func:`PyImport_ExtendInittab`
+  * :c:func:`PyInitFrozenExtensions`


Why is PyInitFrozenExtensions() in this list?

It was a request from @serhiy-storchaka. I don't know the rationale :-)

It is called before Py_Initialize() in Python/frozenmain.c on Windows.

ericsnowcurrently · 2017-11-24T21:29:15Z

Doc/c-api/init.rst

+  * :c:func:`PyInitFrozenExtensions`
+  * :c:func:`PyMem_SetAllocator`
+  * :c:func:`PyMem_SetupDebugHooks`
+  * :c:func:`PyObject_SetArenaAllocator`


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?

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 · 2017-11-24T21:32:32Z

Doc/c-api/init.rst

+  * :c:func:`PyImport_AppendInittab`
+  * :c:func:`PyImport_ExtendInittab`
+  * :c:func:`PyInitFrozenExtensions`
+  * :c:func:`PyMem_SetAllocator`


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

Setting the allocators pre-initialization is required, as that allows the embedding application to control exactly how CPython allocates memory during startup.

ericsnowcurrently · 2017-11-24T21:58:25Z

Doc/c-api/init.rst

+
+.. _global-conf-vars:
+
+Global configuration variables


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. :)

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.

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").

ericsnowcurrently · 2017-11-24T22:00:58Z

Sorry I didn't get my review out sooner.

ncoghlan

Thanks @vstinner, this looks good to me, and defines a solid set of backwards compatibility constraints for the PEP 432 refactoring work.

ncoghlan · 2017-11-25T02:40:16Z

Doc/c-api/init.rst

+  * :c:func:`PyImport_AppendInittab`
+  * :c:func:`PyImport_ExtendInittab`
+  * :c:func:`PyInitFrozenExtensions`
+  * :c:func:`PyMem_SetAllocator`


Setting the allocators pre-initialization is required, as that allows the embedding application to control exactly how CPython allocates memory during startup.

ncoghlan · 2017-11-25T02:41:45Z

Doc/c-api/init.rst

+  * :c:func:`PyInitFrozenExtensions`
+  * :c:func:`PyMem_SetAllocator`
+  * :c:func:`PyMem_SetupDebugHooks`
+  * :c:func:`PyObject_SetArenaAllocator`


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.

vstinner · 2017-11-28T23:27:16Z

@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.

vstinner added the skip news label Nov 24, 2017

the-knights-who-say-ni added the CLA signed label Nov 24, 2017

bedevere-bot added the awaiting merge label Nov 24, 2017

vstinner added 2 commits November 24, 2017 14:25

bpo-32124: Document C functions safe before init

b1375be

Explicitly document C functions and C variables that can be set before Py_Initialize().

Add Py_DecodeLocale()

938684e

Change also how functions are listed: one per line.

Mariatta reviewed Nov 24, 2017

View reviewed changes

serhiy-storchaka reviewed Nov 24, 2017

View reviewed changes

Complete and fix the Preinit doc

12b3b5f

vstinner commented Nov 24, 2017

View reviewed changes

Mariatta requested changes Nov 24, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Nov 24, 2017

serhiy-storchaka reviewed Nov 24, 2017

View reviewed changes

Address new round of comments

b58aba4

bedevere-bot removed the awaiting changes label Nov 24, 2017

bedevere-bot added the awaiting change review label Nov 24, 2017

vstinner added 2 commits November 24, 2017 21:23

Fix typo

b2fc166

Only keep PyMem_Raw memory allocators

e01c316

Mariatta approved these changes Nov 24, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Nov 24, 2017

man page: wizards => expert

f1c305b

vstinner merged commit 84c4b19 into python:master Nov 24, 2017

bedevere-bot removed the awaiting merge label Nov 24, 2017

vstinner deleted the preinit_doc branch November 24, 2017 21:30

ericsnowcurrently reviewed Nov 24, 2017

View reviewed changes

ncoghlan reviewed Nov 25, 2017

View reviewed changes


		Private flag used by ``_freeze_importlib`` and ``frozenmain`` programs.

		.. c:var:: Py_UseClassExceptionsFlag


		.. c:var:: Py_DebugFlag

		Turn on parser debugging output (for wizards only, depending on compilation


		.. note::

		The following functions should not be called before

Uh oh!

Conversation

vstinner commented Nov 24, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mariatta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 24, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 24, 2017

Uh oh!

bedevere-bot commented Nov 24, 2017

Uh oh!

Mariatta commented Nov 24, 2017

vstinner commented Nov 24, 2017 •

edited by bedevere-bot

Loading