Skip to content
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

subprocess.check_output should allow specifying stdin as a string #60828

Closed
zwol mannequin opened this issue Dec 5, 2012 · 22 comments
Closed

subprocess.check_output should allow specifying stdin as a string #60828

zwol mannequin opened this issue Dec 5, 2012 · 22 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@zwol
Copy link
Mannequin

zwol mannequin commented Dec 5, 2012

BPO 16624
Nosy @bitdancer, @serhiy-storchaka
Files
  • issue16624-v34a.diff: Patch vs 3.4 (updated)
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2013-04-22.17:23:51.714>
    created_at = <Date 2012-12-05.22:54:16.995>
    labels = ['type-feature', 'library']
    title = 'subprocess.check_output should allow specifying stdin as a string'
    updated_at = <Date 2013-04-22.17:23:51.713>
    user = 'https://bugs.python.org/zwol'

    bugs.python.org fields:

    activity = <Date 2013-04-22.17:23:51.713>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2013-04-22.17:23:51.714>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2012-12-05.22:54:16.995>
    creator = 'zwol'
    dependencies = []
    files = ['29924']
    hgrepos = []
    issue_num = 16624
    keywords = ['patch']
    message_count = 22.0
    messages = ['177014', '177036', '177120', '177125', '177130', '179692', '180871', '187254', '187272', '187277', '187278', '187279', '187285', '187289', '187290', '187291', '187292', '187294', '187299', '187317', '187573', '187574']
    nosy_count = 5.0
    nosy_names = ['r.david.murray', 'BreamoreBoy', 'python-dev', 'serhiy.storchaka', 'zwol']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue16624'
    versions = ['Python 3.4']

    @zwol
    Copy link
    Mannequin Author

    zwol mannequin commented Dec 5, 2012

    subprocess.check_output calls Popen.communicate but does not allow you to specify its argument (i.e. data to send to the child process's stdin). It would be nice if it were enhanced to allow this. Proposed patch attached (to the 2.7 subprocess.py; should apply with trivial changes in 3.x).

    @zwol zwol mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Dec 5, 2012
    @serhiy-storchaka
    Copy link
    Member

    2.7 is in bugfix only mode. Please update your patch to 3.4.

    @zwol
    Copy link
    Mannequin Author

    zwol mannequin commented Dec 7, 2012

    OK, here is a patch against the latest development version. Now also with tests and documentation updates.

    @serhiy-storchaka
    Copy link
    Member

    This is a beautiful patch. LGTM.

    However it should be tested on Windows. I'm not sure that reading not closed file in different process works on Windows.

    Zack, can you please submit a contributor form?

    http://python.org/psf/contrib/contrib-form/
    http://python.org/psf/contrib/

    @zwol
    Copy link
    Mannequin Author

    zwol mannequin commented Dec 7, 2012

    I don't have the ability to test on Windows, but the construct you are concerned about was copied from other tests in the same file which were not marked as Unix-only.

    I have faxed in a contributor agreement.

    @serhiy-storchaka serhiy-storchaka self-assigned this Dec 29, 2012
    @serhiy-storchaka
    Copy link
    Member

    Zack, can you please resend your agreement by email?

    @zwol
    Copy link
    Mannequin Author

    zwol mannequin commented Jan 28, 2013

    Contributor agreement resent by email. Sorry for the delay.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Apr 18, 2013

    The patch doesn't apply cleanly on Windows. Trying to sort this with TortoiseHg has left me completely flummoxed so can I leave this with the OP to provide a new patch?

    @zwol
    Copy link
    Mannequin Author

    zwol mannequin commented Apr 18, 2013

    Here is a new patch vs latest trunk.

    @serhiy-storchaka
    Copy link
    Member

    I think that it will be better not introduce a new argument, but reuse stdin. Just allow io.BytesIO (or perhaps even any file object) be specified as stdin.

    The change will be straightforward:

    if isinstance(stdin, io.BytesIO):
        inputdata = stdin.read()
        stdin = PIPE

    Or more general:

    if not(stdin is None or stdin in (PIPE, DEVNULL) or isinstance(stdin, int)):
        try:
            stdin.fileno()
        except (AttributeError, UnsupportedOperation, OSError):
            inputdata = stdin.read()
            stdin = PIPE

    @zwol
    Copy link
    Mannequin Author

    zwol mannequin commented Apr 18, 2013

    I think that it will be better not introduce a new argument, but reuse stdin. Just allow io.BytesIO (or perhaps even any file object) be specified as stdin.

    If we were designing from scratch I might agree, but we aren't. Principle of least astonishment says that the API here should match subprocess.communicate, and that has separate stdin=FILE and input=STRING arguments.

    @zwol
    Copy link
    Mannequin Author

    zwol mannequin commented Apr 18, 2013

    Note also that allowing stdin=<any filelike> in a clean fashion would require rather more surgery than you suggest, because a filelike can produce an infinite stream of data, and people would expect that to work when the subprocess only reads a finite prefix; making it actually work would involve teaching communicate() to take a filelike and copy blocks into the pipe. I have no objection to that change but I think it is too much mission creep for this proposal.

    With the present design, where stdin= has to be something for which fileno() is defined, and input= has to be a string (hence of finite length), no one is going to expect something to work that won't.

    @serhiy-storchaka
    Copy link
    Member

    I partially agree with you. We must copy blocks in communicate().

    Your patch is great, but I doubt that there is a best feature. If we teach communicate() to work with file-like objects, this feature will exceed your suggestion and 'input' parameter of check_output() will be redundant. Are you want to implement a more powerful feature? If not, I will commit your patch. But I hope that before the 3.4 release we will replace it with a more powerful feature.

    Mark, can you please run subprocesses tests on Windows?

    @zwol
    Copy link
    Mannequin Author

    zwol mannequin commented Apr 18, 2013

    My position is:

    • input= should be supported in check_output(), for consistency with communicate().

    • I like the idea of making stdin= support file-like objects which don't have a fileno, in both communicate() and everything that calls it, but that does not belong in this issue, I do not have time to code it myself, and, again, it should be *in addition to*, not *instead of*, supporting input= in check_output().

    @bitdancer
    Copy link
    Member

    Since it is a new feature either way, we can add stdin support and deprecate the input= argument of communicate. But we can also go with the input= for check_output now, and see if anyone steps up to do the bigger patch before 3.4 hits beta. Which is what I hear Serhiy suggesting.

    @zwol
    Copy link
    Mannequin Author

    zwol mannequin commented Apr 18, 2013

    ??? communicate() has always had input= AFAIK.

    @bitdancer
    Copy link
    Member

    Yes. IIUC are talking about possibly replacing it with a more powerful feature. We wouldn't actually remove it, but we would recommend using the new feature instead, thus making the fact that check_output doesn't have it irrelevant.

    @zwol
    Copy link
    Mannequin Author

    zwol mannequin commented Apr 18, 2013

    OK, I get that, but what I'm saying is I think input= is still desirable even if stdin= becomes more powerful.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Apr 18, 2013

    Tests rerun on Windows and were fine.

    @bitdancer
    Copy link
    Member

    Why? What is the use case that it serves better? I'm not saying there isn't one, but one isn't occurring to me.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 22, 2013

    New changeset 5c45d0ca984f by Serhiy Storchaka in branch 'default':
    Issue bpo-16624: subprocess.check_output now accepts an input argument,
    http://hg.python.org/cpython/rev/5c45d0ca984f

    @serhiy-storchaka
    Copy link
    Member

    Thanks Zack for you patch. Thanks Mark for testing.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants