classification
Title: Do we support calling Py_Main() after Py_Initialize()?
Type: behavior Stage: resolved
Components: Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: belopolsky, emilyemorehouse, eric.snow, hroncok, mcepl, ncoghlan, vstinner
Priority: normal Keywords: patch

Created on 2018-06-30 06:58 by ncoghlan, last changed 2018-07-24 13:39 by ncoghlan. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 8023 open ncoghlan, 2018-06-30 11:49
PR 8043 merged vstinner, 2018-07-02 08:58
PR 8352 merged miss-islington, 2018-07-20 15:34
Messages (18)
msg320758 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-06-30 06:58
In the current documentation, Py_Main is listed as a regular C API function: https://docs.python.org/3/c-api/veryhigh.html#c.Py_Main

We also didn't add Py_Main() to https://docs.python.org/3/c-api/init.html#before-python-initialization when we did the review for "Functions which are safe to call before Py_Initialize()".

If it truly is a regular C API function, then this suggests that Py_Main() should be called *after* Py_Initialize().

However, Py_Main() has historically called Py_Initialize() *itself*, and generally assumes it has total control over management of the current process - it doesn't expect to be sharing the current process with an embedding application.

In Python 3.7, Py_Main() was changed to call _Py_InitializeCore() and _Py_InitializeMainInterpreter() as separate steps, allowing it to detect cases where an embedding application had already called Py_Initialize(), meaning that Py_Main()'s config setting changes wouldn't be applied.

In effect, we've converted a silent failure (settings not read and applied as expected) into a noisy one (attempting to call Py_Main() in an already initialised interpreter).

(The test suite is silent on the matter, since the idea of an embedding application calling Py_Initialize() before calling Py_Main() never even occurred to us)

So the question is, what should we do about this for Python 3.7:

1. Treat it as a regression, try to replicate the old behaviour of partially (but not completely) ignoring the config settings read by Py_Main()
2. Treat it as a regression, but don't try to replicate the old config-settings-are-partially-applied behaviour - just ignore the settings read by Py_Main() completely
3. Treat it as a long standing documentation error, update the docs to make it clear that Py_Main() expects to handle calling Py_Initialize() itself, and update the Python 3.7 porting guide accordingly
msg320760 - (view) Author: Miro Hrončok (hroncok) * Date: 2018-06-30 07:06
This hits fontforge. See https://bugzilla.redhat.com/show_bug.cgi?id=1595421

An example of how it should be handled there if 3) is selected would be greatly appreciated.  Thanks
msg320768 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-06-30 10:21
Even if it's ugly, calling Py_Main() after Py_Initialize() works in Python
3.6 by ignoring the new config of Py_Main(). I suggest to do nothing (just
return) when Py_Initialize() is called again by Py_Main() in Python 3.7. It
would fix the fontforge *regression*.

I suggest to only enhance the code (really apply the new Py_Main() config)
in the master branch. From what I saw, apply the new config is not trivial.
It will take time to decide what to do for *each* option.
msg320771 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-06-30 10:47
Yeah, I've asked Miro to try essentially that in https://bugzilla.redhat.com/show_bug.cgi?id=1595421#c12

My concern is that Py_Main used to keep a *lot* of state on the local C stack that we've now moved into the main interpreter config.

So my suspicion is that even that change I've asked Miro to try may not do the right thing, but it really depends on *why* fontforge is calling Py_Main, and what they're passing in argc and argv.

Given that the embedded Py_Initialize is a no-op, they may be better off just copying out the parts they actually need from the code execution aspects of it, and skipping the Py_Main call entirely.
msg320772 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-06-30 11:10
At the very least, I think this calls for a documentation change, such that we make it clear that:

1. The expected calling pattern is to call Py_Main() *instead of* Py_Initialize(), since Py_Main() calls Py_Initialize().

2. If you do call Py_Initialize() first, then exactly which settings that Py_Main() will read from the environment and apply is version dependent.

So I'll put together a docs patch that's valid for 3.6+, and then we can decide where to take 3.7 and 3.8 from an implementation perspective after that.

Currently, I'm thinking that for 3.7, our goal should specifically be "fake the 3.6 behaviour well enough to get fontforge working again" (so it will mostly be Fedora that drives what that 3.7.1 change looks like), while for 3.8 we should aim to deprecate that code path, and instead offer a better building block API for folks that want to implement their own Py_Main() variant.
msg320773 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-06-30 11:59
Removing 3.6 from the affected versions, since we re-arranged these docs a bit for 3.7 to make it clearer which functions could be called prior to initialization, as well as adding more tests for the actual pre-init functionality.

Since 3.6 isn't going to change, and we'll be aiming to get back to something a bit closer to the 3.6 behaviour for 3.7.1, it seems simpler to just focus on 3.7 and 3.8.
msg320857 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-02 09:01
My PR 8043 fix the Python 3.7 regression: allow again to call Py_Main() after Py_Initialize().

Nick: if you want to raise an error on that case, I would prefer to see a deprecation warning emitted in version N before raising an error in version N+1.

Technically, it seems possible to allow calling Py_Main() after Py_Initialize(). It's "just" a matter of fixing Py_Main() to apply properly the new configuration.
msg321036 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-04 11:39
Aye, and I think from Miro's comments on your PR, making it apply some of the same configuration settings that 3.6 and earlier applied is going to be required to get fontforge working again for 3.7.

At the very least, the call to set sys.argv is needed.
msg321384 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-10 12:51
Thinking about this some more, I'm inclined to go the same way we did with issue 33932: classify it as an outright regression, work out the desired requirements for a missing embedding test case, and then fix the implementation to pass the new test case.

My suggestion for test commands would be:

    ../Lib/site.py
    -m site
    -c 'import sys; print(sys.argv)' some test data here

Once those work properly, we'd consider the regression relative to Python 3.6 fixed.

Those could either be 3 different test cases, or else we could run them all within a single test case, with a Py_Initialize() call before each one (since Py_Main() calls Py_Finalize() internally).
msg321565 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-12 15:27
I tested fontforge. It became clear to me that the fontforge must be able to call Py_Initialize() and only later call Py_Main(). fontforge calls Py_Initialize() and then uses the Python API:

/* This is called to start up the embedded python interpreter */
void FontForge_InitializeEmbeddedPython(void) {
    // static int python_initialized is declared above.
    if ( python_initialized )
	return;

    SetPythonProgramName("fontforge");
    RegisterAllPyModules();
    Py_Initialize();
    python_initialized = 1;

    /* The embedded python interpreter is now functionally
     * "running". We can modify it to our needs.
     */
    CreateAllPyModules();
    InitializePythonMainNamespace();
}

Py_Main() is called after FontForge_InitializeEmbeddedPython().

To be clear, Py_Main() must be fixed in Python 3.7 to apply properly the new configuration, or *at least* define sys.argv.
msg322006 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-20 13:33
> This hits fontforge. See https://bugzilla.redhat.com/show_bug.cgi?id=1595421

Copy of my comment 20:



I looked one more time to the issue:

* fontforge code is fine: it calls Py_Initialize() before using the Python C API to initialize its Python namespace (modules, functions, types, etc.), and later it calls Py_Main().

* Python 3.7.0 is wrong: it fails with a fatal erro when Py_Main() is called if Py_Initialize() has been called previously. It is a regression since it worked fine in Python 3.6.

* My PR https://github.com/python/cpython/pull/8043 works again the simplest test: "fontforge -script hello.py" where hello.py is just "print('Hello World!')"

* This PR is not enough according to Miro, since fontforge requires sys.argv to be set, whereas sys.argv is no longer set when Py_Main() is after Py_Initialize(): https://github.com/python/cpython/pull/8043#issuecomment-401731992
msg322007 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-20 13:38
I looked one more time at Python 3.6, code before my huge Py_Main()/Py_Initialize() refactoring, before _PyCoreConfig/_PyMainInterpreterConfig have been added. In short, calling Py_Main() after Py_Initialize() "works" in Python 3.6 but only sys.argv is set: almost all other options are lost/ignored. For example, if you call Py_Initialize() you get a sys.path from the current environment, but if Py_Main() gets a new module search path: sys.path is not updated.

I modified PR 8043 to write the minimum patch just to fix fontforge. When Py_Main() is called Py_Initialize(): just set sys.argv, that's all.

If someone wants to fix this use case, apply properly the new Py_Main() configuration carefully, I would suggest to only work in the master branch: that's out of the scope of this specific regression.

IMHO it would be nice to enhance this use case. For example, it would be "nice" to update at least "sys.path" (and other related variables) in such case.
msg322011 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-20 14:59
+1 from me for the idea of fixing Python 3.7 to correctly set sys.argv in this case (and leaving everything else unmodified).

As far as further enhancement in Python 3.8 goes, perhaps that can be seen as part of PEP 432, such that embedding applications have to explicitly opt-in to any new behaviour by calling the new APIs?
msg322017 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-20 15:27
I created bpo-34170: "Py_Initialize(): computing path configuration must not have side effect (PEP 432)" as a follow-up of this issue.
msg322019 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-20 15:34
New changeset fb47bca9ee2d07ce96df94b4e4abafd11826eb01 by Victor Stinner in branch 'master':
bpo-34008: Allow to call Py_Main() after Py_Initialize() (GH-8043)
https://github.com/python/cpython/commit/fb47bca9ee2d07ce96df94b4e4abafd11826eb01
msg322041 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-21 00:16
New changeset 03ec4df67d6b4ce93a2da21db7c84dff8259953f by Victor Stinner (Miss Islington (bot)) in branch '3.7':
bpo-34008: Allow to call Py_Main() after Py_Initialize() (GH-8043) (GH-8352)
https://github.com/python/cpython/commit/03ec4df67d6b4ce93a2da21db7c84dff8259953f
msg322044 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-21 00:20
Ok, I fixed the fontforge bug in 3.7 and master branches. Sorry for the regression, but I really didn't expect that anyone would call Py_Main() after Py_Initialize() :-) I guess we should be prepared for such hiccup when reworking the Python initialization :-)

Nick: Can I close the issue, or do you want to keep it open to change the documentation?
msg322302 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-24 13:39
Since we decided the correct resolution here was to restore the Python 3.6 behaviour, I've filed https://bugs.python.org/issue34206 as a separate docs clarification issue (I'll amend my PR accordingly)
History
Date User Action Args
2018-07-24 13:39:09ncoghlansetstatus: open -> closed
resolution: fixed
messages: + msg322302

stage: patch review -> resolved
2018-07-24 13:37:59ncoghlanlinkissue34206 dependencies
2018-07-21 00:20:07vstinnersetmessages: + msg322044
2018-07-21 00:16:26vstinnersetmessages: + msg322041
2018-07-20 15:34:50miss-islingtonsetpull_requests: + pull_request7887
2018-07-20 15:34:25vstinnersetmessages: + msg322019
2018-07-20 15:27:00vstinnersetmessages: + msg322017
2018-07-20 14:59:12ncoghlansetmessages: + msg322011
2018-07-20 13:38:13vstinnersetmessages: + msg322007
2018-07-20 13:33:39vstinnersetmessages: + msg322006
2018-07-12 15:27:04vstinnersetmessages: + msg321565
2018-07-10 12:51:40ncoghlansetmessages: + msg321384
2018-07-04 23:18:53belopolskysetnosy: + belopolsky
2018-07-04 11:39:17ncoghlansetmessages: + msg321036
2018-07-02 12:35:44mceplsetnosy: + mcepl
2018-07-02 09:01:54vstinnersetmessages: + msg320857
2018-07-02 08:58:56vstinnersetpull_requests: + pull_request7652
2018-06-30 11:59:43ncoghlansetmessages: + msg320773
versions: - Python 3.6
2018-06-30 11:49:05ncoghlansetkeywords: + patch
stage: test needed -> patch review
pull_requests: + pull_request7632
2018-06-30 11:10:30ncoghlansetmessages: + msg320772
2018-06-30 10:47:28ncoghlansetmessages: + msg320771
2018-06-30 10:21:46vstinnersetmessages: + msg320768
2018-06-30 07:06:10hroncoksetnosy: + hroncok
messages: + msg320760
2018-06-30 07:03:00ncoghlansetnosy: + emilyemorehouse
2018-06-30 06:58:41ncoghlancreate