-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-39406: Implement os.putenv() with setenv() if available #18095
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
I updated the PR to use SetEnvironmentVariable() on Windows. @serhiy-storchaka: Would you mind to review the updated PR? |
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why rename it? This increases the size of the diff.
} | ||
#else /* MS_WINDOWS */ | ||
#elif defined(HAVE_PUTENV) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#elif defined(HAVE_PUTENV) | |
#elif defined(HAVE_SETENV) || defined(HAVE_PUTENV) |
Py_RETURN_NONE; | ||
} | ||
#endif /* MS_WINDOWS */ | ||
#endif /* HAVE_PUTENV */ | ||
#endif /* HAVE_PUTENV */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#endif /* HAVE_PUTENV */ | |
#endif /* HAVE_SETENV || HAVE_PUTENV */ |
Oh, a test fail on Windows:
|
I made other changes in the meanwhile which introduced conflicts. So I wrote a new PR instead: PR #18128. |
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