classification
Title: Split pylifecycle.c out from pythonrun.c
Type: enhancement Stage: resolved
Components: Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ncoghlan Nosy List: Arfrever, BreamoreBoy, barry, ncoghlan, pitrou, python-dev, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2014-11-14 11:18 by ncoghlan, last changed 2014-11-22 08:28 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
split_pythonrun.diff ncoghlan, 2014-11-14 11:18 WIP: split pythonrun into two separate modules review
split_pythonrun_v2.diff ncoghlan, 2014-11-14 11:24 Incorporated recent changes to pythonrun review
split_pythonrun_v3.diff ncoghlan, 2014-11-16 05:54 Adjusted patch as per comments on issue review
Messages (21)
msg231155 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-11-14 11:18
When working on the original reference implementation for PEP 432, I found it useful to split the startup & shutdown code out from the main entry points into the eval loop (see issue 22257 for more details).

However, that split made the draft implementation rather hard to maintain - any CPython commit that touched pythonrun.c was almost certain to cause a merge conflict.

The attached patch is a preliminary split into two separate modules, with pythonrun continuing to hold the main eval loop entry points, while the startup and shutdown code moves into pylifecycle.

This is an initial WIP patch (that nonetheless passes the test suite). I started work on it a few months ago, so I need to make sure that I've retained Victor's recent pythonrun changes.
msg231156 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-11-14 11:24
Incorporated pythonrun changes from 668e0bf30042, 6aaa0aab1e93 and d6fb87972dee into pylifecycle
msg231158 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-11-14 11:47
Known issues with the current split:

* reference count printing is duplicated (lifecycle prints it at shutdown, pythonrun at the interactive prompt)

* pythonrun references PyInspect_Flag directly without an extern declaration

* This particular oddity wasn't introduced by this patch, but it turns out PyOptimize_Flag lives in compile.c, rather than with the other global flags in pythonrun.c. It's also declared as a public data API in pydebug.h
msg231211 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-11-15 18:24
+1 on the principle.

> reference count printing is duplicated (lifecycle prints it at 
> shutdown, pythonrun at the interactive prompt)

You could make it an API in object.c.

> * pythonrun references PyInspect_Flag directly without an extern
> declaration
> 
> * This particular oddity wasn't introduced by this patch, but it 
> turns out PyOptimize_Flag lives in compile.c, rather than with the 
> other global flags in pythonrun.c. It's also declared as a public 
> data API in pydebug.h

Should be easy to fix as well :-)
msg231232 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-11-16 05:52
It turns out *all* the global config variations are in pydebug.h (and they're basically the only thing in there, aside from the environment variable access macro). That seems a little weird to me (perhaps historical due to the early global config variables being debugging related?), but for now I've just added a couple of comments cross referencing pydebug.h from pylifecycle.c and vice-versa.

The latest version of the patch:

- moves the reference count & block allocation printing to object.[ch] as Antoine suggested (Macro = _PY_DEBUG_PRINT_TOTAL_REFS(); Symbol = _PyDebug_PrintTotalRefs())

- moves Py_OptimizeFlag to pylifecycle.c together with the other global configuration flags (and tweaks the order of definitions to exactly match the order of declarations in pydebug.h)

Since the extern declaration is actually there in pydebug.h, the reference to Py_InspectFlag from pythonrun.c is actually fine for now (although removing that direct reference will likely end up being part of the PEP 432 implementation).
msg231233 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-11-16 05:54
I think this is ready to go now, unless anyone spots any major flaws I missed.
msg231234 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-11-16 05:57
Oops, "global config variations" above should have been "global config variable declarations". I guess my brain decided that was too much typing and jammed the last two words together :)
msg231425 - (view) Author: Roundup Robot (python-dev) Date: 2014-11-20 11:40
New changeset b9775a92c1d0 by Nick Coghlan in branch 'default':
Issue #22869: Split pythonrun into two modules
https://hg.python.org/cpython/rev/b9775a92c1d0
msg231450 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-11-21 00:16
I'm not absolutely sure but I think this has broken the Windows builds with the following being typical of numerous errors.

2>main.obj : error LNK2019: unresolved external symbol _Py_Finalize referenced in function _Py_Main
2>main.obj : error LNK2019: unresolved external symbol _Py_Initialize referenced in function _Py_Main
2>main.obj : error LNK2019: unresolved external symbol _Py_SetProgramName referenced in function _Py_Main
2>main.obj : error LNK2019: unresolved external symbol _Py_FdIsInteractive referenced in function _Py_Main
msg231453 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-11-21 00:30
Thanks. The Windows build files (Visual Studio project files) typically have to be updated when a new C file is added to the source tree. Probably one of our Windows experts can help with that :-)
msg231454 - (view) Author: Roundup Robot (python-dev) Date: 2014-11-21 00:46
New changeset 31fd106bb68a by Steve Dower in branch 'default':
Issue #22869: Add pylifecycle.c/.h files to pythoncore project.
https://hg.python.org/cpython/rev/31fd106bb68a
msg231455 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2014-11-21 00:47
Fixed. (I was conveniently sitting waiting for other things to build...)
msg231456 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-11-21 00:59
2>  getbuildinfo.c
2>pythonrun.obj : error LNK2005: _PyOS_CheckStack already defined in pylifecycle.obj
2>     Creating library C:\cpython\PCbuild\python35.lib and object C:\cpython\PCbuild\python35.exp
2>C:\cpython\PCbuild\python35.dll : fatal error LNK1169: one or more multiply defined symbols found

Still with you or have I got finger trouble? :)
msg231457 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2014-11-21 01:01
Looks like a few Windows only things were missed. Can you track them down and get them into a patch? I'm out of time to do more right now.
msg231458 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-11-21 01:05
Out of my depth I'm afraid, sorry :(
msg231509 - (view) Author: Roundup Robot (python-dev) Date: 2014-11-22 04:34
New changeset e7df0a47af16 by Steve Dower in branch 'default':
Issue #22869: Remove duplicate stack check from pythonrun.c
https://hg.python.org/cpython/rev/e7df0a47af16
msg231510 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2014-11-22 04:35
That should be the last fix for Windows - a bit of code that was copied into the new file but not removed from the old one.
msg231511 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-11-22 04:43
Steve, I was actually just minutes away from committing the same fix in the opposite direction when I got the notification of your commit.  Nick will correct me if I'm wrong but I think PyOS_CheckStack was supposed to stay in pythonrun.c, especially since it's still declared in pythonrun.h.
msg231512 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2014-11-22 05:07
You may be right, I didn't think of looking in the include files. I assumed that it was copied without being removed, rather than the copy itself being accidental. Feel free to move it back.
msg231513 - (view) Author: Roundup Robot (python-dev) Date: 2014-11-22 05:37
New changeset 406965684327 by Zachary Ware in branch 'default':
Closes #22869: Move PyOS_CheckStack back to pythonrun.c
https://hg.python.org/cpython/rev/406965684327
msg231515 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-11-22 08:28
Thanks folks - Zach's right, that wasn't supposed to move.

I missed the second copy in pylifecycle.c because it's all #ifdef'ed out on Linux, so the linker didn't complain about the duplication.
History
Date User Action Args
2014-11-22 08:28:30ncoghlansetmessages: + msg231515
2014-11-22 05:37:46python-devsetstatus: open -> closed

messages: + msg231513
2014-11-22 05:07:03steve.dowersetmessages: + msg231512
2014-11-22 04:43:51zach.waresetmessages: + msg231511
2014-11-22 04:35:25steve.dowersetmessages: + msg231510
2014-11-22 04:34:07python-devsetmessages: + msg231509
2014-11-21 01:05:05BreamoreBoysetmessages: + msg231458
2014-11-21 01:01:56steve.dowersetmessages: + msg231457
2014-11-21 00:59:01BreamoreBoysetmessages: + msg231456
2014-11-21 00:47:36steve.dowersetmessages: + msg231455
2014-11-21 00:46:44python-devsetmessages: + msg231454
2014-11-21 00:30:03pitrousetstatus: closed -> open
nosy: + tim.golden, zach.ware, steve.dower
messages: + msg231453

2014-11-21 00:16:37BreamoreBoysetnosy: + BreamoreBoy
messages: + msg231450
2014-11-20 11:46:14ncoghlansetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2014-11-20 11:40:38python-devsetnosy: + python-dev
messages: + msg231425
2014-11-16 05:57:06ncoghlansetmessages: + msg231234
2014-11-16 05:55:12ncoghlansetfiles: + split_pythonrun_v3.diff

messages: + msg231233
stage: patch review -> commit review
2014-11-16 05:52:29ncoghlansetmessages: + msg231232
2014-11-15 18:24:11pitrousetnosy: + pitrou
messages: + msg231211
2014-11-15 00:17:03Arfreversetnosy: + Arfrever
2014-11-14 14:04:37barrysetnosy: + barry
2014-11-14 11:47:19ncoghlansetmessages: + msg231158
2014-11-14 11:28:15ncoghlanlinkissue22257 dependencies
2014-11-14 11:24:20ncoghlansetfiles: + split_pythonrun_v2.diff

messages: + msg231156
2014-11-14 11:18:41ncoghlancreate