Skip to content

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

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

Merged
merged 5 commits into from
Aug 5, 2018
Merged

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

merged 5 commits into from
Aug 5, 2018

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Aug 3, 2018

  • -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

* -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 [3.7] bpo-34247: Fix Python initialization Aug 3, 2018
@vstinner vstinner requested a review from ronaldoussoren August 3, 2018 20:28
Restore legacy_windows_fs_encoding change.
The symbol is not exported.
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 bpo-34247: Fix Python 3.7 initialization Aug 3, 2018
@vstinner
Copy link
Member Author

vstinner commented Aug 3, 2018

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
Copy link
Member Author

vstinner commented Aug 3, 2018

@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
Copy link
Member Author

vstinner commented Aug 3, 2018

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 bpo-34247: Fix Python 3.7 initialization (GH-8659) Aug 4, 2018
@ncoghlan
Copy link
Contributor

ncoghlan commented Aug 4, 2018

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)

Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

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

PyAPI_FUNC(int) _Py_IsCoreInitialized(void);
#ifndef Py_LIMITED_API
Copy link
Contributor

Choose a reason for hiding this comment

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

We're already inside a Py_LIMITED_API guard here, so this isn't needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1,2 @@
-X dev: it is now possible to override the memory allocator using
PYTHONMALLOC even if the developer mode is enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need a NEWS entry for the bpo-34247 fix

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Modules/main.c Outdated
@@ -1714,6 +1731,27 @@ cmdline_get_env_flags(_Py_CommandLineDetails *cmdline)
}


/* Set global variable variables from environment variables */
void
_Py_Initialize_ReadEnvVars(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

Something like _Py_Initialize_ReadEnvVarsNoAlloc might be a better name here, as this doesn't read all the env vars, only the ones that it can process without needing to allocate any memory to store the results.

With the current name, it gives the impression it reads all the environment variables, when it actually skips over the more complex ones like PYTHONPATH.

Copy link
Member Author

Choose a reason for hiding this comment

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

function renamed


interp = tstate->interp;
if (interp == NULL) {
return _Py_INIT_ERR("can't make main interpreter");
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message isn't right for the reconfiguration case. Perhaps something like "Core reconfiguration: no main interpreter found".

Then the thread state lookup above could also gain the "Core reconfiguration: " prefix.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the error message to "no main interpreter found". The displayed error message contains the name of the current function _Py_InitializeCore_impl() thanks to _Py_INIT_ERR() magic. I don't want to add a prefix per error message. Such error should not occur in practice anyway.

@ncoghlan
Copy link
Contributor

ncoghlan commented Aug 4, 2018

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


def check_config(self, testname, expected):
env = dict(os.environ)
for key in list(env):
Copy link
Member

Choose a reason for hiding this comment

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

I am curious, why do you need to convert the dict to a list? usually, you will iterate over the keys, is there a subtlety?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the loop body modifies the env dict.

@Mariatta
Copy link
Member

Mariatta commented Aug 4, 2018

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

@Mariatta
Copy link
Member

Mariatta commented Aug 4, 2018

And the PR needs to start with [3.7].

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

ncoghlan commented Aug 4, 2018

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) [3.7] bpo-34247: Fix Python 3.7 initialization Aug 4, 2018
@Mariatta
Copy link
Member

Mariatta commented Aug 4, 2018

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

* Remove a redundant #ifndef Py_LIMITED_API in pylifecycle.h.
* Rename _Py_Initialize_ReadEnvVars() to
  _Py_Initialize_ReadEnvVarsNoAlloc().
* Change _Py_InitializeCore_impl() error message
Copy link
Member Author

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

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

PyAPI_FUNC(int) _Py_IsCoreInitialized(void);
#ifndef Py_LIMITED_API
Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1,2 @@
-X dev: it is now possible to override the memory allocator using
PYTHONMALLOC even if the developer mode is enabled.
Copy link
Member Author

Choose a reason for hiding this comment

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

done

Modules/main.c Outdated
@@ -1714,6 +1731,27 @@ cmdline_get_env_flags(_Py_CommandLineDetails *cmdline)
}


/* Set global variable variables from environment variables */
void
_Py_Initialize_ReadEnvVars(void)
Copy link
Member Author

Choose a reason for hiding this comment

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

function renamed


interp = tstate->interp;
if (interp == NULL) {
return _Py_INIT_ERR("can't make main interpreter");
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the error message to "no main interpreter found". The displayed error message contains the name of the current function _Py_InitializeCore_impl() thanks to _Py_INIT_ERR() magic. I don't want to add a prefix per error message. Such error should not occur in practice anyway.

Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

+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
Copy link

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants