[Python-Dev] [Python-checkins] cpython (merge 3.2 -> 3.3): Fixes Issue #16114: The subprocess module no longer provides a

[Chris Jerdonek]

On Wed, Oct 10, 2012 at 3:52 AM, gregory.p.smith
<python-checkins at python.org> wrote:
> http://hg.python.org/cpython/rev/ee30d7ef70be
> changeset:   79633:ee30d7ef70be
> branch:      3.3
> parent:      79630:9451908da615
> parent:      79632:e938fa6be509
> user:        Gregory P. Smith <greg at krypto.org>
> date:        Wed Oct 10 03:44:47 2012 -0700
> summary:
>   Fixes Issue #16114: The subprocess module no longer provides a
> misleading error message stating that args[0] did not exist when
> either the cwd or executable keyword arguments specified a path that
> did not exist.

> +    def test_exception_cwd(self):
> +        """Test error in the child raised in the parent for a bad cwd."""
> +        desired_exception = self._get_chdir_exception()
>          try:
>              p = subprocess.Popen([sys.executable, "-c", ""],
> -                                 cwd=nonexistent_dir)
> +                                 cwd=self._nonexistent_dir)
>          except OSError as e:
>              # Test that the child process chdir failure actually makes
>              # it up to the parent process as the correct exception.
> @@ -1064,6 +1070,33 @@
>          else:
>              self.fail("Expected OSError: %s" % desired_exception)

I was on this issue but didn't have an opportunity to comment.

It would be cleaner to use the self.assertRaises() pattern here and
also probably better to share code across the three test methods which
are nearly identical to one another (there is a fourth scenario I
would also add of shell=True).

I would also check for FileNotFoundError instead of OSError in the 3.3
and later versions.

--Chris

>
> +    def test_exception_bad_executable(self):
> +        """Test error in the child raised in the parent for a bad executable."""
> +        desired_exception = self._get_chdir_exception()
> +        try:
> +            p = subprocess.Popen([sys.executable, "-c", ""],
> +                                 executable=self._nonexistent_dir)
> +        except OSError as e:
> +            # Test that the child process exec failure actually makes
> +            # it up to the parent process as the correct exception.
> +            self.assertEqual(desired_exception.errno, e.errno)
> +            self.assertEqual(desired_exception.strerror, e.strerror)
> +        else:
> +            self.fail("Expected OSError: %s" % desired_exception)
> +
> +    def test_exception_bad_args_0(self):
> +        """Test error in the child raised in the parent for a bad args[0]."""
> +        desired_exception = self._get_chdir_exception()
> +        try:
> +            p = subprocess.Popen([self._nonexistent_dir, "-c", ""])
> +        except OSError as e:
> +            # Test that the child process exec failure actually makes
> +            # it up to the parent process as the correct exception.
> +            self.assertEqual(desired_exception.errno, e.errno)
> +            self.assertEqual(desired_exception.strerror, e.strerror)
> +        else:
> +            self.fail("Expected OSError: %s" % desired_exception)
> +
>      def test_restore_signals(self):
>          # Code coverage for both values of restore_signals to make sure it
>          # at least does not blow up.
> diff --git a/Misc/NEWS b/Misc/NEWS
> --- a/Misc/NEWS
> +++ b/Misc/NEWS
> @@ -37,6 +37,10 @@
>  Library
>  -------
>
> +- Issue #16114: The subprocess module no longer provides a misleading error
> +  message stating that args[0] did not exist when either the cwd or executable
> +  keyword arguments specified a path that did not exist.
> +
>  - Issue #16169: Fix ctypes.WinError()'s confusion between errno and winerror.
>
>  - Issue #16089: Allow ElementTree.TreeBuilder to work again with a non-Element
> diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
> --- a/Modules/_posixsubprocess.c
> +++ b/Modules/_posixsubprocess.c
> @@ -356,7 +356,7 @@
>             PyObject *preexec_fn,
>             PyObject *preexec_fn_args_tuple)
>  {
> -    int i, saved_errno, unused;
> +    int i, saved_errno, unused, reached_preexec = 0;
>      PyObject *result;
>      const char* err_msg = "";
>      /* Buffer large enough to hold a hex integer.  We can't malloc. */
> @@ -440,6 +440,7 @@
>          POSIX_CALL(setsid());
>  #endif
>
> +    reached_preexec = 1;
>      if (preexec_fn != Py_None && preexec_fn_args_tuple) {
>          /* This is where the user has asked us to deadlock their program. */
>          result = PyObject_Call(preexec_fn, preexec_fn_args_tuple, NULL);
> @@ -489,6 +490,10 @@
>          }
>          unused = write(errpipe_write, cur, hex_errno + sizeof(hex_errno) - cur);
>          unused = write(errpipe_write, ":", 1);
> +        if (!reached_preexec) {
> +            /* Indicate to the parent that the error happened before exec(). */
> +            unused = write(errpipe_write, "noexec", 6);
> +        }
>          /* We can't call strerror(saved_errno).  It is not async signal safe.
>           * The parent process will look the error message up. */
>      } else {
>
> --
> Repository URL: http://hg.python.org/cpython
>
> _______________________________________________
> Python-checkins mailing list
> Python-checkins at python.org
> http://mail.python.org/mailman/listinfo/python-checkins
>