Skip to content

Conversation

@ronaldoussoren
Copy link
Contributor

@ronaldoussoren ronaldoussoren commented Jul 28, 2018

As mentioned in bpo-34247 a program embedding Python with Py_Initialize / Py_Run... (no Py_Main) cannot affect a number of interpreter options through environment variables.

This is a pull request against the 3.7 branch, primarily because Nick Coghlan mentioned that there is related work on the master branch by Victor Stinner (bpo-34170).

The patch works for me, but I haven't dug deeply enough in the code to be sure that this is the right patch, I'm particularly unsure about the additional argument to _PyCoreConfig_Read.

https://bugs.python.org/issue34247

…sys.flags

This patch moves a number of startup options from the
_Py_CommandLineDetails to _PyCoreConfig to make it possible to process
them in Py_Initialize, actually uses this and adds a function to update
the relevant global variables from _Py_Initialize.
_PyCoreConfig config = _PyCoreConfig_INIT;

err = _PyCoreConfig_Read(&config);
err = _PyCoreConfig_Read(&config, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about this one, this is the minimal change to make this code compile with my other changes and without any changes to its behaviour. I haven't check the code yet to see if the changed behaviour you get with "0" as the second argument would be more useful here.


static _PyInitError
config_read_env_vars(_PyCoreConfig *config)
config_read_env_vars(_PyCoreConfig *config, int ignore_cmdline)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the ignore_cmdline to be 100% sure that I don't change the behaviour of Py_Main with this patch, and haven't checked yet if this is really necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Provided we are handling the -I option correctly and are also not looking at argv unless using Py_Main, it shouldn't be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've talked with Victor about this issue (without looking at this code in detail), the master branch contains a much better implementation of this included much improved unit tests.

I won't update this pull request until he has had time to think about this issue.

@vstinner
Copy link
Member

vstinner commented Aug 3, 2018

I proposed a different approach: PR #8659.

@vstinner
Copy link
Member

vstinner commented Aug 5, 2018

I merged my PR #8659 instead which is more complete and contains unit tests. Thanks for your PR anyway!

@vstinner vstinner closed this Aug 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants