Repository navigation
Create a bytes version of os.environ and getenvb() #52849
Description
| BPO | 8603 |
|---|---|
| Nosy | @malemburg, @loewis, @gpshead, @pitrou, @vstinner, @ezio-melotti |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2010-09-10.22:19:03.024>
created_at = <Date 2010-05-03.08:56:24.666>
labels = ['interpreter-core', 'type-feature', 'library', 'expert-unicode']
title = 'Create a bytes version of os.environ and getenvb()'
updated_at = <Date 2010-09-10.22:19:03.023>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2010-09-10.22:19:03.023>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2010-09-10.22:19:03.024>
closer = 'vstinner'
components = ['Interpreter Core', 'Library (Lib)', 'Unicode']
creation = <Date 2010-05-03.08:56:24.666>
creator = 'vstinner'
dependencies = []
files = ['17236']
hgrepos = []
issue_num = 8603
keywords = ['patch']
message_count = 38.0
messages = ['104825', '104827', '104838', '104839', '104871', '104877', '104878', '104879', '104880', '104883', '104884', '104886', '104889', '104894', '104895', '104897', '104898', '104900', '104901', '104913', '104920', '104987', '105003', '105006', '105080', '105115', '105116', '105124', '105131', '105142', '105143', '105167', '111878', '111880', '111897', '111972', '111976', '116050']
nosy_count = 7.0
nosy_names = ['lemburg', 'loewis', 'gregory.p.smith', 'pitrou', 'vstinner', 'ezio.melotti', 'Arfrever']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue8603'
versions = ['Python 3.2']
Activity
vstinner commentedon May 3, 2010
As discussed in issue bpo-8514, I propose a bytes version of os.envionb which would be synchronized with os.environ (which is possible thanks to surrogateescape error handler).
I also propose a os.getenvb()->bytes function.
I don't know yet if it's a good idea of not, but my patch accepts both bytes and str for os.environ(b).get() and os.getenv(b)().
antoine> In posixmodule.c, (...) if memory allocation of the bytes
antoine> object fails, we should error out.
I would require to change also the Windows version and the code specific to OS/2. Ok to do that, but after closing this issue ;-) I don't want to change to much things at the same time.
vstinner commentedon May 3, 2010
The patch creates also fsencode()/fsdecode() functions proposed in bpo-8514: I can rename them to use protected name (eg. "_encodeenv" and "_decodeenv") until we decided for these functions.
malemburg commentedon May 3, 2010
A view comments on the patch:
+ def __init__(self, data, encodekey, decodekey, encodevalue, decodevalue, putenv, unsetenv):
As general guideline: When adding new parameter, please add them to the
end of the parameter list and preferably with a default argument in order
to not break the API.
Doesn't matter much in this case, since _Environ is only used internally,
but it's still good practice.
--
+data = {}
+for key, value in environ.items():
+ data[_keymap(key)] = fsencode(value)
Please put such init code into a function or make sure that the global
module space is not polluted with temporary variables such as data,
key, value.
--
+ # bytes environ
+ environb = _Environ(data, _keymap, fsencode, fsencode, fsencode, _putenv, _unsetenv)
This looks wrong even though it will work, but that's only a
side-effect of how fsencode is coded and that's not how the
stdlib should be coded (see below).
--
+ def fsencode(value):
+ """
+ unicode to file system
+ """
+ if isinstance(value, bytes):
+ return value
+ else:
+ return value.encode(sys.getfilesystemencoding(), 'surrogateescape')
The function should not accept bytes as input or at least
document this pass-through behavior, leaving the user to decide
whether that's wanted or not.
--
The patch is also missing code which keeps the two dictionaries in
sync. If os.environ changes, os.environb would have to change as
well.
vstinner commentedon May 3, 2010
No, it doesn't :-) os.environ and os.environb are synchronized and there is a test for this! ;-)
I will see later for your other comments.
loewis commentedon May 3, 2010
Can somebody please explain what problem is being solved with this patch?
malemburg commentedon May 3, 2010
Martin v. Löwis wrote:
The way os.environ is currently set up on Unix breaks applications
using the environment to pass data to helper applications.
Please see the discussion on http://bugs.python.org/issue8514
for details.
loewis commentedon May 3, 2010
I can't see any report of actual breakage in that report, only claims of
potential breakage (with no supporting examples)
vstinner commentedon May 3, 2010
An issue was opened 2 years ago: "It was brought up in a discussion of sending non-ASCii data to a CGI-WSGI script where the data would be transferred via os.environ." => bpo-4006 (closed as "wont fix").
loewis commentedon May 3, 2010
Fortunately, that issue could now be reconsidered as "fixed"; the
example in the report (msg74118) now works correctly - thanks to PEP-383. So I still fail to see the problem.
malemburg commentedon May 3, 2010
Martin v. Löwis wrote:
Set your CODESET to ASCII and watch the surrogate escaping
begin... seriously, Martin, if you've ever worked with CGI
or WSGI or FastCGI or SCGI or any of the many other protocols
that use the OS environment for passing data between processes,
it doesn't take much imagination to come up with examples
that fail left and right.
Here's one (RFC 3875, sections 4.1.7 and 4.1.5):
(HTML uses Latin-1 as default encoding and so do many of the
protocols invented for it !)
The file system encoding simply doesn't relate to the OS
environment at all - it's just a collection of name=value mappings
with no explicit encoding information. It may be a good guess,
but that's it.
loewis commentedon May 3, 2010
I still don't see a *failure* here. AFAICT, it all works correctly.
In particular, I fail to see the advantage of using bytes over using
escaped strings, in terms of correctness. I'm even skeptical that there
is an advantage in terms of usability (and if there is, I'd like to see
a demonstration of that, as well).
loewis commentedon May 3, 2010
BTW, I think you are misinterpreting the RFC. It doesn't actually say
that QUERY_STRING is Latin-1 encoded, but instead, it says
"the details of the parsing, reserved characters and support for non
US-ASCII characters depends on the context"
Latin-1 is only given as a possible example. Apache passes the URL from
the HTTP request unescaped; browsers will likely CGI-escape it. So most
likely, it will be
IMO, applications are much better off to consider QUERY_STRING as a
character string.
16 remaining items
vstinner commentedon May 6, 2010
Aaaah, *posix*.environ, not *os*.environ, ok. I will fix posix documentation. I didn't knew this dictionary :-)
vstinner commentedon May 6, 2010
Patch version 3:
vstinner commentedon May 6, 2010
Oh no, I forgot to remove the annotations from getenv() and getenvb() in os.py. I only removed them from the documentation.
vstinner commentedon May 6, 2010
Commited as r80885 (py3k), blocked in 3.1 (r80886).
Thank you Martin and Marc for your great help and all your reviews ;-)
ezio-melotti commentedon Jul 29, 2010
FWIW os.environb is missing from os.__all__.
ezio-melotti commentedon Jul 29, 2010
A quick search0 also shows that environ.data is used by several projects. Changing it from str to bytes will most likely break these programs, so I'm not sure it's a good idea.
Also, if I understand correctly, on Windows os.environ.data still contains str, so it's inconsistent with other systems.
IMHO os.environ.data should contain str, whereas os.environb.data bytes. If they must share the same data I would prefer them to be both str.
malemburg commentedon Jul 29, 2010
Ezio Melotti wrote:
Direct use of os.environ.data is not permitted as it is not a documented
feature, so I wouldn't worry about this.
The few uses you found are easy to fix.
vstinner commentedon Jul 29, 2010
Le jeudi 29 juillet 2010 03:08:27, Ezio Melotti a écrit :
_Environ is a wrapper on data: call putenv() when a value is changed and
unputenv() when a value is removed. Since os.environb, it does also convert
key and value types. So .data should not be used directly.
It looks like a common usage of .data is to get get to "a real dict" (and not
"an user dict"). I think that os.environ.data or os.environ.data.copy() can be
replaced by dict(os.environ) is that case.
I'm not sure of that. It looks (in the search result) that os.environ.data is
just stored and passed to a function, but not used as a dictionary to get a
value (well, I'm not sure because it's hard to say with just 3 lines of the
program). In that case, it doesn't matter if the dictionary contains byte or
unicode strings.
Eg. zope2instance:
It does still work with bytes.
Yes, it's not a good idea to use .data :-) This attribute should be protected,
not public (even if it is not documented).
data is shared between os.environ and os.environb, because data is the
solution to synchronize both dictionaries.
No, data should use the native type: bytes on UNIX and BSD, str on Windows.
--
If you still consider that the change on .data as a bug, I think that the fix
is to remove .data (mark it as protected: environ.data => environ._data).
vstinner commentedon Jul 29, 2010
Le jeudi 29 juillet 2010 03:00:03, Ezio Melotti a écrit :
Fixed by r83237
vstinner commentedon Sep 10, 2010
r84690 marks os.environ.data as protected. Close this issue again.