◐ Shell
reader mode source ↗
Skip to content

[3.7] bpo-34247: Fix Python 3.7 initialization#8659

Merged
vstinner merged 5 commits into
python:3.7from
vstinner:fix_config
Aug 5, 2018
Merged

[3.7] bpo-34247: Fix Python 3.7 initialization#8659
vstinner merged 5 commits into
python:3.7from
vstinner:fix_config

Conversation

@vstinner

@vstinner vstinner commented Aug 3, 2018 •
edited by ncoghlan
Loading

Copy link
Copy Markdown
Member
  • -X dev: it is now possible to override the memory allocator using
    PYTHONMALLOC even if the developer mode is enabled.
  • Add _Py_InitializeFromConfig()
  • Add _Py_Initialize_ReadEnvVars() to set global configuration
    variables from environment variables
  • Fix the code to initialize Python: Py_Initialize() now also reads
    environment variables
  • _Py_InitializeCore() can now be called repeatedly: the second
    and subsequent calls only replace the configuration, they don't
    recreate the main interpreter
  • Write unit tests on Py_Initialize() and the different ways to
    configure Python
  • The isolated mode now always sets Py_IgnoreEnvironmentFlag and
    Py_NoUserSiteDirectory to 1.
  • pymain_read_conf() now saves/restores the configuration
    if the encoding changed

https://bugs.python.org/issue34247

@vstinner
bpo-34247: Fix Python initialization …
463792e 
* -X dev: it is now possible to override the memory allocator using
  PYTHONMALLOC even if the developer mode is enabled.
* Add _Py_InitializeFromConfig()
* Add _Py_Initialize_ReadEnvVars() to set global configuration
  variables from environment variables
* Fix the code to initialize Python: Py_Initialize() now also reads
  environment variables
* _Py_InitializeCore() can now be called twice: the second call
  only replaces the configuration.
* Write unit tests on Py_Initialize() and the different ways to
  configure Python
* The isolated mode now always sets Py_IgnoreEnvironmentFlag and
  Py_NoUserSiteDirectory to 1.
* pymain_read_conf() now saves/restores the configuration
  if the encoding changed
@vstinner vstinner changed the title bpo-34247: Fix Python initialization Aug 3, 2018
@vstinner vstinner mentioned this pull request Aug 3, 2018
Closed
@vstinner vstinner requested a review from ronaldoussoren August 3, 2018 20:28
vstinner added 3 commits August 3, 2018 22:32
@vstinner
Fix compilation on Windows …
249aba7 
Restore legacy_windows_fs_encoding change.
@vstinner
Cannot test _Py_CheckHashBasedPycsMode …
1533c6e 
The symbol is not exported.
@vstinner
Fix _freeze_importlib.c …
e219dca 
Replace _Py_InitializeEx_Private() with _Py_InitializeFromConfig().

The _Py_InitializeEx_Private() function has been removed.
@vstinner vstinner requested a review from a team August 3, 2018 22:02
@vstinner vstinner changed the title [3.7] bpo-34247: Fix Python initialization Aug 3, 2018
@vstinner

vstinner commented Aug 3, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member Author

bedevere/backport-pr — Not a valid backport PR title.

Stupid bot. It doesn't like "[3.7] bpo-34247: Fix Python initialization" nor "bpo-34247: Fix Python 3.7 initialization". Hopefully, it doesn't prevent to merge the PR :-)

@vstinner

vstinner commented Aug 3, 2018

Copy link
Copy Markdown
Member Author

@ronaldoussoren: I will merge this change in two weeks if I don't hear anything from you, since I know feel guilty of having introducing regressions in Python 3.7. This change adds new tests which is something very new for this part of the C API. The tests make me confident in my change :-) They helped me to fix bugs in this PR :-)

@vstinner

vstinner commented Aug 3, 2018

Copy link
Copy Markdown
Member Author

This PR backports the best enhancements from the master branch without backporting everything, since I made too many changes in the master. And it seems that master is going to get much more changes, according to the PEP 432 discussion on python-dev. So I prefer to implement the minimum changes for 3.7 just to handle environment variables and to have working unit tests.

@ncoghlan ncoghlan changed the title bpo-34247: Fix Python 3.7 initialization Aug 4, 2018
@ncoghlan

ncoghlan commented Aug 4, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Seeing if I can make the backport-pr bot happy by updating the PR title to refer to itself :)

(Update: I don't think the title edit triggered any of the bots to re-run, not even the backport-pr bot)

@ncoghlan ncoghlan mentioned this pull request Aug 4, 2018
Closed
ncoghlan
ncoghlan reviewed Aug 4, 2018
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

This is looking pretty good to me - just a few readability/debuggability comments inline.

@ncoghlan

ncoghlan commented Aug 4, 2018

Copy link
Copy Markdown
Contributor

After reviewing the change, I slightly reworded the section in the commit message about _Py_InitializeCore

@Mariatta

Mariatta commented Aug 4, 2018

Copy link
Copy Markdown
Member

bedevere/backport-pr status is not required and does not block merging. You'll notice the "Squash and merge" button is still enabled.

@Mariatta

Mariatta commented Aug 4, 2018

Copy link
Copy Markdown
Member

And the PR needs to start with [3.7].

@ncoghlan ncoghlan changed the title bpo-34247: Fix Python 3.7 initialization (GH-8659) Aug 4, 2018
@ncoghlan

ncoghlan commented Aug 4, 2018

Copy link
Copy Markdown
Contributor

Ah, cool - that means the self-reference is a valid workaround, I just missed that the prefix was missing as well.

@Mariatta Mariatta changed the title [3.7] bpo-34247: Fix Python 3.7 initialization (GH-8659) Aug 4, 2018
@Mariatta

Mariatta commented Aug 4, 2018

Copy link
Copy Markdown
Member

I've deployed the latest change in bedevere, so now it is only looking for the [X.Y] prefix, and it does not care about the (GH-NNNN) anymore.
But this status is now "Required".

Example:
screen shot 2018-08-04 at 8 08 33 am

@vstinner
Add NEWS entry for bpo-34247 fix …
6c3b5ec 
* Remove a redundant #ifndef Py_LIMITED_API in pylifecycle.h.
* Rename _Py_Initialize_ReadEnvVars() to
  _Py_Initialize_ReadEnvVarsNoAlloc().
* Change _Py_InitializeCore_impl() error message
vstinner
vstinner commented Aug 4, 2018
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hide comment

I addressed all @ncoghlan's comment in my new commit 6c3b5ec.

ncoghlan
ncoghlan approved these changes Aug 5, 2018
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

+1 for going ahead with this version.

We may want to tweak the porting note in What's New, as I believe this may read more environment variables at init time than the interpreter did previously, so embedding applications that want more complete control may now need to set Py_IgnoreEnvironmentFlag when they could previously get by without it.

@vstinner vstinner merged commit 0c90d6f into python:3.7 Aug 5, 2018
@bedevere-bot

bedevere-bot commented Aug 5, 2018

Copy link
Copy Markdown

@vstinner: Please replace # with GH- in the commit message next time. Thanks!

@vstinner vstinner deleted the fix_config branch August 5, 2018 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matrixise matrixise matrixise left review comments

@ncoghlan ncoghlan ncoghlan approved these changes

@ronaldoussoren ronaldoussoren

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vstinner @ncoghlan @Mariatta @bedevere-bot @matrixise @the-knights-who-say-ni