Issue 14156: argparse.FileType for '-' doesn't work for a mode of 'rb' (original) (raw)
Created on 2012-02-29 11:01 by anacrolix, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Messages (33)
Author: Matt Joiner (anacrolix)
Date: 2012-02-29 11:01
If an argument of '-' is handled by argparse.FileType, it defaults to sys.stdin. However a mode of 'rb' is ignored, the returned file object does not work with raw bytes.
Author: Steven Bethard (bethard) * 
Date: 2012-02-29 15:27
Yes, the problem is in FileType.call - the handling of '-' is pretty simplistic. Patches welcome.
Author: Matt Joiner (anacrolix)
Date: 2012-03-14 16:17
Roger that. I'll start on a patch for this in a month or two if all goes well.
Author: Matt Joiner (anacrolix)
Date: 2012-03-15 18:11
Steven, patch attached. I lost steam in the unittests with all the meta, suffice it that the names match the file descriptors of the stream sources. i.e. FileType('rb') would give a file with name=0, and so forth. My chosen method also allows other mode flags as well as custom bufsizes.
Author: Moritz Klammler (moritz)
Date: 2012-06-05 13:51
I don't know how if this is the perfect solution but it keeps the program from crashing.
1154c1154,1157 < return _sys.stdin
if 'b' in self._mode: return _sys.stdin.buffer else: return _sys.stdin1156c1159,1162 < return _sys.stdout
if 'b' in self._mode: return _sys.stdout.buffer else: return _sys.stdout
Author: Steven Bethard (bethard) *
Date: 2012-07-22 21:06
The fix looks right, but we definitely need a test. I tried to write one, but I'm not sure how to do this properly given how test_argparse redirects standard input and output (so that fileno() doesn't work anymore). I've attached my current (failing) attempt to test this.
Author: paul j3 (paul.j3) * 
Date: 2014-04-01 07:42
A related issue http://bugs.python.org/issue13824 "argparse.FileType opens a file and never closes it"
I proposed a FileContext class that returns a `partial(open, filename,...)' context object. The issues of how to deal with stdin/out are similar.
Author: paul j3 (paul.j3) *
Date: 2014-04-03 00:11
There are a couple of complications to using 'fileno'.
We probably don't want to close 'sys.stdin' or 'sys.stdout' (not even if they are redirected to other files?). That means using:
open(sys.stdin.fileno(), ..., closefd=False)'closefd', on the other hand, has to be True for string file specifications.
But in 'test_argparse.py', 'sys.stdout' is redirected to an 'io.StringIO'. This has many of the same features as an open file, but 'fileno' is not implemented. So the TypeFile probably needs to make an exception for this case. I don't how this will play with a 'BytesIO' for 'wb' cases.
Author: Eli Bendersky (eli.bendersky) *
Date: 2014-06-22 16:17
Nosy-ing myself since I just ran into it. Annoying issue that precludes from using argparse's builtin '-' recognition for reading binary data.
I'll try to carve some time later to look at the patches.
Author: Eli Bendersky (eli.bendersky) *
Date: 2014-06-24 13:27
The patch looks reasonable? Is the only remaining problem with crafting the test?
Author: Eli Bendersky (eli.bendersky) *
Date: 2014-06-24 13:28
[sorry, the first question mark shouldn't be - the patch indeed looks reasonable to me]
Steven - how about launching a subprocess for stdin tests to avoid weird issues?
Author: Aviv Palivoda (palaviv) *
Date: 2016-03-05 17:56
I fixed the tests to work with Steven patch. Also changed the patch to open sys.std{in,out} with closefd=False.
I changed the 'io.StringIO' that we redirect the stdout, stderr to. Now the 'StdIOBuffer' return the real stdout,stderr when '-' is passed to FileType. This was already done in the 'stderr_to_parser_error' function previously after the call to 'parse_args'.
Author: Aviv Palivoda (palaviv) *
Date: 2016-12-10 13:47
Pinging as mentioned in the devguide.
Author: Matt Joiner (anacrolix)
Date: 2016-12-11 14:55
This is why I stopped contributing to Python.
Author: paul j3 (paul.j3) *
Date: 2016-12-11 21:21
The problem with the argparse backlog is that the original author of the module is too busy with other things. And no one has stepped into his shoes. There are people with experience in apply patches, and people who know the argparse code well, but few, if any with both skills (and/or the time to invest in this module).
In addition the module has some serious backward compatibility issues. I know of several patches that were applied, and then withdrawn because of unforseen (or at least untested) compatibility problems.
While I commented earlier, I don't recall testing it. I just tried it now, and ran into problems - until I realized this isn't compatible with Python2.7. Py3 is the development world, but there's still a lot of PY2 use (e.g look at Stackoverflow argparse questions). On SO if people have problems with FileType, I often recommend that they just accept the filename, and take care of opening it themselves.
To raise the attention to this patch I'd suggest
making the case that it is really needed
demonstrating that it has been field tested, and is free of backward compatibility issues.
Author: Aviv Palivoda (palaviv) *
Date: 2016-12-14 18:32
Hi paul thanks for looking into this. First are you sure this is a bug in python 2? If so I will happily port this patch once it is reviewed. As for use cases you may look at issue #26488. Although the patch was rejected you can see that I first used argparse.FileType and moved it because of this issue. I can't prove this patch is free of backward's compatibility issue's but there are tests now which should help to avoid problem.
Author: paul j3 (paul.j3) *
Date: 2016-12-14 19:10
The problem with Python2.7 is that 'open' does not take 'closefd', or any of the other parameters that were added for Python3.
open(name[, mode[, buffering]])'rb' may make a difference on Py2 on Windows, but I haven't done any work in the environment in a long time.
I wasn't aware of that other issue. Some core Python developers have participated in that one. I suspect a lot of the discussion is beyond my level of expertise.
I once wrote that I thought 'FileType' was included primarily as an example of a 'type' factory. Something users could copy and extend for their own use. Bethard corrected me, saying that it was meant for quick-n-dirty script uses, ones with an input file, output file and a few options. In a bigger scripts, the users are encouraged to open/close files in 'with' contexts.
See http://bugs.python.org/issue22884 and the issues I reference there.
Author: Aviv Palivoda (palaviv) *
Date: 2016-12-14 20:00
As we talk here about stdin and stdout which we don't want to close I think this issue is irrelevant to python2. In any case the patch in issue #13824 cover that problem. I actually write a lot of small scripts and if I need to open the file in binary mode I don't use FileType because of this issue. Any way I think this discussion is irrelevant because it does not seem like any core developer is currently working on argparse.
Author: Evan Andrews (evan_) *
Date: 2016-12-17 03:45
This issue is relevant to Python 2 on Windows since you need to disable the EOL conversion if you're trying to receive binary data on stdin.
See: http://stackoverflow.com/questions/2850893/reading-binary-data-from-stdin
Author: Matteo Bertini (naufraghi) *
Date: 2017-05-05 10:31
Bumped in this bug yesterday, sadly a script working (by chance) in Python2 doesn't work in Python3 because of this bug.
Author: Marcel H (Marcel H2)
Date: 2017-05-26 10:03
I want to see this fixed in python3.x as well, please :) the patch should be the same
Author: (sedrubal)
Date: 2017-07-09 21:07
What is the problem with using the patch by moritz (https://bugs.python.org/issue14156#msg162342) and change the unit test to this:
class TestFileTypeWB(TempDirMixin, ParserTestCase): ... successes = [ ..., ('-x - -', NS(x=sys.stdout.buffer, spam=sys.stdout.buffer)), ]
and respectively for stdin?
Author: R. David Murray (r.david.murray) *
Date: 2017-07-12 17:21
The biggest problem, as paul.j3 says, is to get someone from core to review the argparse issues. I am currently planning to make argparse one of my foci in a sprint we are doing at the beginning of September, so there is some hope....
Any reviews/testing people do on argparse patches between now and then will be helpful.
Author: Leo Singer (Leo Singer)
Date: 2018-09-08 19:25
I just hit this bug. Would the proposed patch get any more attention if submitted as a pull request?
Author: paul j3 (paul.j3) *
Date: 2018-09-09 17:03
It's been sometime since I looked at this issue.
The main sticking point is passing unittests, and ensuring that there are no backward compatibility issues.
But, since FileType is a standalone class, anyone could put a corrected version in their own workspace without modifying their stock version. The 'type' parameter is designed for this kind of flexibility - it accepts any callable, whether a function, or a class with a call method.
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2018-09-11 14:38
The solution with fileno() is clever, but as was mentioned before, it doesn't work if stdin or stdout are not real files, but something like StringIO. It is not that in common use of argparse for parsing arguments in scripts they are redefined, but argparse can be used in uncommon environments, for example for emulating command line in the environment like IDLE which redefines standard streams. And I'm sure this will break third-party tests which main() with patched stdin/stdout for testing CLI. The initial solution proposed by Moritz is more reliable, although it doesn't fix an issues with closing stdin/stdout. But this is a different issue at all.
Author: Josh Rosenberg (josh.r) *
Date: 2019-05-07 16:18
Serhiy: To be clear, Moritz's patch won't work if stdin/stdout are io.StringIO or the like either, since StringIO lacks a buffer attribute.
Personally, I'm content to:
- Ignore the closeability of the standard handles; that's not part of this bug, and just introduces more headaches to getting a patch approved (and conceivably breaks back compat if the user wants the handle closed even if it's a standard handle). Nothing is made worse by ignoring it after all, just not made better immediately.
- Ignore the possibility of stdin/stdout without a buffer attribute; it will raise an error, but that's a reasonable response to demanding a binary stream in a scenario where only a text stream is available
- Not use fileno, as it mostly supports the "anti-close" behavior I dismissed in #1, and has a bunch of pitfalls (e.g. a standard handle rebound to a TextIOWrapper around GzipFile or the like has a fileno attribute, but using it bypasses the compression; basically, you can't consider fileno to be equivalent to the underlying binary stream, because binary streams can be wrapped as well).
I'm going to convert Moritz's proposal to a PR and hope I can get core developer approval for it.
Author: Josh Rosenberg (josh.r) *
Date: 2019-05-07 18:52
I've created PR13165 to address this bug. It's 99% test updates; the actual code changes to argparse.py are trivial and should be equally trivial to review.
The only functional change beyond Moritz's proposal is that I added support for having accepting '-' when the mode string uses 'a' or 'x' instead of 'w'; for sys.stdout, they're all effectively equivalent ('x' is trying to prevent stomping an existing file, which borrowing sys.stdout won't do, and sys.stdout is already more closely equivalent to mode 'a' in any event). No working code should break as a result of that change (passing 'a' or 'x' previously just caused FileType to exit immediately with a ValueError, which in turn caused parse_args to kill the program, which I'm assuming isn't considered a valuable "feature").
In addition to testing binary mode with argument '-' properly, I also added complete test cases for mode 'x' and 'xb' (for all arguments, both file names and '-') since we had no such tests, and ensuring exclusive creation mode behaves correctly is fairly important.
Author: Peter Wu (lekensteyn)
Date: 2020-07-05 10:53
I just ran into this issue on Linux when piping a binary file to stdin resulted in a UnicodeDecodeError while trying to read a byte from the stream. Passing /dev/stdin is a workaround that does not require modifications to an application.
As for the proposed PR 13165, I'd suggest to gracefully fallback to normal stdout/stdin if the buffer is not available. That approach is also followed in the fileinput module, and takes care of the note for library developers in the documentation at https://docs.python.org/3/library/sys.html#sys.stdin
Not feeling particular strong about the graceful handling, but I hope that the test code can be simplified in that case.
Author: William Woodruff (yossarian) *
Date: 2022-03-03 22:07
Nosying myself; this affects 3.9 and 3.10 as well.
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2022-03-06 11:49
New changeset eafec26ae5327bb23b6dace2650b074c3327dfa0 by MojoVampire in branch 'main': bpo-14156: Make argparse.FileType work correctly for binary file modes when argument is '-' (GH-13165) https://github.com/python/cpython/commit/eafec26ae5327bb23b6dace2650b074c3327dfa0
Author: miss-islington (miss-islington)
Date: 2022-03-06 12:12
New changeset ee18df425209cfa4f394b57220177c168fc3a1da by Miss Islington (bot) in branch '3.10': bpo-14156: Make argparse.FileType work correctly for binary file modes when argument is '-' (GH-13165) https://github.com/python/cpython/commit/ee18df425209cfa4f394b57220177c168fc3a1da
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2022-03-18 15:02
New changeset 4d2099f455229b10f88846dbe9fe6debbee55356 by Serhiy Storchaka in branch '3.9': [3.9] bpo-14156: Make argparse.FileType work correctly for binary file modes when argument is '-' (GH-13165) (GH-31979) https://github.com/python/cpython/commit/4d2099f455229b10f88846dbe9fe6debbee55356
History
Date
User
Action
Args
2022-04-11 14:57:27
admin
set
github: 58364
2022-03-18 15:03:40
serhiy.storchaka
set
status: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: + Python 3.11, - Python 3.6, Python 3.7, Python 3.8
2022-03-18 15:02:53
serhiy.storchaka
set
messages: +
2022-03-18 13:43:51
serhiy.storchaka
set
pull_requests: + <pull%5Frequest30069>
2022-03-07 01:45:11
owenh
set
nosy: - owenh
2022-03-06 12:12:12
miss-islington
set
messages: +
2022-03-06 11:50:05
miss-islington
set
nosy: + miss-islington
pull_requests: + <pull%5Frequest29826>
2022-03-06 11:49:47
serhiy.storchaka
set
messages: +
2022-03-03 22:07:29
yossarian
set
nosy: + yossarian
messages: +
versions: + Python 3.9, Python 3.10
2021-08-17 03:03:23
owenh
set
nosy: + owenh
2020-07-05 10:53:50
lekensteyn
set
nosy: + lekensteyn
messages: +
2019-05-07 18:52:07
josh.r
set
keywords: + needs review
messages: +
2019-05-07 18:34:43
josh.r
set
pull_requests: + <pull%5Frequest13082>
2019-05-07 16🔞25
josh.r
set
messages: +
2018-09-11 14:38:56
serhiy.storchaka
set
nosy: + serhiy.storchaka
messages: +
2018-09-11 14:25:21
serhiy.storchaka
set
versions: + Python 3.8, - Python 3.4, Python 3.5
2018-09-09 17:03:17
paul.j3
set
messages: +
2018-09-09 09:17:44
hongweipeng
set
stage: test needed -> patch review
pull_requests: + <pull%5Frequest8577>
2018-09-08 19:25:35
Leo Singer
set
nosy: + Leo Singer
messages: +
2017-07-12 17:21:09
r.david.murray
set
nosy: + r.david.murray
messages: +
2017-07-09 21:07:18
sedrubal
set
nosy: + sedrubal
messages: +
2017-05-26 10:03:33
pitrou
set
nosy: - pitrou
2017-05-26 10:03:09
Marcel H2
set
nosy: + Marcel H2
messages: +
versions: + Python 3.6, Python 3.7
2017-05-05 10:31:29
naufraghi
set
nosy: + naufraghi
messages: +
2016-12-17 03:45:24
evan_
set
nosy: + evan_
messages: +
2016-12-15 12:47:52
anacrolix
set
nosy: - anacrolix
2016-12-14 20:00:53
palaviv
set
messages: +
2016-12-14 19:10:41
paul.j3
set
messages: +
2016-12-14 18:32:58
palaviv
set
messages: +
2016-12-11 21:21:20
paul.j3
set
messages: +
2016-12-11 14:55:27
anacrolix
set
messages: +
2016-12-10 21:06:06
paul.j3
set
priority: normal -> high
2016-12-10 13:47:48
palaviv
set
messages: +
2016-04-02 00:32:18
martin.panter
unlink
2016-03-28 09:06:48
SilentGhost
link
2016-03-05 17:56:49
palaviv
set
files: + 14156-2.patch
nosy: + palaviv
messages: +
2014-09-30 14:31:00
serhiy.storchaka
set
stage: test needed
type: crash -> behavior
versions: - Python 3.2, Python 3.3
2014-06-24 13:28:12
eli.bendersky
set
messages: +
2014-06-24 13:27:20
eli.bendersky
set
messages: +
2014-06-22 16:17:05
eli.bendersky
set
nosy: + eli.bendersky
messages: +
versions: + Python 3.3, Python 3.4, Python 3.5
2014-04-30 19:09:21
markgrandi
set
nosy: + markgrandi
2014-04-03 00:11:01
paul.j3
set
messages: +
2014-04-01 07:43:00
paul.j3
set
messages: +
2014-04-01 03:25:38
paul.j3
set
nosy: + paul.j3
2014-03-27 23:26:56
josh.r
set
nosy: + josh.r
2012-07-22 21:06:30
bethard
set
files: + 14156.patch
messages: +
2012-06-05 13:51:40
moritz
set
versions: + Python 3.2, - Python 3.3
nosy: + moritz
messages: +
type: behavior -> crash
2012-03-15 18:11:30
anacrolix
set
files: + argparse-filetype-stdio-modes.patch
keywords: + patch
messages: +
2012-03-14 16:17:47
anacrolix
set
messages: +
2012-02-29 15:27:10
bethard
set
messages: +
2012-02-29 11:25:07
eric.araujo
set
2012-02-29 11:01:10
anacrolix
create