From 18de7d421008eb62b1d9fa2f7e2bd3b59001f752 Mon Sep 17 00:00:00 2001 From: Gunnar Kreitz Date: Wed, 2 Sep 2026 10:31:27 +0200 Subject: [PATCH] Detect silent gs failures. Surface image errors and warnings from HTML rendering in verifyproblem --- problemtools/ProblemPlasTeX/__init__.py | 25 +++++++++++-------- problemtools/checks/statements.py | 2 +- problemtools/problem2html.py | 32 +++++++++++++++---------- problemtools/tex2html.py | 5 ++-- tests/test_latex.py | 8 +++---- tests/test_markdown.py | 18 +++++++------- tests/test_xss.py | 16 ++++++------- 7 files changed, 59 insertions(+), 47 deletions(-) diff --git a/problemtools/ProblemPlasTeX/__init__.py b/problemtools/ProblemPlasTeX/__init__.py index 65e5ddeb..2aec6fbd 100644 --- a/problemtools/ProblemPlasTeX/__init__.py +++ b/problemtools/ProblemPlasTeX/__init__.py @@ -6,13 +6,11 @@ from plasTeX.Filenames import Filenames from plasTeX.Imagers import Image -from plasTeX.Logging import getLogger from plasTeX.Renderers.PageTemplate import Renderer +from problemtools.diagnostics import Diagnostics from problemtools.template import TemplateError -log = getLogger() - # Adapted from plasTeX.Imagers.Imager class class ImageConverter: @@ -25,9 +23,10 @@ class ImageConverter: '.pdf': ('.png', ['gs', '-dUseCropBox', '-sDEVICE=pngalpha', '-r300', '-o']) } - def __init__(self, document: Any) -> None: + def __init__(self, document: Any, diag: Diagnostics) -> None: self.config = document.config self.ownerDocument = document + self.diag = diag # Cache of already seen images self.staticimages: dict[str, Image] = {} @@ -47,7 +46,7 @@ def close(self) -> None: def getImage(self, node: Any) -> Image | None: name = getattr(node, 'imageoverride', None) if name is None: - log.error(f'Image handler called for non-image node "{node.source}"') + self.diag.error(f'Image handler called for non-image node "{node.source}"') return None if name in self.staticimages: @@ -66,9 +65,11 @@ def getImage(self, node: Any) -> Image | None: path = os.path.splitext(path)[0] + newext cmd = self.imageConversion[oldext][1] + [path, name] result = subprocess.run(cmd, stdout=subprocess.DEVNULL, stderr=subprocess.PIPE, check=False) - if result.returncode: - log.error( - 'Failed to convert %s image "%s" to %s.\n%s', oldext, name, newext, result.stderr.decode(errors='replace') + # We need to check if output exists. gs can fail with exit code 0, e.g., if the pdf is empty (0 bytes). + if result.returncode or not os.path.isfile(path): + self.diag.error( + f'Failed to convert {oldext} image "{os.path.basename(name)}" to {newext}.', + f'Command: {" ".join(cmd)}\nStderr: {result.stderr.decode(errors="replace")}', ) else: # Just copy it @@ -79,7 +80,7 @@ def getImage(self, node: Any) -> Image | None: return img except Exception as msg: - log.warning(f'{msg} in image "{name}".') + self.diag.error(f'{msg} in image "{name}".') return None @@ -93,6 +94,10 @@ class ProblemRenderer(Renderer): imageTypes: list[str] = ['.png', '.jpg', '.jpeg', '.gif'] # noqa: RUF012 vectorImageTypes: list[str] = ['.svg'] # noqa: RUF012 + def __init__(self, diag: Diagnostics) -> None: + super().__init__() + self.diag = diag + def render(self, document: Any, postProcess: Any = None) -> None: templatepaths = [ os.path.join(os.path.dirname(__file__), '../templates/html'), @@ -117,7 +122,7 @@ def render(self, document: Any, postProcess: Any = None) -> None: f.invalid.clear() # Setup our own mini-imager which just does copying and converts pdfs to png - self.imager = ImageConverter(document) + self.imager = ImageConverter(document, self.diag) Renderer.render(self, document) diff --git a/problemtools/checks/statements.py b/problemtools/checks/statements.py index 34cbc771..9ecb60c3 100644 --- a/problemtools/checks/statements.py +++ b/problemtools/checks/statements.py @@ -83,7 +83,7 @@ def _latex_heuristic(name: str) -> bool: options.destdir = os.path.join(work_dir, 'html') options.language = lang options.quiet = True - problem2html.convert(options, file) + problem2html.convert(options, diag, file) except Exception as e: diag.error( f'Could not convert problem statement to html for language "{lang}". Run problem2html --language {lang} on the problem to diagnose.\n{e}\n{traceback.format_exc()}' diff --git a/problemtools/problem2html.py b/problemtools/problem2html.py index 6f689ecb..1652eda9 100644 --- a/problemtools/problem2html.py +++ b/problemtools/problem2html.py @@ -1,5 +1,6 @@ #! /usr/bin/env python3 import argparse +import logging import os.path import re import string @@ -8,10 +9,11 @@ from pathlib import Path from . import md2html, statement_util, tex2html +from .diagnostics import Diagnostics, LoggingDiagnostics from .version import add_version_arg -def convert(options: argparse.Namespace, force_statement_file: Path | None = None) -> None: +def convert(options: argparse.Namespace, diag: Diagnostics, force_statement_file: Path | None = None) -> None: problem_root = Path(options.problem).resolve(strict=True) if force_statement_file: # Used by verifyproblem to test rendering even if there are multiple statements in a language @@ -36,7 +38,7 @@ def convert(options: argparse.Namespace, force_statement_file: Path | None = Non case '.md': md2html.convert(problem_root, options, statement_file) case '.tex': - tex2html.convert(problem_root, options, statement_file) + tex2html.convert(problem_root, options, statement_file, diag) case _: raise NotImplementedError('Unsupported file type, expected md or tex: {statement_file.name}') @@ -45,18 +47,16 @@ def convert(options: argparse.Namespace, force_statement_file: Path | None = Non try: subprocess.call(['tidy', '-utf8', '-i', '-q', '-m', destfile], stderr=devnull) except OSError: - if not options.quiet: - print("Warning: Command 'tidy' not found. Install tidy or run with --messy") + diag.warning("Command 'tidy' not found. Install tidy or run with --messy") # identify any large generated files (especially images) - if not options.quiet: - for path, _dirs, files in os.walk('.'): - for f in files: - file_size_kib = os.stat(os.path.join(path, f)).st_size // 1024 - if file_size_kib > 1024: - print(f'WARNING: FILE {f} HAS SIZE {file_size_kib} KiB; CONSIDER REDUCING IT') - elif file_size_kib > 300: - print(f'Warning: file {f} has size {file_size_kib} KiB; consider reducing it') + for path, _dirs, files in os.walk('.'): + for f in files: + file_size_kib = os.stat(os.path.join(path, f)).st_size // 1024 + if file_size_kib > 1024: + diag.warning(f'FILE {f} HAS SIZE {file_size_kib} KiB; CONSIDER REDUCING IT') + elif file_size_kib > 300: + diag.warning(f'File {f} has size {file_size_kib} KiB; consider reducing it') if options.bodyonly: content = Path(destfile).read_text(encoding='utf-8') @@ -94,6 +94,7 @@ def get_parser() -> argparse.ArgumentParser: parser.add_argument( '-L', '--log-level', dest='loglevel', help='set log level (debug, info, warning, error, critical)', default='warning' ) + # Quiet is basically a no-op now, supersceded by --log-level. Should probably be dropped at some point parser.add_argument('-q', '--quiet', dest='quiet', action='store_true', help='quiet', default=False) parser.add_argument('-i', '--imgbasedir', dest='imgbasedir', default='') parser.add_argument('problem', help='the problem to convert') @@ -105,12 +106,17 @@ def get_parser() -> argparse.ArgumentParser: def main() -> None: parser = get_parser() options = parser.parse_args() + diag = LoggingDiagnostics.create('problem2html', log_level=getattr(logging, options.loglevel.upper())) try: - convert(options) + convert(options, diag) except Exception as e: print(e) sys.exit(1) + if diag.errors: + print(f'{diag.errors} errors and {diag.warnings} warnings') + sys.exit(1) + if __name__ == '__main__': main() diff --git a/problemtools/tex2html.py b/problemtools/tex2html.py index f9f9f077..df5549ea 100644 --- a/problemtools/tex2html.py +++ b/problemtools/tex2html.py @@ -5,9 +5,10 @@ from pathlib import Path from . import template +from .diagnostics import Diagnostics -def convert(problem_root: Path, options: argparse.Namespace, statement_file: Path) -> None: +def convert(problem_root: Path, options: argparse.Namespace, statement_file: Path, diag: Diagnostics) -> None: # PlasTeX.Logging statically overwrites logging and formatting, so delay loading import plasTeX.Logging import plasTeX.TeX @@ -44,7 +45,7 @@ def convert(problem_root: Path, options: argparse.Namespace, statement_file: Pat # tell plasTeX where to search for problemtools' built-in packages tex.ownerDocument.config['general']['packages-dirs'] = [os.path.join(os.path.dirname(__file__), 'ProblemPlasTeX')] - renderer = ProblemRenderer() + renderer = ProblemRenderer(diag) if not options.quiet: print('Parsing TeX source...') diff --git a/tests/test_latex.py b/tests/test_latex.py index fcacf13e..41484a12 100644 --- a/tests/test_latex.py +++ b/tests/test_latex.py @@ -29,13 +29,13 @@ def test_pdf_render_problem2pdf(): assert temp_file.read(5) == b'%PDF-', 'Output header does not look like a PDF.' -def test_html_render_different(): +def test_html_render_different(diag): # Same options as typical problem2html usage with tempfile.TemporaryDirectory() as temp_dir: problem_path = Path(__file__).parent / '..' / 'examples' / 'different' temp_dir = Path(temp_dir) / 'different_html' options = problem2html.get_parser().parse_args(['-d', str(temp_dir), '-l', 'en', '-q', str(problem_path.resolve())]) - problem2html.convert(options) + problem2html.convert(options, diag) with open(temp_dir / 'index.html', 'r') as temp_file: full_html = temp_file.read() assert re.search('', full_html) @@ -45,13 +45,13 @@ def test_html_render_different(): assert re.search('71293781758123 72784', full_html) # part of sample -def test_html_render_guess(): +def test_html_render_guess(diag): # Same options as typical problem2html usage with tempfile.TemporaryDirectory() as temp_dir: problem_path = Path(__file__).parent / '..' / 'examples' / 'guess' temp_dir = Path(temp_dir) / 'guess_html' options = problem2html.get_parser().parse_args(['-d', str(temp_dir), '-l', 'en', '-q', str(problem_path.resolve())]) - problem2html.convert(options) + problem2html.convert(options, diag) with open(temp_dir / 'index.html', 'r') as temp_file: full_html = temp_file.read() assert re.search('', full_html) diff --git a/tests/test_markdown.py b/tests/test_markdown.py index 4382be48..71e48521 100644 --- a/tests/test_markdown.py +++ b/tests/test_markdown.py @@ -15,9 +15,9 @@ # problem2pdf.convert(args) -def test_sample_escaping(): +def test_sample_escaping(diag): problem_path = Path(__file__).parent / 'problems' / 'specialcharacterssample' - html = render(problem_path) + html = render(problem_path, diag) all_printable = r"""0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~""" # We escape &, < and > all_printable = all_printable.replace('&', '&') @@ -26,32 +26,32 @@ def test_sample_escaping(): assert all_printable in html -def test_footnotes(): +def test_footnotes(diag): # We always want footnotes to be at the bottom # When we insert samples, we need to insert them right above the first footnote # To do this, we search for a string (very fragile) problem_path = Path(__file__).parent / 'problems' / 'footnote' - html = render(problem_path) + html = render(problem_path, diag) assert find_footnotes(html) is not None problem_path = Path(__file__).parent / 'problems' / 'twofootnotes' - html = render(problem_path) + html = render(problem_path, diag) assert find_footnotes(html) is not None -def test_footnotes_href(): +def test_footnotes_href(diag): # We use allowlist-based id values for footnotes. Ensure they have not changed problem_path = Path(__file__).parent / 'problems' / 'footnote' - html = render(problem_path) + html = render(problem_path, diag) assert 'fn1' in html and 'fnref1' in html -def test_invalid_image_throws(): +def test_invalid_image_throws(diag): # If images can point to img that doesn't exist, it's arbitrary web request for problem in ('imgrequest', 'imgrequest2'): problem_path = Path(__file__).parent / 'problems' / problem with pytest.raises(ValueError): - render(problem_path) + render(problem_path, diag) # Pandoc won't make a web request for imgrequest2 with pytest.raises(ValueError): diff --git a/tests/test_xss.py b/tests/test_xss.py index 67b40258..597ab468 100644 --- a/tests/test_xss.py +++ b/tests/test_xss.py @@ -5,12 +5,12 @@ from problemtools import problem2html, problem2pdf -def render(problem_path): +def render(problem_path, diag): with tempfile.TemporaryDirectory() as temp_dir: args, _unknown = problem2html.get_parser().parse_known_args( ['--problem', str(problem_path.resolve()), '--dest-dir', str(temp_dir)] ) - problem2html.convert(args) + problem2html.convert(args, diag) with open(f'{temp_dir}/index.html', 'r') as f: html = f.read() return html @@ -23,19 +23,19 @@ def renderpdf(problem_path): problem2pdf.convert(args) -def test_no_xss_statement(): +def test_no_xss_statement(diag): problem_path = Path(__file__).parent / 'problems' / 'statementxss' - html = render(problem_path) + html = render(problem_path, diag) assert 'alert' not in html -def test_no_xss_problemname(): +def test_no_xss_problemname(diag): problem_path = Path(__file__).parent / 'problems' / 'problemnamexss' - html = render(problem_path) + html = render(problem_path, diag) assert '