bpo-34170: Add _PyCoreConfig.isolated#8417

vstinner · 2018-07-23T16:25:48Z

_PyCoreConfig: add isolated and site_import attributes
Replace Py_IgnoreEnvironment with config->ignore_environment when
reading the current configuration
_PyCoreConfig_Read() now sets ignore_environment, utf8_mode,
isolated and site_import from Py_IgnoreEnvironment, Py_UTF8Mode,
Py_IsolatedFlag and Py_NoSiteFlag
_Py_InitializeCore() now sets Py_xxx flags from the configuration
pymain_read_conf() now uses _PyCoreConfig_Copy() to save/restore
the configuration.

https://bugs.python.org/issue34170

* _PyCoreConfig: add isolated and site_import attributes * Replace Py_IgnoreEnvironment with config->ignore_environment when reading the current configuration * _PyCoreConfig_Read() now sets ignore_environment, utf8_mode, isolated and site_import from Py_IgnoreEnvironment, Py_UTF8Mode, Py_IsolatedFlag and Py_NoSiteFlag * _Py_InitializeCore() now sets Py_xxx flags from the configuration * pymain_read_conf() now uses _PyCoreConfig_Copy() to save/restore the configuration.

vstinner · 2018-07-23T16:36:23Z

@ncoghlan, @ericsnowcurrently, @emilyemorehouse: OK, this PR should be big enhancement for the PEP 432 :-)

Previously, _PyCoreConfig "tried" to be isolated from Py_xxx global configuration variables like Py_IsolatedFlag. In practice, it already contained ignore_environment and utf8_mode which duplicated Py_IgnoreEnvironment and Py_UTF8Mode global configuration which can lead to inconsistency.

With this change, the priority becomes more explicit: _PyCoreConfig has now the priority over Py_xxx. To get a smooth transition, _PyCoreConfig are initialized to -1 which means "unset": in that case, the fields are initialized from Py_xxx.

Core config has the priority:

_PyCoreConfig config = _PyCoreConfig_INIT;
Py_UTF8Mode = 0;   /* just for the example */
config.utf8_mode = 1;  /* override Py_UTF8Mode */
Py_Initialize(&config);
# UTF-8 Mode enabled from config.utf8_mode

Backward compatibility:

_PyCoreConfig config = _PyCoreConfig_INIT;
Py_UTF8Mode = 1;
Py_Initialize(config);
# UTF-8 Mode enabled from Py_UTF8Mode

This change simplify and clarify main.c:

Py_xxx variables are no longer modified when reading the configuration. Reading the configuration must no longer modify Python global variables: only local variables like pymain, config and cmdline
Unset fields (set to -1) of config are now always initialized from Py_xxx
Python initialization now always override Py_xxx from the config <= this part is now better defined

vstinner · 2018-07-23T16:45:36Z

Include/pystate.h

    wchar_t *dll_path;      /* Windows DLL path */
 #endif

+    /* If greater than 0, enable isolated mode sys.path contains


oops, colon is missing: "enable isolated mode: sys.path contains ..."

vstinner · 2018-07-23T16:46:17Z

Include/pystate.h

+       Ignored if set to -1.
+
+       Related to Py_IsolatedFlag global configuration variable and -I command
+       line option. */


Maybe "Initialized from Py_IsolatedFlag global configuration variable and set to 1 by -I command line option"?

I updated the doc.

vstinner · 2018-07-23T16:46:34Z

Include/pystate.h

+       Ignored if set to -1.
+
+       Related to Py_NoSiteFlag global configuration variable and -S command
+       line option. */


Maybe "Initialized from Py_NoSiteFlag global configuration variable and set to 0 by -S command line option"?

I updated the doc.

vstinner · 2018-07-23T16:49:20Z

This PR also removes the ugly hack added by the commit f2626ce: two int* pointers added to _PyCoreConfig_Read().

+PyAPI_FUNC(_PyInitError) _PyCoreConfig_Read(
+    _PyCoreConfig *config,
+    int *isolated,
+    int *no_site_import);

It was a temporary hack until I make this more brave change.

emilyemorehouse · 2018-07-23T17:23:27Z

Include/pystate.h


+    /* If greater than 0, enable isolated mode sys.path contains
+       neither the script's directory nor the user's site-packages directory.
+       Ignored if set to -1.


What if it's set to 0? Is it ignored in that case as well? (the comment covers >1 and -1 but not 0)

I updated the doc.

emilyemorehouse · 2018-07-23T17:43:00Z

Python/pylifecycle.c

 {
    assert(core_config != NULL);

-    PyInterpreterState *interp;


This is just a question regarding the preferred C style for my learning -- from the code I've read, I noticed that most variable declarations are at the top of the function, but you've moved them to just before they are used (I assume for locality while reading). Is there a preference I should stick to?

CPython is 28+ years old and has been written for ISO C90. Since Python 3.7, we are allowed to move variables declaration in the middle of functions: ISO C99 is allowed!!!

I don't recall why I decided to move these variables in this PR 😁 In general, we reject PR which are only coding style changes.

* Rename _disable_importlib of _PyCoreConfig to _install_importlib * _PyCoreConfig_SetGlobalConfig() now also set Py_HashRandomizationFlag * Replace !Py_NoSiteFlag with core_config->site_import

ncoghlan

The changes themselves look good to me (with one minor fix to a comment), but it would be good to add a test case that ensures the settings in the config struct override the preconfigured ones in the environment (or not, as the case may be).

My suggestion would be to have a test embed helper command that runs 3 cases:

global variables set to 1, core config fields set to -1 -> sys.flags reports 1
global variables set to 1, core config fields set to 0 -> sys.flags reports 0
global variables set to 0, core config fields set to 1 -> sys.flags reports 1

The embedding helper would just run import sys; print(sys.flags); sys.stdout.flush(), and then the Python test case would handle checking the output flags matched what the test excepted for each case.

ncoghlan · 2018-07-24T10:54:08Z

Include/pystate.h

+       site.main() if you want them to be triggered).
+
+       Set to 0 by the -S command line option. If set to -1 (default), set to
+       the negative value of Py_NoSiteFlag. */


This comment isn't right, as the field gets set to the result of !Py_NoSiteFlag

I wrote "negative value of !Py_NoSiteFlag". Is that wrong?

"negative value" is ambiguous here because we're dealing with C integers rather than strict booleans, and arithmetic negation (-value) and logical negation (!value) do different things. Since this is C code, and the English spelling is ambiguous, just including the exact C expression in the comment is clearer than trying to express the operation as words.

vstinner · 2018-07-24T11:54:04Z

Write more tests is a good idea! But this change is already big enough, I will write them in my following change. I plan to move all "cmdline" fields into the core config.

vstinner requested a review from a team as a code owner July 23, 2018 16:25

the-knights-who-say-ni added the CLA signed label Jul 23, 2018

bedevere-bot added the awaiting merge label Jul 23, 2018

vstinner requested review from ericsnowcurrently and ncoghlan July 23, 2018 16:26

vstinner added the skip news label Jul 23, 2018

vstinner commented Jul 23, 2018

View reviewed changes

emilyemorehouse reviewed Jul 23, 2018

View reviewed changes

Adjust _PyCoreConfig documentation

497bc77

* Rename _disable_importlib of _PyCoreConfig to _install_importlib * _PyCoreConfig_SetGlobalConfig() now also set Py_HashRandomizationFlag * Replace !Py_NoSiteFlag with core_config->site_import

ncoghlan reviewed Jul 24, 2018

View reviewed changes

vstinner merged commit d19d8d5 into python:master Jul 24, 2018

bedevere-bot removed the awaiting merge label Jul 24, 2018

vstinner deleted the isolated branch July 24, 2018 12:50

Uh oh!

Conversation

vstinner commented Jul 23, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jul 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jul 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vstinner commented Jul 23, 2018 •

edited by bedevere-bot

Loading

vstinner commented Jul 23, 2018 •

edited

Loading