Merge branch 'mp-saveXML'

Author: chrissimpkins

lib/fdiff/__main__.py

@@ -59,6 +59,9 @@ def run(argv):
     )
     parser.add_argument("--head", type=int, help="Display first n lines of output")
     parser.add_argument("--tail", type=int, help="Display last n lines of output")
+    parser.add_argument(
+        "--nomp", action="store_true", help="Do not use multi process optimizations"
+    )
     parser.add_argument("PREFILE", help="Font file path 1")
     parser.add_argument("POSTFILE", help="Font file path 2")
 
@@ -113,6 +116,10 @@ def run(argv):
     include_list = get_tables_argument_list(args.include)
     exclude_list = get_tables_argument_list(args.exclude)
 
+    # flip logic of the command line flag for multi process
+    # optimizations for use as a u_diff function argument
+    use_mp = not args.nomp
+
     # perform the unified diff analysis
     try:
         diff = u_diff(
@@ -121,6 +128,7 @@ def run(argv):
             context_lines=args.lines,
             include_tables=include_list,
             exclude_tables=exclude_list,
+            use_multiprocess=use_mp,
         )
     except Exception as e:
         sys.stderr.write(f"[*] ERROR: {e}{os.linesep}")
@@ -136,8 +144,17 @@ def run(argv):
         iterable = diff
 
     # print unified diff results to standard output stream
+    has_diff = False
     if args.color:
         for line in iterable:
+            has_diff = True
             sys.stdout.write(color_unified_diff_line(line))
     else:
-        sys.stdout.writelines(iterable)
+        for line in iterable:
+            has_diff = True
+            sys.stdout.write(line)
+
+    # if no difference was found, tell the user instead of
+    # simply closing with zero exit status code.
+    if not has_diff:
+        print("[*] There is no difference between the files.")

lib/fdiff/diff.py

@@ -1,5 +1,6 @@
 import asyncio
 import os
+from multiprocessing import Pool, cpu_count
 import tempfile
 
 from fontTools.ttLib import TTFont
@@ -13,8 +14,19 @@
 from fdiff.utils import get_file_modtime
 
 
+def _ttfont_save_xml(ttf, filepath, include_tables, exclude_tables):
+    """Writes TTX specification formatted XML to disk on filepath."""
+    ttf.saveXML(filepath, tables=include_tables, skipTables=exclude_tables)
+    return True
+
+
 def u_diff(
-    filepath_a, filepath_b, context_lines=3, include_tables=None, exclude_tables=None
+    filepath_a,
+    filepath_b,
+    context_lines=3,
+    include_tables=None,
+    exclude_tables=None,
+    use_multiprocess=True,
 ):
     """Performs a unified diff on a TTX serialized data format dump of font binary data using
     a modified version of the Python standard libary difflib module.
@@ -105,20 +117,30 @@ def u_diff(
         fromdate = get_file_modtime(prepath)
         todate = get_file_modtime(postpath)
 
-        tt_left.saveXML(
-            os.path.join(tmpdirname, "left.ttx"),
-            tables=include_tables,
-            skipTables=exclude_tables,
-        )
-        tt_right.saveXML(
-            os.path.join(tmpdirname, "right.ttx"),
-            tables=include_tables,
-            skipTables=exclude_tables,
-        )
+        left_ttxpath = os.path.join(tmpdirname, "left.ttx")
+        right_ttxpath = os.path.join(tmpdirname, "right.ttx")
+
+        if use_multiprocess and cpu_count() > 1:
+            # Use parallel fontTools.ttLib.TTFont.saveXML dump
+            # by default on multi CPU systems.  This is a performance
+            # optimization. Profiling demonstrates that this can reduce
+            # execution time by up to 30% for some fonts
+            mp_args_list = [
+                (tt_left, left_ttxpath, include_tables, exclude_tables),
+                (tt_right, right_ttxpath, include_tables, exclude_tables),
+            ]
+            with Pool(processes=2) as pool:
+                pool.starmap(_ttfont_save_xml, mp_args_list)
+        else:
+            # use sequential fontTools.ttLib.TTFont.saveXML dumps
+            # when use_multiprocess is False or single CPU system
+            # detected
+            _ttfont_save_xml(tt_left, left_ttxpath, include_tables, exclude_tables)
+            _ttfont_save_xml(tt_right, right_ttxpath, include_tables, exclude_tables)
 
-        with open(os.path.join(tmpdirname, "left.ttx")) as ff:
+        with open(left_ttxpath) as ff:
             fromlines = ff.readlines()
-        with open(os.path.join(tmpdirname, "right.ttx")) as tf:
+        with open(right_ttxpath) as tf:
             tolines = tf.readlines()
 
         return unified_diff(

tests/test_diff.py

@@ -1,8 +1,11 @@
 import os
+import tempfile
 
 import pytest
 
-from fdiff.diff import u_diff
+from fontTools.ttLib import TTFont
+
+from fdiff.diff import u_diff, _ttfont_save_xml
 from fdiff.exceptions import AIOError
 
 ROBOTO_BEFORE_PATH = os.path.join("tests", "testfiles", "Roboto-Regular.subset1.ttf")
@@ -44,6 +47,12 @@
     ROBOTO_UDIFF_EXCLUDE_HEADPOST_EXPECTED = robo_udiff_ex_headpost.read()
 
 
+def test_unified_diff_default_no_diff():
+    res = u_diff(ROBOTO_BEFORE_PATH, ROBOTO_BEFORE_PATH)
+    res_string = "".join(res)
+    assert res_string == ""
+
+
 def test_unified_diff_default():
     res = u_diff(ROBOTO_BEFORE_PATH, ROBOTO_AFTER_PATH)
     res_string = "".join(res)
@@ -63,6 +72,25 @@ def test_unified_diff_default():
             assert line == expected_string_list[x]
 
 
+def test_unified_diff_without_mp_optimizations():
+    res = u_diff(ROBOTO_BEFORE_PATH, ROBOTO_AFTER_PATH, use_multiprocess=False)
+    res_string = "".join(res)
+    res_string_list = res_string.split("\n")
+    expected_string_list = ROBOTO_UDIFF_EXPECTED.split("\n")
+
+    # have to handle the tests for the top two file path lines
+    # differently than the rest of the comparisons because
+    # the time is defined using local platform settings
+    # which makes tests fail on remote CI testing services vs.
+    # my local testing platform...
+    for x, line in enumerate(res_string_list):
+        # treat top two lines of the diff as comparison of first 10 chars only
+        if x in (0, 1):
+            assert line[0:9] == expected_string_list[x][0:9]
+        else:
+            assert line == expected_string_list[x]
+
+
 def test_unified_diff_context_lines_1():
     res = u_diff(ROBOTO_BEFORE_PATH, ROBOTO_AFTER_PATH, context_lines=1)
     res_string = "".join(res)
@@ -210,4 +238,24 @@ def test_unified_diff_remote_404_first_file():
 
 def test_unified_diff_remote_404_second_file():
     with pytest.raises(AIOError):
-        u_diff(ROBOTO_BEFORE_URL, URL_404)
+        u_diff(ROBOTO_BEFORE_URL, URL_404)
+
+
+def test_unified_diff_remote_non_url_exception():
+    """This raises an exception in the aiohttp get request call"""
+    with pytest.raises(AIOError):
+        u_diff("https:bogus", "https:bogus")
+
+
+#
+#
+#  Private functions
+#
+#
+
+def test_ttfont_save_xml():
+    tt = TTFont(ROBOTO_BEFORE_PATH)
+    with tempfile.TemporaryDirectory() as tmpdirname:
+        path = os.path.join(tmpdirname, "test.ttx")
+        _ttfont_save_xml(tt, path, None, None)
+        assert os.path.isfile(path)

tests/test_main.py

@@ -92,6 +92,15 @@ def test_main_include_exclude_defined_simultaneously(capsys):
 #  Unified diff integration tests
 #
 
+def test_main_run_unified_default_local_files_no_diff(capsys):
+    """Test default behavior when there is no difference in font files under evaluation"""
+    args = [ROBOTO_BEFORE_PATH, ROBOTO_BEFORE_PATH]
+
+    run(args)
+    captured = capsys.readouterr()
+    assert captured.out.startswith("[*] There is no difference between the files.")
+
+
 def test_main_run_unified_default_local_files(capsys):
     args = [ROBOTO_BEFORE_PATH, ROBOTO_AFTER_PATH]
 
@@ -113,6 +122,27 @@ def test_main_run_unified_default_local_files(capsys):
             assert line == expected_string_list[x]
 
 
+def test_main_run_unified_local_files_without_mp_optimizations(capsys):
+    args = ["--nomp", ROBOTO_BEFORE_PATH, ROBOTO_AFTER_PATH]
+
+    run(args)
+    captured = capsys.readouterr()
+
+    res_string_list = captured.out.split("\n")
+    expected_string_list = ROBOTO_UDIFF_EXPECTED.split("\n")
+
+    # have to handle the tests for the top two file path lines
+    # differently than the rest of the comparisons because
+    # the time is defined using local platform settings
+    # which makes tests fail on different remote CI testing services
+    for x, line in enumerate(res_string_list):
+        # treat top two lines of the diff as comparison of first 10 chars only
+        if x in (0, 1):
+            assert line[0:9] == expected_string_list[x][0:9]
+        else:
+            assert line == expected_string_list[x]
+
+
 def test_main_run_unified_default_remote_files(capsys):
     args = [ROBOTO_BEFORE_URL, ROBOTO_AFTER_URL]