Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split pylifecycle.c out from pythonrun.c #67058

Closed
ncoghlan opened this issue Nov 14, 2014 · 21 comments
Closed

Split pylifecycle.c out from pythonrun.c #67058

ncoghlan opened this issue Nov 14, 2014 · 21 comments
Assignees
Labels
type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

BPO 22869
Nosy @warsaw, @ncoghlan, @pitrou, @tjguk, @zware, @zooba
Files
  • split_pythonrun.diff: WIP: split pythonrun into two separate modules
  • split_pythonrun_v2.diff: Incorporated recent changes to pythonrun
  • split_pythonrun_v3.diff: Adjusted patch as per comments on issue
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ncoghlan'
    closed_at = <Date 2014-11-22.05:37:46.372>
    created_at = <Date 2014-11-14.11:18:41.869>
    labels = ['type-feature']
    title = 'Split pylifecycle.c out from pythonrun.c'
    updated_at = <Date 2014-11-22.08:28:30.969>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2014-11-22.08:28:30.969>
    actor = 'ncoghlan'
    assignee = 'ncoghlan'
    closed = True
    closed_date = <Date 2014-11-22.05:37:46.372>
    closer = 'python-dev'
    components = []
    creation = <Date 2014-11-14.11:18:41.869>
    creator = 'ncoghlan'
    dependencies = []
    files = ['37194', '37195', '37204']
    hgrepos = []
    issue_num = 22869
    keywords = ['patch']
    message_count = 21.0
    messages = ['231155', '231156', '231158', '231211', '231232', '231233', '231234', '231425', '231450', '231453', '231454', '231455', '231456', '231457', '231458', '231509', '231510', '231511', '231512', '231513', '231515']
    nosy_count = 9.0
    nosy_names = ['barry', 'ncoghlan', 'pitrou', 'tim.golden', 'Arfrever', 'BreamoreBoy', 'python-dev', 'zach.ware', 'steve.dower']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue22869'
    versions = ['Python 3.5']

    @ncoghlan
    Copy link
    Contributor Author

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

    @ncoghlan ncoghlan self-assigned this Nov 14, 2014
    @ncoghlan ncoghlan added the type-feature A feature request or enhancement label Nov 14, 2014
    @ncoghlan
    Copy link
    Contributor Author

    Incorporated pythonrun changes from 668e0bf30042, 6aaa0aab1e93 and d6fb87972dee into pylifecycle

    @ncoghlan
    Copy link
    Contributor Author

    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

    @pitrou
    Copy link
    Member

    pitrou commented Nov 15, 2014

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

    @ncoghlan
    Copy link
    Contributor Author

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

    @ncoghlan
    Copy link
    Contributor Author

    I think this is ready to go now, unless anyone spots any major flaws I missed.

    @ncoghlan
    Copy link
    Contributor Author

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 20, 2014

    New changeset b9775a92c1d0 by Nick Coghlan in branch 'default':
    Issue bpo-22869: Split pythonrun into two modules
    https://hg.python.org/cpython/rev/b9775a92c1d0

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Nov 21, 2014

    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

    @pitrou
    Copy link
    Member

    pitrou commented Nov 21, 2014

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

    @pitrou pitrou reopened this Nov 21, 2014
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 21, 2014

    New changeset 31fd106bb68a by Steve Dower in branch 'default':
    Issue bpo-22869: Add pylifecycle.c/.h files to pythoncore project.
    https://hg.python.org/cpython/rev/31fd106bb68a

    @zooba
    Copy link
    Member

    zooba commented Nov 21, 2014

    Fixed. (I was conveniently sitting waiting for other things to build...)

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Nov 21, 2014

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

    @zooba
    Copy link
    Member

    zooba commented Nov 21, 2014

    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.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Nov 21, 2014

    Out of my depth I'm afraid, sorry :(

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 22, 2014

    New changeset e7df0a47af16 by Steve Dower in branch 'default':
    Issue bpo-22869: Remove duplicate stack check from pythonrun.c
    https://hg.python.org/cpython/rev/e7df0a47af16

    @zooba
    Copy link
    Member

    zooba commented Nov 22, 2014

    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.

    @zware
    Copy link
    Member

    zware commented Nov 22, 2014

    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.

    @zooba
    Copy link
    Member

    zooba commented Nov 22, 2014

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 22, 2014

    New changeset 406965684327 by Zachary Ware in branch 'default':
    Closes bpo-22869: Move PyOS_CheckStack back to pythonrun.c
    https://hg.python.org/cpython/rev/406965684327

    @python-dev python-dev mannequin closed this as completed Nov 22, 2014
    @ncoghlan
    Copy link
    Contributor Author

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants