bpo-34170: Rework _PyCoreConfig_Read() to avoid side effect by vstinner · Pull Request #8353 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation8 Commits6 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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()

https://bugs.python.org/issue34170

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.

Choose a reason for hiding this comment

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

+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)

}

if (config->executable == NULL) {
config->executable = _PyMem_RawWcsdup(path_config.program_full_path);

Choose a reason for hiding this comment

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

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):

In file included from Parser/pgenmain.c:20:
./Include/internal/pystate.h:55:3: warning: redefinition of typedef '_PyPathConfig' is a C11 feature [-Wtypedef-redefinition]
} _PyPathConfig;
  ^
./Include/pylifecycle.h:123:30: note: previous definition is here
typedef struct _PyPathConfig _PyPathConfig;
                             ^
1 warning generated.

This commit causes a warning with Clang 5.0.0 (as seen in the TravisCI build):

It should have been fixed now ;-)

Choose a reason for hiding this comment

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