Skip to content

Don't use stdlib private API in animation.py. #12368

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 2, 2018
Merged

Don't use stdlib private API in animation.py. #12368

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 18 additions & 32 deletions lib/matplotlib/animation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import abc
import base64
import contextlib
from io import BytesIO
from io import BytesIO, TextIOWrapper
import itertools
import logging
import os
Expand Down Expand Up @@ -346,12 +346,11 @@ def _run(self):
# movie file. *args* returns the sequence of command line arguments
# from a few configuration options.
command = self._args()
output = subprocess.PIPE
_log.info('MovieWriter.run: running command: %s', command)
self._proc = subprocess.Popen(command, shell=False,
stdout=output, stderr=output,
stdin=subprocess.PIPE,
creationflags=subprocess_creation_flags)
PIPE = subprocess.PIPE
self._proc = subprocess.Popen(
command, stdin=PIPE, stdout=PIPE, stderr=PIPE,
creationflags=subprocess_creation_flags)

def finish(self):
'''Finish any processing for writing the movie.'''
Expand Down Expand Up @@ -394,8 +393,18 @@ def cleanup(self):
'''Clean-up and collect the process used to write the movie file.'''
out, err = self._proc.communicate()
self._frame_sink().close()
_log.debug('MovieWriter -- Command stdout:\n%s', out)
_log.debug('MovieWriter -- Command stderr:\n%s', err)
# 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)
if self._proc.returncode:
raise RuntimeError('Error creating movie, return code {}\n'
'stdout:\n'
'{}\n'
'stderr:\n'
'{}\n'
.format(self._proc.returncode, out, err))

@classmethod
def bin_path(cls):
Expand Down Expand Up @@ -520,19 +529,6 @@ def finish(self):
self._run()
MovieWriter.finish(self) # Will call clean-up

# Check error code for creating file here, since we just run
# the process here, rather than having an open pipe.
if self._proc.returncode:
try:
stdout = [s.decode() for s in self._proc._stdout_buff]
stderr = [s.decode() for s in self._proc._stderr_buff]
_log.info("MovieWriter.finish: stdout: %s", stdout)
_log.info("MovieWriter.finish: stderr: %s", stderr)
except Exception as e:
pass
raise RuntimeError('Error creating movie, return code: {}'
.format(self._proc.returncode))

def cleanup(self):
MovieWriter.cleanup(self)

Expand Down Expand Up @@ -894,17 +890,7 @@ def grab_frame(self, **savefig_kwargs):
else:
return super().grab_frame(**savefig_kwargs)

def _run(self):
# make a duck-typed subprocess stand in
# this is called by the MovieWriter base class, but not used here.
class ProcessStandin(object):
returncode = 0

def communicate(self):
return '', ''

self._proc = ProcessStandin()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is removing this safe? self._run() is still called, I guess it is deferring to a baseclass implementation?

Copy link
Contributor Author

@anntzer anntzer Oct 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parent class (FileMovieWriter) invokes _run in finish:

    def finish(self):
        # Call run here now that all frame grabbing is done. All temp files
        # are available to be assembled.
        self._run()
        MovieWriter.finish(self)  # Will call clean-up

so if we override finish(), we don't call _run() at all.
(TBH I think the inheritance chain of methods could be refactored a bit but that's not the point of this PR.)

Also note that there is actually test coverage for this :)

def finish(self):
# save the frames to an html file
if self.embed_frames:
fill_frames = _embedded_frames(self._saved_frames,
Expand Down