◐ Shell
reader mode source ↗
Skip to content

bpo-39406: Implement os.putenv() with setenv() if available#18095

Closed
vstinner wants to merge 2 commits into
python:masterfrom
vstinner:setenv
Closed

bpo-39406: Implement os.putenv() with setenv() if available#18095
vstinner wants to merge 2 commits into
python:masterfrom
vstinner:setenv

Conversation

@vstinner

@vstinner vstinner commented Jan 21, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

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

@vstinner
bpo-39406: Implement os.putenv() with setenv() if available …
472b8cf 
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.
@vstinner

vstinner commented Jan 21, 2020

Copy link
Copy Markdown
Member Author

I updated the PR to use SetEnvironmentVariable() on Windows.

@serhiy-storchaka: Would you mind to review the updated PR?

@vstinner

vstinner commented Jan 21, 2020

Copy link
Copy Markdown
Member Author

On Windows, I removed the explicit check on _MAX_ENV:

    if (size > _MAX_ENV) {
        PyErr_Format(PyExc_ValueError,
                     "the environment variable is longer than %u characters",
                     _MAX_ENV);
        goto error;
    }

On my Windows 10 VM, _MAX_ENV is equal to 32767, but SetEnvironmentVariableW() seems to accept way longer values!

>>> value='x' * (32767); os.environ['long'] = value; os.environ['long'] == value, len(value)    
(True, 32767)
>>> value='x' * (32767+10); os.environ['long'] = value; os.environ['long'] == value, len(value) 
(True, 32777)
>>> value='abcd' * (32767); os.environ['long'] = value; os.environ['long'] == value, len(value) 
(True, 131068)

But I got ERROR_NO_SYSTEM_RESOURCES (error 1450: "Insufficient system resources exist to complete the requested service") for a value of 1 GB:

>>> import os
>>> value='x'*(2**30)
>>> os.putenv('long', value)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [WinError 1450] Ressources système insuffisantes pour terminer le service demandé

The limit seems to be the memory rather than an hardcoded limit. A "shorter" value (64 MB) is fine:

>>> import os
>>> value='x'*(2**26)
>>> os.putenv('long', value)

@vstinner

vstinner commented Jan 21, 2020

Copy link
Copy Markdown
Member Author

Oh, a test fail on Windows:


2020-01-21T10:50:49.1283745Z ======================================================================
2020-01-21T10:50:49.1283887Z ERROR: test_unset_error (test.test_os.EnvironTests)
2020-01-21T10:50:49.1284029Z ----------------------------------------------------------------------
2020-01-21T10:50:49.1284166Z Traceback (most recent call last):
2020-01-21T10:50:49.1284296Z   File "D:\a\cpython\cpython\lib\test\support\__init__.py", line 695, in wrapper
2020-01-21T10:50:49.1284429Z     return func(*args, **kw)
2020-01-21T10:50:49.1284504Z   File "D:\a\cpython\cpython\lib\test\test_os.py", line 962, in test_unset_error
2020-01-21T10:50:49.1284628Z     self.assertRaises(ValueError, os.environ.__delitem__, key)
2020-01-21T10:50:49.1284764Z   File "D:\a\cpython\cpython\lib\unittest\case.py", line 799, in assertRaises
2020-01-21T10:50:49.1284895Z     return context.handle('assertRaises', args, kwargs)
2020-01-21T10:50:49.1285989Z   File "D:\a\cpython\cpython\lib\unittest\case.py", line 202, in handle
2020-01-21T10:50:49.1286157Z     callable_obj(*args, **kwargs)
2020-01-21T10:50:49.1286268Z   File "D:\a\cpython\cpython\lib\os.py", line 691, in __delitem__
2020-01-21T10:50:49.1286377Z     raise KeyError(key) from None
2020-01-21T10:50:49.1295463Z KeyError:

@vstinner

vstinner commented Jan 22, 2020

Copy link
Copy Markdown
Member Author

I made other changes in the meanwhile which introduced conflicts. So I wrote a new PR instead: PR #18128.

@vstinner vstinner mentioned this pull request Jan 22, 2020
Merged
@vstinner vstinner closed this Jan 22, 2020
@vstinner vstinner deleted the setenv branch January 22, 2020 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@pitrou pitrou Awaiting requested review from pitrou

@pablogsal pablogsal Awaiting requested review from pablogsal

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vstinner @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot