subprocess should include filename in FileNotFoundError exception

[charpov]

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]

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

[travisthieman]

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]

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]

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]

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]

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]

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]

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]

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]

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

[vstinner]

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]

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

[gpshead]

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