Surface image errors and warnings from HTML rendering in verifyproblem

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)
 

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()}'

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()

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...')

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('<html>', 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('<html>', full_html)

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('&', '&amp;')
@@ -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):

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 '<script>' not in html
 
 
-def test_no_xss_sample():
+def test_no_xss_sample(diag):
     problem_path = Path(__file__).parent / 'problems' / 'samplexss'
-    html = render(problem_path)
+    html = render(problem_path, diag)
     assert '<script>' not in html