bpo-35239: _PySys_EndInit() copies module_search_path #10532
Conversation
* The _PySys_EndInit() function now copies the config->module_search_path list, so config is longer modified when sys.path is updated. * config->warnoptions list and config->xoptions dict are also copied * test_embed: InitConfigTests now also tests main_config['module_search_path']
Programs/_testembed.c
Outdated
| "print('sys.warnoptions:', sys.warnoptions); " | ||
| "print('sys._xoptions:', sys._xoptions); " | ||
| "warnings = sys.modules['warnings']; " |
There was a problem hiding this comment.
Oh, this change is a bug related to _PySys_ReadPreInitOptions().
There was a problem hiding this comment.
_PySys_EndInit() calls _PySys_ReadPreInitOptions() to read options added by PySys_AddWarnOption() and PySys_AddXOption(). Problem: with this change, _PySys_ReadPreInitOptions() options are no longer added to interp->config.warnoptions, whereas _Py_InitializeMainInterpreter() checks interp->config.warnoptions is a non-empty list to decide if the "warnings" modules must be initialized.
There was a problem hiding this comment.
Ah, fixed by my second commit.
Don't use config->warnoptions but sys.warnoptions to decide if the warnings module should be imported at startup.
|
The test suite succeded on Travis CI, but the job failed because of https://bugs.python.org/issue35240 bug. |
|
@ncoghlan, @ericsnowcurrently: Would you mind to review this PR? It is now ready for a review :-) See https://bugs.python.org/issue35239 for the rationale. |
* test main_config['module_search_path'] * unset COPY_LIST
I became convincing that separating the "initial configuration" (ex: core config) from the "runtime configuration" (ex: sys.path) is the way to go. It might require minor change in the code to read the "runtime configuration" rather than the "initial configuration", but I'm only aware of a single issue: _Py_InitializeMainInterpreter() which used interp->config.warnoptions rather than PySys_GetObject("warnoptions"), and my PR fix it. So I plan to merge this PR next week if I don't hear anything from you in the meanwhile. |
| @@ -346,7 +342,8 @@ class InitConfigTests(EmbeddingTestsMixin, unittest.TestCase): | |||
| 'prefix', | |||
| 'pycache_prefix', | |||
| 'warnoptions', | |||
There was a problem hiding this comment.
You switch warnoptions over to using the new COPY_LIST macro below, as well as updating it to be read from the sys module when needed. Did you mean to also remove it from this list?
There was a problem hiding this comment.
And that's what I get for reviewing the code before reading the author's initial comments on the review - you already answered that question (the warnoptions here are the warning options as specified in the initial configuration, while the "live" state is always the copy in the sys module).
There was a problem hiding this comment.
warnoptions comes from DEFAULT_CORE_CONFIG: warnoptions=[]. Can it can be replaced in other tests like: warnoptions=['default', 'error::ResourceWarning']. COPY_MAIN_CONFIG ensures that core and main configuration contain exactly the same value, it's just to not repeat the expected value twice.
Spoiler(!): I'm now experimenting a change to avoid redundancy between core and main configuration! Stay tuned.
|
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
* The _PySys_EndInit() function now copies the config->module_search_path list, so config is longer modified when sys.path is updated. * config->warnoptions list and config->xoptions dict are also copied * test_embed: InitConfigTests now also tests main_config['module_search_path'] * Fix _Py_InitializeMainInterpreter(): don't use config->warnoptions but sys.warnoptions to decide if the warnings module should be imported at startup. (cherry picked from commit 37cd982) Co-authored-by: Victor Stinner <vstinner@redhat.com>
|
GH-10566 is a backport of this pull request to the 3.7 branch. |
|
Thanks for the review @ncoghlan, I wasn't 100% sure if it's a good thing or not :-) |
* The _PySys_EndInit() function now copies the config->module_search_path list, so config is longer modified when sys.path is updated. * config->warnoptions list and config->xoptions dict are also copied * test_embed: InitConfigTests now also tests main_config['module_search_path'] * Fix _Py_InitializeMainInterpreter(): don't use config->warnoptions but sys.warnoptions to decide if the warnings module should be imported at startup. (cherry picked from commit 37cd982) Co-authored-by: Victor Stinner <vstinner@redhat.com>
|
@vstinner The potential benefit I see in separating them is that it should eventually allow us to offer the ability to reset the warnings options back to the way they were when the process started, similar to sys.stdout and friends. I'm less sure about backporting the change to 3.7, as I think it may introduce some subtle differences in the way subinterpreters get initialised (as they populate their sys module from the config settings, not from the main interpreter's sys module). |
My main motivation to backport changes is to ease future backports. The Python initialization code was half baked in Python 3.7.0, and I had to fix many bugs in 3.7.1 :-( sys.path is mostly modified by the site module, but subinterpreters also import site (again). So it should be fine, no? If the change is not fine for 3.7, maybe it's not fine for master neither? |
|
I think the current initialisation semantics of subinterpreters are ill-defined enough that the change is definitely OK for 3.8 (the most we'd need there is a note in the version porting guide). The case I'm concerned about for 3.7.2 is just that this is different from what 3.7.1 did, and for maintenance release, we tend to let ill-defined semantics be dictated by whatever the previous maintenance release in that series did. I don't feel too strongly about this particular instance though (since subinterpreters are an obscure feature to start with, and modifying sys.path outside site before creating them is even more obscure), and I agree it's desirable to keep backports simple when feasible to do so. |
|
Ok, I wrote PR #10660 to revert the _PySys_EndInit() change in 3.7. |
config->module_search_path list, so config is longer modified when
sys.path is updated.
main_config['module_search_path']
https://bugs.python.org/issue35239