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 should include filename in FileNotFoundError exception #66726

Closed
charpov mannequin opened this issue Oct 2, 2014 · 13 comments
Closed

subprocess should include filename in FileNotFoundError exception #66726

charpov mannequin opened this issue Oct 2, 2014 · 13 comments
Assignees
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@charpov
Copy link
Mannequin

charpov mannequin commented Oct 2, 2014

BPO 22536
Nosy @gpshead, @ronaldoussoren, @vstinner, @4kir4, @berkerpeksag
PRs
  • bpo-22536: Set the filename in FileNotFoundError. #3194
  • bpo-22536: Skip two non-windows tests on Windows #3202
  • bpo-22536 [3.6] Set filename in FileNotFoundError  #3305
  • Files
  • bug.py
  • 22536-subprocess-exception-filename.patch
  • 22536-subprocess-exception-filename-2.patch
  • 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/gpshead'
    closed_at = <Date 2017-09-06.20:40:35.155>
    created_at = <Date 2014-10-02.00:49:37.238>
    labels = ['3.7', 'type-bug', 'library']
    title = 'subprocess should include filename in FileNotFoundError exception'
    updated_at = <Date 2017-09-06.20:40:35.152>
    user = 'https://bugs.python.org/charpov'

    bugs.python.org fields:

    activity = <Date 2017-09-06.20:40:35.152>
    actor = 'gregory.p.smith'
    assignee = 'gregory.p.smith'
    closed = True
    closed_date = <Date 2017-09-06.20:40:35.155>
    closer = 'gregory.p.smith'
    components = ['Library (Lib)']
    creation = <Date 2014-10-02.00:49:37.238>
    creator = 'charpov'
    dependencies = []
    files = ['36774', '37202', '37322']
    hgrepos = []
    issue_num = 22536
    keywords = ['patch']
    message_count = 13.0
    messages = ['228147', '231221', '231293', '231297', '231392', '231400', '231414', '231881', '300751', '300808', '300814', '300815', '301266']
    nosy_count = 9.0
    nosy_names = ['gregory.p.smith', 'ronaldoussoren', 'vstinner', 'akira', 'tshepang', 'berker.peksag', 'charpov', 'travis.thieman', 'Christian H']
    pr_nums = ['3194', '3202', '3305']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue22536'
    versions = ['Python 3.6', 'Python 3.7']

    @charpov
    Copy link
    Mannequin Author

    charpov mannequin commented Oct 2, 2014

    FileNotFoundError should contain a 'filename' information, as per its specification. It's 'None' after a failure to execute a subprocess.

    @charpov charpov mannequin assigned ronaldoussoren Oct 2, 2014
    @charpov charpov mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) OS-mac type-bug An unexpected behavior, bug, or error labels Oct 2, 2014
    @ned-deily ned-deily added stdlib Python modules in the Lib dir and removed interpreter-core (Objects, Python, Grammar, and Parser dirs) OS-mac labels Oct 2, 2014
    @ned-deily ned-deily changed the title Missing filename in FileNotFoundError subprocess should include filename in FileNotFoundError exception Oct 2, 2014
    @travisthieman
    Copy link
    Mannequin

    travisthieman mannequin commented Nov 15, 2014

    The attached patch includes the first element in args in _execute_child to the OSError exception subclass. This correctly populates the 'filename' field on the resulting exception. A test is also included that fails without the patch.

    @4kir4
    Copy link
    Mannequin

    4kir4 mannequin commented Nov 17, 2014

    I can confirm that without the patch the filename attribute is None
    despite being mentioned in strerror.

    Travis, you should use orig_executable instead of args[0] to cover:

      subprocess.call("exit 0", shell=True, executable='/nonexistent bash')

    case.

    And use cwd if child_exec_never_called, to be consistent with the error message (see if/else statements above the raise statement).

    It seems appropriate to set filename even if errno is not ENOENT
    but to be conservative, you could provide filename iff err_msg is also changed i.e., iff errno is ENOENT.

    @4kir4
    Copy link
    Mannequin

    4kir4 mannequin commented Nov 17, 2014

    If the_oserror.filename is not None then str(the_oserror) appends the
    filename twice:

    [Errno 2] No such file or directory: 'nonexistent': 'nonexistent'

    You could remove err_msg += ':' ... statements to avoid the
    repeatition. It may break the code that uses strerror attribute. But
    exception error messages are explicitly excluded from backward
    compatibility considirations therefore it might be ok to break it
    here. I can't find the reference so it should probably be resolved as a
    new issue (independent from providing the filename attribute value).

    @vstinner
    Copy link
    Member

    You should only add the filename if the error if a FileNotFound exception, not for any OSError, and only if exec() was called. It looks like the variable child_exec_never_called indicates if exec() was called.

    @4kir4
    Copy link
    Mannequin

    4kir4 mannequin commented Nov 19, 2014

    It would be inconsitent to provide filename only if exec is called e.g.:

       >>> import subprocess
       subprocess.call("not used", cwd="nonexistent")
       FileNotFoundError: [Errno 2] No such file or directory: 'nonexistent'

    The error message shows the filename (cwd) despite exec not being
    called therefore it would be logical to provide non-None filename
    attribute here too.

    If we ignore the backward compatibility issue I've mentioned in msg231297
    then the current code:

                            if errno_num == errno.ENOENT:
                                if child_exec_never_called:
                                    # The error must be from chdir(cwd).
                                    err_msg += ': ' + repr(cwd)
                                else:
                                    err_msg += ': ' + repr(orig_executable)
                        raise child_exception_type(errno_num, err_msg)

    could be replaced with:

                        if errno_num == errno.ENOENT:
                            if child_exec_never_called:
                                \# The error must be from chdir(cwd).
                                filename = cwd
                            else:
                                filename = orig_executable
                            raise child_exception_type(errno_num, err_msg, filename)
                    raise child_exception_type(errno_num, err_msg)
    

    [1] https://hg.python.org/cpython/file/23ab1197df0b/Lib/subprocess.py#l1443

    @vstinner
    Copy link
    Member

    I wnated to say that args[0] is not the right filename if exec() was not
    called. Yes, there is also cwd for example. The logic to choose the
    filename should be done in the child.

    @travisthieman
    Copy link
    Mannequin

    travisthieman mannequin commented Nov 30, 2014

    Thank you all for the helpful comments. A revised attempt is attached as -2.patch, with improved behavior around using cwd if the child is never called and orig_executable if it is. I opted not to fix the issue with the redundancy in the error message as I agree that should be handled as a separate issue.

    @ChristianH
    Copy link
    Mannequin

    ChristianH mannequin commented Aug 23, 2017

    I was also bitten by this bug, and would like to see it merged. The patch 22536-subprocess-exception-filename-2.patch looks fine to me.

    @gpshead gpshead self-assigned this Aug 23, 2017
    @gpshead
    Copy link
    Member

    gpshead commented Aug 24, 2017

    New changeset 8621bb5 by Gregory P. Smith in branch 'master':
    bpo-22536: Set the filename in FileNotFoundError. (bpo-3194)
    8621bb5

    @vstinner
    Copy link
    Member

    Your change makes test_subprocess f1iling on Windows. Example:
    http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/891/steps/test/logs/stdio

    @gpshead
    Copy link
    Member

    gpshead commented Aug 25, 2017

    right, the test needs to be excluded there. fixing... (bummer windows wasn't in the CI)

    @gpshead
    Copy link
    Member

    gpshead commented Sep 4, 2017

    New changeset 1dba378 by Gregory P. Smith in branch '3.6':
    bpo-22536 [3.6] Set filename in FileNotFoundError (bpo-3305)
    1dba378

    @gpshead gpshead added the 3.7 (EOL) end of life label Sep 6, 2017
    @gpshead gpshead closed this as completed Sep 6, 2017
    @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
    3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants