Perfect your code
With built-in code review tools, GitHub makes it easy to raise the quality bar before you ship. Join the 40 million developers who've merged over 200 million pull requests.
Sign up for free See pricing for teams and enterprisesbpo-39406: Implement os.putenv() with setenv() if available #18095
Conversation
If setenv() C function is available, os.putenv() is now implemented with setenv() instead of putenv(), so Python doesn't have to handle the environment variable memory. Renane posix_putenv_garbage to posix_putenv_dict in posixmodule.c. On Windows or if setenv() is available, posix_putenv_dict is no longer needed.
This comment has been minimized.
This comment has been minimized.
|
I updated the PR to use SetEnvironmentVariable() on Windows. @serhiy-storchaka: Would you mind to review the updated PR? |
This comment has been minimized.
This comment has been minimized.
|
On Windows, I removed the explicit check on _MAX_ENV:
On my Windows 10 VM, _MAX_ENV is equal to 32767, but SetEnvironmentVariableW() seems to accept way longer values!
But I got ERROR_NO_SYSTEM_RESOURCES (error 1450: "Insufficient system resources exist to complete the requested service") for a value of 1 GB:
The limit seems to be the memory rather than an hardcoded limit. A "shorter" value (64 MB) is fine:
|
| /* putenv() requires to manage the memory manually: use a Python dict | ||
| object mapping name (bytes) to env (bytes) where env is a bytes string | ||
| like "name=value\0". */ | ||
| PyObject *posix_putenv_dict; |
This comment has been minimized.
This comment has been minimized.
| } | ||
| #else /* MS_WINDOWS */ | ||
| #elif defined(HAVE_PUTENV) |
This comment has been minimized.
This comment has been minimized.
serhiy-storchaka
Jan 21, 2020
Member
| #elif defined(HAVE_PUTENV) | |
| #elif defined(HAVE_SETENV) || defined(HAVE_PUTENV) |
| Py_RETURN_NONE; | ||
| } | ||
| #endif /* MS_WINDOWS */ | ||
| #endif /* HAVE_PUTENV */ | ||
| #endif /* HAVE_PUTENV */ |
This comment has been minimized.
This comment has been minimized.
serhiy-storchaka
Jan 21, 2020
Member
| #endif /* HAVE_PUTENV */ | |
| #endif /* HAVE_SETENV || HAVE_PUTENV */ |
This comment has been minimized.
This comment has been minimized.
|
Oh, a test fail on Windows:
|
vstinner commentedJan 21, 2020
•
edited by bedevere-bot
If setenv() C function is available, os.putenv() is now implemented
with setenv() instead of putenv(), so Python doesn't have to handle
the environment variable memory.
If setenv() is available, posix_putenv_garbage state is not needed.
https://bugs.python.org/issue39406