diff --git a/doc/api/next_api_changes/2018-10-02-AL-animation.rst b/doc/api/next_api_changes/2018-10-02-AL-animation.rst new file mode 100644 index 000000000000..def91339751a --- /dev/null +++ b/doc/api/next_api_changes/2018-10-02-AL-animation.rst @@ -0,0 +1,7 @@ +Exception on failing animations changed +``````````````````````````````````````` + +Previously, subprocess failures in the animation framework would raise either +in a `RuntimeError` or a `ValueError` depending on when the error occurred. +They now raise a `subprocess.CalledProcessError` with attributes set as +documented by the exception class. diff --git a/lib/matplotlib/animation.py b/lib/matplotlib/animation.py index b3ed6da0e6a6..770c1a0f955e 100644 --- a/lib/matplotlib/animation.py +++ b/lib/matplotlib/animation.py @@ -364,22 +364,14 @@ def grab_frame(self, **savefig_kwargs): command that saves the figure. ''' _log.debug('MovieWriter.grab_frame: Grabbing frame.') - try: - # re-adjust the figure size in case it has been changed by the - # user. We must ensure that every frame is the same size or - # the movie will not save correctly. - self.fig.set_size_inches(self._w, self._h) - # Tell the figure to save its data to the sink, using the - # frame format and dpi. - self.fig.savefig(self._frame_sink(), format=self.frame_format, - dpi=self.dpi, **savefig_kwargs) - except (RuntimeError, IOError) as e: - out, err = self._proc.communicate() - _log.info('MovieWriter -- Error running proc:\n%s\n%s', out, err) - raise IOError('Error saving animation to file (cause: {0}) ' - 'Stdout: {1} StdError: {2}. It may help to re-run ' - 'with logging level set to ' - 'DEBUG.'.format(e, out, err)) + # re-adjust the figure size in case it has been changed by the + # user. We must ensure that every frame is the same size or + # the movie will not save correctly. + self.fig.set_size_inches(self._w, self._h) + # Tell the figure to save its data to the sink, using the + # frame format and dpi. + self.fig.savefig(self._frame_sink(), format=self.frame_format, + dpi=self.dpi, **savefig_kwargs) def _frame_sink(self): '''Returns the place to which frames should be written.''' @@ -396,15 +388,15 @@ def cleanup(self): # Use the encoding/errors that universal_newlines would use. out = TextIOWrapper(BytesIO(out)).read() err = TextIOWrapper(BytesIO(err)).read() - _log.debug("MovieWriter stdout:\n%s", out) - _log.debug("MovieWriter stderr:\n%s", err) + _log.log( + logging.WARNING if self._proc.returncode else logging.DEBUG, + "MovieWriter stdout:\n%s", out) + _log.log( + logging.WARNING if self._proc.returncode else logging.DEBUG, + "MovieWriter stderr:\n%s", err) if self._proc.returncode: - raise RuntimeError('Error creating movie, return code {}\n' - 'stdout:\n' - '{}\n' - 'stderr:\n' - '{}\n' - .format(self._proc.returncode, out, err)) + raise subprocess.CalledProcessError( + self._proc.returncode, self._proc.args, out, err) @classmethod def bin_path(cls): @@ -511,17 +503,11 @@ def grab_frame(self, **savefig_kwargs): ''' # Overloaded to explicitly close temp file. _log.debug('MovieWriter.grab_frame: Grabbing frame.') - try: - # Tell the figure to save its data to the sink, using the - # frame format and dpi. - with self._frame_sink() as myframesink: - self.fig.savefig(myframesink, format=self.frame_format, - dpi=self.dpi, **savefig_kwargs) - - except RuntimeError: - out, err = self._proc.communicate() - _log.info('MovieWriter -- Error running proc:\n%s\n%s', out, err) - raise + # Tell the figure to save its data to the sink, using the + # frame format and dpi. + with self._frame_sink() as myframesink: + self.fig.savefig(myframesink, format=self.frame_format, + dpi=self.dpi, **savefig_kwargs) def finish(self): # Call run here now that all frame grabbing is done. All temp files diff --git a/lib/matplotlib/tests/test_animation.py b/lib/matplotlib/tests/test_animation.py index eff03936109b..a0cb521885fa 100644 --- a/lib/matplotlib/tests/test_animation.py +++ b/lib/matplotlib/tests/test_animation.py @@ -1,5 +1,6 @@ import os from pathlib import Path +import subprocess import sys import numpy as np @@ -237,13 +238,10 @@ def test_cleanup_temporaries(method_name, tmpdir): assert list(Path(str(tmpdir)).iterdir()) == [] -# Currently, this fails with a ValueError after we try to communicate() twice -# with the Popen. -@pytest.mark.xfail @pytest.mark.skipif(os.name != "posix", reason="requires a POSIX OS") def test_failing_ffmpeg(tmpdir, monkeypatch): """ - Test that we correctly raise an OSError when ffmpeg fails. + Test that we correctly raise a CalledProcessError when ffmpeg fails. To do so, mock ffmpeg using a simple executable shell script that succeeds when called with no arguments (so that it gets registered by @@ -252,12 +250,12 @@ def test_failing_ffmpeg(tmpdir, monkeypatch): try: with tmpdir.as_cwd(): monkeypatch.setenv("PATH", ".:" + os.environ["PATH"]) - exe_path = Path(tmpdir, "ffmpeg") + exe_path = Path(str(tmpdir), "ffmpeg") exe_path.write_text("#!/bin/sh\n" "[[ $@ -eq 0 ]]\n") os.chmod(str(exe_path), 0o755) animation.writers.reset_available_writers() - with pytest.raises(OSError): + with pytest.raises(subprocess.CalledProcessError): make_animation().save("test.mpeg") finally: animation.writers.reset_available_writers()