◐ Shell
reader mode source ↗
Skip to content

bpo-32030: Split Py_Main() into subfunctions#4399

Merged
vstinner merged 3 commits into
python:masterfrom
vstinner:cmdline
Nov 15, 2017
Merged

bpo-32030: Split Py_Main() into subfunctions#4399
vstinner merged 3 commits into
python:masterfrom
vstinner:cmdline

Conversation

@vstinner

@vstinner vstinner commented Nov 15, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member
  • Don't use "Python runtime" anymore to parse command line options or
    to get environment variables: pymain_init() is now a strict
    separation.
  • Use an error message rather than "crashing" directly with
    Py_FatalError(). Limit the number of calls to Py_FatalError(). It
    prepares the code to handle errors more nicely later.
  • Warnings options (-W, PYTHONWARNINGS) and "XOptions" (-X) are now
    only added to the sys module once Python core is properly
    initialized.
  • _PyMain is now the well identified owner of some important strings
    like: warnings options, XOptions, and the "program name". The
    program name string is now properly freed at exit.
    pymain_free() is now responsible to free the "command" string.
  • Rename most methods in Modules/main.c to use a "pymain_" prefix to
    avoid conflits and ease debug.
  • Replace _Py_CommandLineDetails_INIT with memset(0)
  • Reorder a lot of code to fix the initialization ordering. For
    example, initializing standard streams now comes before parsing
    PYTHONWARNINGS.
  • pymain_init() now uses the cmdline structure rather than relying on
    Py_xxx global configuration variables. For example, replace
    Py_UnbufferedStdioFlag with cmdline.use_unbuffered_io.
  • Py_Main() now handles errors when adding warnings options and
    XOptions.
  • Add _PyMem_GetDefaultRawAllocator() private function.

https://bugs.python.org/issue32030

@vstinner vstinner requested a review from a team November 15, 2017 01:30
@vstinner

vstinner commented Nov 15, 2017

Copy link
Copy Markdown
Member Author

Hum, test_capi fails on Windows.

@vstinner

vstinner commented Nov 15, 2017

Copy link
Copy Markdown
Member Author

test_capi failed on the "gcc C" job of Travis CI:

0:01:20 load avg: 44.72 [ 46/404] test_capi
*** Error in `/home/travis/build/python/cpython/venv/bin/python': munmap_chunk(): invalid pointer: 0x00007f0f5c5e90a0 ***
*** Error in `/home/travis/build/python/cpython/venv/bin/python': munmap_chunk(): invalid pointer: 0x00007f26c992e0a0 ***
*** Error in `/home/travis/build/python/cpython/venv/bin/python': munmap_chunk(): invalid pointer: 0x00007fab599500a0 ***
test test_capi failed -- multiple errors occurred; run in verbose mode for details

ncoghlan
ncoghlan reviewed Nov 15, 2017
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Nice! This looks really good to me - just a couple of minor suggestions inline.

@ncoghlan

ncoghlan commented Nov 15, 2017

Copy link
Copy Markdown
Contributor

One general comment: while I didn't spot any particular instances of this going wrong, it's important to keep in mind that the various global flags (Py_IsolatedFlag, etc) are part of the embedding configuration API. This means that even as we add the more explicit configuration structs and use those to pass settings from Py_Main into the CPython runtime, we still need to account for other applications that are passing in the settings via the global variables instead (and either keep that model working, or else provide explicit porting guidance).

@vstinner

vstinner commented Nov 15, 2017

Copy link
Copy Markdown
Member Author

The PR compiles correctly on macOS and the following tests passed:

macbook:master haypo$ PYTHONMALLOC=pymalloc ./python.exe -m test -j0 test_capi test_cmd_line test_cmd_line_script
Run tests in parallel using 6 child processes
0:00:08 load avg: 2.30 [1/3] test_cmd_line passed
0:00:11 load avg: 2.35 [2/3] test_cmd_line_script passed
0:00:17 load avg: 2.49 [3/3] test_capi passed
All 3 tests OK.

Total duration: 17 sec
Tests result: SUCCESS

@vstinner
bpo-32030: Split Py_Main() into subfunctions …
91fa799 
* Don't use "Python runtime" anymore to parse command line options or
  to get environment variables: pymain_init() is now a strict
  separation.
* Use an error message rather than "crashing" directly with
  Py_FatalError(). Limit the number of calls to Py_FatalError(). It
  prepares the code to handle errors more nicely later.
* Warnings options (-W, PYTHONWARNINGS) and "XOptions" (-X) are now
  only added to the sys module once Python core is properly
  initialized.
* _PyMain is now the well identified owner of some important strings
  like: warnings options, XOptions, and the "program name". The
  program name string is now properly freed at exit.
  pymain_free() is now responsible to free the "command" string.
* Rename most methods in Modules/main.c to use a "pymain_" prefix to
  avoid conflits and ease debug.
* Replace _Py_CommandLineDetails_INIT with memset(0)
* Reorder a lot of code to fix the initialization ordering. For
  example, initializing standard streams now comes before parsing
  PYTHONWARNINGS.
* Py_Main() now handles errors when adding warnings options and
  XOptions.
* Add _PyMem_GetDefaultRawAllocator() private function.
* Cleanup _PyMem_Initialize(): remove useless global constants: move
  them into _PyMem_Initialize().
* Call _PyRuntime_Initialize() as soon as possible:
  _PyRuntime_Initialize() now returns an error message on failure.
* Add _PyInitError structure and following macros:

  * _Py_INIT_OK()
  * _Py_INIT_ERR(msg)
  * _Py_INIT_USER_ERR(msg): "user" error, don't abort() in that case
  * _Py_INIT_FAILED(err)
@vstinner

vstinner commented Nov 15, 2017

Copy link
Copy Markdown
Member Author

One general comment: while I didn't spot any particular instances of this going wrong, it's important to keep in mind that the various global flags (Py_IsolatedFlag, etc) are part of the embedding configuration API. (...)

Ok, I changed my code to set Py_xxx flags as soon as possible, and use Py_xxx flags rather than using pymain.

@vstinner

vstinner commented Nov 15, 2017

Copy link
Copy Markdown
Member Author

Hum, test_capi fails on Windows.

and

test_capi failed on the "gcc C" job of Travis CI: (...

Oh, I expect PyMem_RawMalloc() to be usable directly, but this changed recently: _PyRuntime_Initialize() must now be called before the first call to PyMem_RawMalloc(). I fixed that as well.

@vstinner
Fix _Py_InitializeCore() …
02282e5 
Call _PyRuntime_Initialize() and copy core config before calling
_PyMem_SetupAllocators().
@vstinner
python.vcxproj: define Py_BUILD_CORE …
10a6e78 
Programs/python.c now uses internals/pystate.h which requires
Py_BUILD_CORE.
@vstinner vstinner requested a review from a team as a code owner November 15, 2017 21:22
@zooba

zooba commented Nov 15, 2017

Copy link
Copy Markdown
Member

Haven't looked closely, but this looks like overall goodness!

If you want one extra case to keep in mind, see how I implemented my spython.c for my PEP 551 implementation. There's obviously no compatibility requirement, but I'd want to be able to keep doing the same things after this change.

@vstinner vstinner merged commit f7e5b56 into python:master Nov 15, 2017
@vstinner vstinner deleted the cmdline branch November 15, 2017 23:48
@vstinner vstinner changed the title [WIP] bpo-32030: Split Py_Main() into subfunctions Nov 15, 2017
@vstinner

vstinner commented Nov 16, 2017

Copy link
Copy Markdown
Member Author

@zooba: "Haven't looked closely, but this looks like overall goodness!"

Oh thanks. With @ncoghlan and you who know the topic, I'm now confident enough to merge my change (I already merged it ;-)).

"If you want one extra case to keep in mind, see how I implemented my spython.c for my PEP 551 implementation. There's obviously no compatibility requirement, but I'd want to be able to keep doing the same things after this change."

My change shouldn't break this code. @ncoghlan: would you mind to double-check?

In the first version of this change, I overrided Py_xxx flags from the command lines, ignoring their previous value. @ncoghlan warned me about Py_xxx flags set when Python is embedded. So I fixed my code.

jimmylai added a commit to jimmylai/cpython that referenced this pull request Nov 17, 2017
@jimmylai
Merge branch 'master' of https://github.com/python/cpython …
d914928 
* 'master' of https://github.com/python/cpython: (550 commits)
  bpo-31867: Remove duplicates in default mimetypes. (python#4388)
  tokenizer: Remove unused tabs options (python#4422)
  bpo-31691: Specify where to find build instructions for the Windows installer (python#4426)
  Fix typo in atexit documentation. (pythonGH-4419)
  bpo-31702: Allow to specify rounds for SHA-2 hashing in crypt.mksalt(). (python#4110)
  bpo-32043: New "developer mode": "-X dev" option (python#4413)
  bpo-30349: Raise FutureWarning for nested sets and set operations (python#1553)
  bpo-32037: Use the INT opcode for 32-bit integers in protocol 0 pickles. (python#4407)
  bpo-30143: 2to3 now generates a code that uses abstract collection classes (python#1262)
  bpo-32030: Enhance Py_Main() (python#4412)
  bpo-32030: Split Py_Main() into subfunctions (python#4399)
  bpo-32034: Make IncompleteReadError & LimitOverrunError pickleable python#4409
  bpo-32025: Add time.thread_time() (python#4410)
  bpo-32018: Fix inspect.signature repr to follow PEP 8 (python#4408)
  bpo-30399: Get rid of trailing comma in the repr of BaseException. (python#1650)
  bpo-30950: Convert round() to Argument Clinic. (python#2740)
  bpo-32011: Revert "Issue python#15480: Remove the deprecated and unused TYPE_INT64 code from marshal." (python#4381)
  bpo-32023: Disallow genexprs without parenthesis in class definitions. (python#4400)
  bpo-31949: Fixed several issues in printing tracebacks (PyTraceBack_Print()). (python#4289)
  bpo-32032: Test both implementations of module-level pickle API. (python#4401)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ncoghlan ncoghlan ncoghlan left review comments

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vstinner @ncoghlan @zooba @the-knights-who-say-ni @bedevere-bot