From 409511bec08843cad0513920da7d5e7a3f9c61c6 Mon Sep 17 00:00:00 2001 From: Jonathan Yong Date: Sat, 26 Jun 2021 22:03:27 +0100 Subject: [PATCH 1/4] Check modification times of included files in plot_directive Fixes #17860. --- lib/matplotlib/sphinxext/plot_directive.py | 46 ++++++++++++++++++---- 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/lib/matplotlib/sphinxext/plot_directive.py b/lib/matplotlib/sphinxext/plot_directive.py index 890eca262580..f0d4e4331410 100644 --- a/lib/matplotlib/sphinxext/plot_directive.py +++ b/lib/matplotlib/sphinxext/plot_directive.py @@ -429,14 +429,23 @@ def filenames(self): return [self.filename(fmt) for fmt in self.formats] -def out_of_date(original, derived): +def out_of_date(original, derived, includes=None): """ - Return whether *derived* is out-of-date relative to *original*, both of - which are full file paths. + Return whether *derived* is out-of-date relative to *original* or any of + the RST files included in it using the RST include directive (*includes*). + *derived* and *original* are full paths, and *includes* is optionally a + list of full paths which may have been included in the *original*. """ - return (not os.path.exists(derived) or - (os.path.exists(original) and - os.stat(derived).st_mtime < os.stat(original).st_mtime)) + if includes is None: + includes = [] + files_to_check = [original, *includes] + + def out_of_date_one(original, derived): + return (not os.path.exists(derived) or + (os.path.exists(original) and + os.stat(derived).st_mtime < os.stat(original).st_mtime)) + + return any(out_of_date_one(f, derived) for f in files_to_check) class PlotError(RuntimeError): @@ -532,7 +541,8 @@ def get_plot_formats(config): def render_figures(code, code_path, output_dir, output_base, context, function_name, config, context_reset=False, - close_figs=False): + close_figs=False, + code_includes=None): """ Run a pyplot script and save the images in *output_dir*. @@ -742,6 +752,25 @@ def run(arguments, content, options, state_machine, state, lineno): build_dir_link = build_dir source_link = dest_dir_link + '/' + output_base + source_ext + # get list of included rst files so that the output is updated when any + # plots in the included files change. These attributes are modified by the + # include directive (see the docutils.parsers.rst.directives.misc module). + try: + source_file_includes = [os.path.join(os.getcwd(), t[0]) + for t in state.document.include_log] + except AttributeError: + # the document.include_log attribute only exists in docutils >=0.17, + # before that we need to inspect the state machine + possible_sources = [os.path.join(setup.confdir, t[0]) + for t in state_machine.input_lines.items] + source_file_includes = [f for f in set(possible_sources) + if os.path.isfile(f)] + # remove the source file itself from the includes + try: + source_file_includes.remove(source_file_name) + except ValueError: + pass + # make figures try: results = render_figures(code, @@ -752,7 +781,8 @@ def run(arguments, content, options, state_machine, state, lineno): function_name, config, context_reset=context_opt == 'reset', - close_figs=context_opt == 'close-figs') + close_figs=context_opt == 'close-figs', + code_includes=source_file_includes) errors = [] except PlotError as err: reporter = state.memo.reporter From 608a868090882db2adffd25f4b2e8bd96b410269 Mon Sep 17 00:00:00 2001 From: Jonathan Yong Date: Sat, 26 Jun 2021 22:04:15 +0100 Subject: [PATCH 2/4] Always assume plots with context are out of date Fixes #20523. --- lib/matplotlib/sphinxext/plot_directive.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/matplotlib/sphinxext/plot_directive.py b/lib/matplotlib/sphinxext/plot_directive.py index f0d4e4331410..b98215fa5866 100644 --- a/lib/matplotlib/sphinxext/plot_directive.py +++ b/lib/matplotlib/sphinxext/plot_directive.py @@ -559,7 +559,8 @@ def render_figures(code, code_path, output_dir, output_base, context, all_exists = True img = ImageFile(output_base, output_dir) for format, dpi in formats: - if out_of_date(code_path, img.filename(format)): + if context or out_of_date(code_path, img.filename(format), + includes=code_includes): all_exists = False break img.formats.append(format) @@ -579,7 +580,8 @@ def render_figures(code, code_path, output_dir, output_base, context, else: img = ImageFile('%s_%02d' % (output_base, j), output_dir) for fmt, dpi in formats: - if out_of_date(code_path, img.filename(fmt)): + if context or out_of_date(code_path, img.filename(fmt), + includes=code_includes): all_exists = False break img.formats.append(fmt) From 31ffe4bbc926eb119b1f9b2282733dce6c725b8c Mon Sep 17 00:00:00 2001 From: Jonathan Yong Date: Sat, 26 Jun 2021 16:38:40 +0100 Subject: [PATCH 3/4] Add rebuild tests for plot_directive The new tests: - make sure that unchanged plots are not recreated upon a second invocation of sphinx-build - make sure that plots with `:context:` are recreated (#20523) - make sure that plots included via `.. include:` are updated when modified (#17860) --- lib/matplotlib/tests/test_sphinxext.py | 72 ++++++++++++++----- .../tests/tinypages/included_plot_21.rst | 6 ++ lib/matplotlib/tests/tinypages/some_plots.rst | 5 ++ 3 files changed, 67 insertions(+), 16 deletions(-) create mode 100644 lib/matplotlib/tests/tinypages/included_plot_21.rst diff --git a/lib/matplotlib/tests/test_sphinxext.py b/lib/matplotlib/tests/test_sphinxext.py index 5d48a9817c83..490e7ece2927 100644 --- a/lib/matplotlib/tests/test_sphinxext.py +++ b/lib/matplotlib/tests/test_sphinxext.py @@ -5,6 +5,7 @@ from pathlib import Path from subprocess import Popen, PIPE import sys +import shutil import pytest @@ -13,27 +14,21 @@ def test_tinypages(tmpdir): - tmp_path = Path(tmpdir) - html_dir = tmp_path / 'html' - doctree_dir = tmp_path / 'doctrees' - # Build the pages with warnings turned into errors - cmd = [sys.executable, '-msphinx', '-W', '-b', 'html', - '-d', str(doctree_dir), - str(Path(__file__).parent / 'tinypages'), str(html_dir)] - proc = Popen(cmd, stdout=PIPE, stderr=PIPE, universal_newlines=True, - env={**os.environ, "MPLBACKEND": ""}) - out, err = proc.communicate() + source_dir = Path(tmpdir) / 'src' + shutil.copytree(str(Path(__file__).parent / 'tinypages'), str(source_dir)) + html_dir = source_dir / '_build' / 'html' + doctree_dir = source_dir / 'doctrees' - assert proc.returncode == 0, \ - f"sphinx build failed with stdout:\n{out}\nstderr:\n{err}\n" - if err: - pytest.fail(f"sphinx build emitted the following warnings:\n{err}") - - assert html_dir.is_dir() + # Build the pages with warnings turned into errors + build_sphinx_html(source_dir, doctree_dir, html_dir) def plot_file(num): return html_dir / f'some_plots-{num}.png' + def plot_directive_file(num): + # This is always next to the doctree dir. + return doctree_dir.parent / 'plot_directive' / f'some_plots-{num}.png' + range_10, range_6, range_4 = [plot_file(i) for i in range(1, 4)] # Plot 5 is range(6) plot assert filecmp.cmp(range_6, plot_file(5)) @@ -48,6 +43,7 @@ def plot_file(num): assert filecmp.cmp(range_4, plot_file(13)) # Plot 14 has included source html_contents = (html_dir / 'some_plots.html').read_bytes() + assert b'# Only a comment' in html_contents # check plot defined in external file. assert filecmp.cmp(range_4, html_dir / 'range4.png') @@ -62,3 +58,47 @@ def plot_file(num): assert b'plot-directive my-class my-other-class' in html_contents # check that the multi-image caption is applied twice assert html_contents.count(b'This caption applies to both plots.') == 2 + # Plot 21 is range(6) plot via an include directive. But because some of + # the previous plots are repeated, the argument to plot_file() is only 17. + assert filecmp.cmp(range_6, plot_file(17)) + + # Modify the included plot + with open(str(source_dir / 'included_plot_21.rst'), 'r') as file: + contents = file.read() + contents = contents.replace('plt.plot(range(6))', 'plt.plot(range(4))') + with open(str(source_dir / 'included_plot_21.rst'), 'w') as file: + file.write(contents) + # Build the pages again and check that the modified file was updated + modification_times = [plot_directive_file(i).stat().st_mtime + for i in (1, 2, 3, 5)] + build_sphinx_html(source_dir, doctree_dir, html_dir) + assert filecmp.cmp(range_4, plot_file(17)) + # Check that the plots in the plot_directive folder weren't changed. + # (plot_directive_file(1) won't be modified, but it will be copied to html/ + # upon compilation, so plot_file(1) will be modified) + assert plot_directive_file(1).stat().st_mtime == modification_times[0] + assert plot_directive_file(2).stat().st_mtime == modification_times[1] + assert plot_directive_file(3).stat().st_mtime == modification_times[2] + assert filecmp.cmp(range_10, plot_file(1)) + assert filecmp.cmp(range_6, plot_file(2)) + assert filecmp.cmp(range_4, plot_file(3)) + # Make sure that figures marked with context are re-created (but that the + # contents are the same) + assert plot_directive_file(5).stat().st_mtime > modification_times[3] + assert filecmp.cmp(range_6, plot_file(5)) + + +def build_sphinx_html(source_dir, doctree_dir, html_dir): + # Build the pages with warnings turned into errors + cmd = [sys.executable, '-msphinx', '-W', '-b', 'html', + '-d', str(doctree_dir), str(source_dir), str(html_dir)] + proc = Popen(cmd, stdout=PIPE, stderr=PIPE, universal_newlines=True, + env={**os.environ, "MPLBACKEND": ""}) + out, err = proc.communicate() + + assert proc.returncode == 0, \ + f"sphinx build failed with stdout:\n{out}\nstderr:\n{err}\n" + if err: + pytest.fail(f"sphinx build emitted the following warnings:\n{err}") + + assert html_dir.is_dir() diff --git a/lib/matplotlib/tests/tinypages/included_plot_21.rst b/lib/matplotlib/tests/tinypages/included_plot_21.rst new file mode 100644 index 000000000000..761beae6c02d --- /dev/null +++ b/lib/matplotlib/tests/tinypages/included_plot_21.rst @@ -0,0 +1,6 @@ +Plot 21 has length 6 + +.. plot:: + + plt.plot(range(6)) + diff --git a/lib/matplotlib/tests/tinypages/some_plots.rst b/lib/matplotlib/tests/tinypages/some_plots.rst index bab58fd3a8c2..3cdfe39c53a5 100644 --- a/lib/matplotlib/tests/tinypages/some_plots.rst +++ b/lib/matplotlib/tests/tinypages/some_plots.rst @@ -166,3 +166,8 @@ scenario: plt.figure() plt.plot(range(4)) + +Plot 21 is generated via an include directive: + +.. include:: included_plot_21.rst + From 829f92e427e2f28e728a0c9f89582ea4a99fce27 Mon Sep 17 00:00:00 2001 From: Jonathan Yong Date: Tue, 6 Jul 2021 22:57:35 +0100 Subject: [PATCH 4/4] apply suggestions from code review Co-authored with @QuLogic Co-authored-by: Elliott Sales de Andrade --- lib/matplotlib/sphinxext/plot_directive.py | 19 +++++++++++-------- lib/matplotlib/tests/test_sphinxext.py | 10 ++++------ 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/lib/matplotlib/sphinxext/plot_directive.py b/lib/matplotlib/sphinxext/plot_directive.py index b98215fa5866..013a494269df 100644 --- a/lib/matplotlib/sphinxext/plot_directive.py +++ b/lib/matplotlib/sphinxext/plot_directive.py @@ -436,16 +436,19 @@ def out_of_date(original, derived, includes=None): *derived* and *original* are full paths, and *includes* is optionally a list of full paths which may have been included in the *original*. """ + if not os.path.exists(derived): + return True + if includes is None: includes = [] files_to_check = [original, *includes] - def out_of_date_one(original, derived): - return (not os.path.exists(derived) or - (os.path.exists(original) and - os.stat(derived).st_mtime < os.stat(original).st_mtime)) + def out_of_date_one(original, derived_mtime): + return (os.path.exists(original) and + derived_mtime < os.stat(original).st_mtime) - return any(out_of_date_one(f, derived) for f in files_to_check) + derived_mtime = os.stat(derived).st_mtime + return any(out_of_date_one(f, derived_mtime) for f in files_to_check) class PlotError(RuntimeError): @@ -763,9 +766,9 @@ def run(arguments, content, options, state_machine, state, lineno): except AttributeError: # the document.include_log attribute only exists in docutils >=0.17, # before that we need to inspect the state machine - possible_sources = [os.path.join(setup.confdir, t[0]) - for t in state_machine.input_lines.items] - source_file_includes = [f for f in set(possible_sources) + possible_sources = {os.path.join(setup.confdir, t[0]) + for t in state_machine.input_lines.items} + source_file_includes = [f for f in possible_sources if os.path.isfile(f)] # remove the source file itself from the includes try: diff --git a/lib/matplotlib/tests/test_sphinxext.py b/lib/matplotlib/tests/test_sphinxext.py index 490e7ece2927..c0aeb3387df0 100644 --- a/lib/matplotlib/tests/test_sphinxext.py +++ b/lib/matplotlib/tests/test_sphinxext.py @@ -3,9 +3,9 @@ import filecmp import os from pathlib import Path +import shutil from subprocess import Popen, PIPE import sys -import shutil import pytest @@ -15,7 +15,7 @@ def test_tinypages(tmpdir): source_dir = Path(tmpdir) / 'src' - shutil.copytree(str(Path(__file__).parent / 'tinypages'), str(source_dir)) + shutil.copytree(Path(__file__).parent / 'tinypages', source_dir) html_dir = source_dir / '_build' / 'html' doctree_dir = source_dir / 'doctrees' @@ -63,11 +63,9 @@ def plot_directive_file(num): assert filecmp.cmp(range_6, plot_file(17)) # Modify the included plot - with open(str(source_dir / 'included_plot_21.rst'), 'r') as file: - contents = file.read() + contents = (source_dir / 'included_plot_21.rst').read_text() contents = contents.replace('plt.plot(range(6))', 'plt.plot(range(4))') - with open(str(source_dir / 'included_plot_21.rst'), 'w') as file: - file.write(contents) + (source_dir / 'included_plot_21.rst').write_text(contents) # Build the pages again and check that the modified file was updated modification_times = [plot_directive_file(i).stat().st_mtime for i in (1, 2, 3, 5)]