bpo-34170: Rework _PyCoreConfig_Read() to avoid side effect#8353
bpo-34170: Rework _PyCoreConfig_Read() to avoid side effect#8353vstinner merged 6 commits intopython:masterfrom vstinner:pathconfig_setglobal
Conversation
Rework _PyCoreConfig_Read() function which *reads* core configuration to not *modify* the path configuration. A new _PyCoreConfig_SetPathConfig() function now recreates the path configuration from the core configuration. This function is now called very late in _Py_InitializeCore(), just before calling initimport(). Changes: * Add _PyCoreConfig.dll_path * Rename _PyPathConfig_Calculate() to _PyPathConfig_Calculate_impl() * Replace _PyPathConfig_Init() with _PyPathConfig_Calculate(): the function now requires a _PyPathConfig * Add _PyPathConfig_SetGlobal() to set the _Py_path_config global variable. * Add _PyCoreConfig_InitPathConfig(): compute the path configuration * Add _PyCoreConfig_SetPathConfig(): set path configuration from core configuration * Rename wstrlist_append() to _Py_wstrlist_append()
|
I don't think that "Add _PyCoreConfig.dll_path" needs a public changelog entry, since the structure is still private. |
ncoghlan
left a comment
ncoghlan
left a comment
There was a problem hiding this comment.
+1 for avoiding side effects when reading the configuration, and the general structure here looks good to me (I didn't look closely at the internals of the relocated functions, though)
Python/pathconfig.c
Outdated
| } | ||
|
|
||
| if (config->executable == NULL) { | ||
| config->executable = _PyMem_RawWcsdup(path_config.program_full_path); |
There was a problem hiding this comment.
Hum, maybe copy_wstr() should be used. I'm not sure if these fields can be NULL or not.
|
While I can't think of a way this could cause an externally visible change in behaviour, if you wanted to backport this to Python 3.7 to keep the code structure consistent, then I think it would need a public change log entry so it appeared in the 3.7.1 notes. If it remains 3.8 only though, then yeah, I agree it would be covered by the eventual change of making the new initialisation API public. |
* wstrlist_join(): fix off-by-one error * Don't call _PyCoreConfig_SetPathConfig() if importlib is disabled
Py_SetPath() now fails with a fatal python error on memory allocation failure.
|
I see this change as an enhancement rather than a bugfix. The change is large, I would be risky to backport it. I don't want to backport this change, I wrote it for the master branch only. It's part of a larger effort to fix Py_Main() called after Py_Initialize(): apply the new Py_Main() configuration (not sure if this PR really makes things easier, but I wanted to do this change anyway :-D). |
|
This commit causes a warning with Clang 5.0.0 (as seen in the TravisCI build): |
It should have been fixed now ;-) |
| } | ||
| else { | ||
| *dst = NULL; | ||
| } |
There was a problem hiding this comment.
6 participants
Rework _PyCoreConfig_Read() function which reads core configuration
to not modify the path configuration.
A new _PyCoreConfig_SetPathConfig() function now recreates the path
configuration from the core configuration. This function is now
called very late in _Py_InitializeCore(), just before calling
initimport().
Changes:
function now requires a _PyPathConfig
variable.
configuration
https://bugs.python.org/issue34170