From af6804bed54d26d210e00bc416376f9315bae7d1 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 21 Apr 2022 18:11:20 +0100 Subject: [PATCH] make USE_HARFBUZZ_REPACKER a 3-state option, defaults to auto if explicitly enabled, it will raise ImportError if uharfbuzz is not found, and will propagate the uharfbuzz error instead of silently falling back to the pure-python serializer --- Lib/fontTools/config/__init__.py | 8 +++--- Lib/fontTools/misc/configTools.py | 15 +++++++++++ Lib/fontTools/subset/__init__.py | 8 +++--- Lib/fontTools/ttLib/tables/otBase.py | 40 +++++++++++++++++++--------- Tests/misc/configTools_test.py | 21 +++++++++++++++ Tests/subset/subset_test.py | 34 ++++++++++++++++++----- 6 files changed, 100 insertions(+), 26 deletions(-) diff --git a/Lib/fontTools/config/__init__.py b/Lib/fontTools/config/__init__.py index 3877742e7..f5a62eaf1 100644 --- a/Lib/fontTools/config/__init__.py +++ b/Lib/fontTools/config/__init__.py @@ -49,9 +49,11 @@ Config.register_option( FontTools tries to use the HarfBuzz Repacker to serialize GPOS/GSUB tables if the uharfbuzz python bindings are importable, otherwise falls back to its slower, less efficient serializer. Set to False to always use the latter. + Set to True to explicitly request the HarfBuzz Repacker (will raise an + error if uharfbuzz cannot be imported). """ ), - default=True, - parse=lambda s: str(s).lower() not in {"0", "no", "false"}, - validate=lambda v: isinstance(v, bool), + default=None, + parse=Option.parse_optional_bool, + validate=Option.validate_optional_bool, ) diff --git a/Lib/fontTools/misc/configTools.py b/Lib/fontTools/misc/configTools.py index dbbd56ce1..38bbada24 100644 --- a/Lib/fontTools/misc/configTools.py +++ b/Lib/fontTools/misc/configTools.py @@ -100,6 +100,21 @@ class Option: validate: Optional[Callable[[Any], bool]] = None """Return true if the given value is an acceptable value.""" + @staticmethod + def parse_optional_bool(v: str) -> Optional[bool]: + s = str(v).lower() + if s in {"0", "no", "false"}: + return False + if s in {"1", "yes", "true"}: + return True + if s in {"auto", "none"}: + return None + raise ValueError("invalid optional bool: {v!r}") + + @staticmethod + def validate_optional_bool(v: Any) -> bool: + return v is None or isinstance(v, bool) + class Options(Mapping): """Registry of available options for a given config system. diff --git a/Lib/fontTools/subset/__init__.py b/Lib/fontTools/subset/__init__.py index 0e2ab898c..ed47c7ceb 100644 --- a/Lib/fontTools/subset/__init__.py +++ b/Lib/fontTools/subset/__init__.py @@ -146,9 +146,11 @@ Output options The Zopfli Python bindings are available at: https://pypi.python.org/pypi/zopfli ---harfbuzz-repacker [default] - Serialize GPOS/GSUB using the HarfBuzz Repacker if uharfbuzz can be - imported [default]. +--harfbuzz-repacker + By default, we serialize GPOS/GSUB using the HarfBuzz Repacker when + uharfbuzz can be imported and is successful, otherwise fall back to + the pure-python serializer. Set the option to force using the HarfBuzz + Repacker (raises an error if uharfbuzz can't be found or fails). --no-harfbuzz-repacker Always use the pure-python serializer even if uharfbuzz is available. diff --git a/Lib/fontTools/ttLib/tables/otBase.py b/Lib/fontTools/ttLib/tables/otBase.py index 64e70ffe7..a99d55997 100644 --- a/Lib/fontTools/ttLib/tables/otBase.py +++ b/Lib/fontTools/ttLib/tables/otBase.py @@ -76,35 +76,49 @@ class BaseTTXConverter(DefaultTable): # If a lookup subtable overflows an offset, we have to start all over. overflowRecord = None + # this is 3-state option: default (None) means automatically use hb.repack or + # silently fall back if it fails; True, use it and raise error if not possible + # or it errors out; False, don't use it, even if you can. use_hb_repack = font.cfg[USE_HARFBUZZ_REPACKER] if self.tableTag in ("GSUB", "GPOS"): - if not use_hb_repack: + if use_hb_repack is False: log.debug( "hb.repack disabled, compiling '%s' with pure-python serializer", self.tableTag, ) elif not have_uharfbuzz: - log.debug( - "uharfbuzz not found, compiling '%s' with pure-python serializer", - self.tableTag, - ) + if use_hb_repack is True: + raise ImportError("No module named 'uharfbuzz'") + else: + assert use_hb_repack is None + log.debug( + "uharfbuzz not found, compiling '%s' with pure-python serializer", + self.tableTag, + ) while True: try: writer = OTTableWriter(tableTag=self.tableTag) self.table.compile(writer, font) - if use_hb_repack and have_uharfbuzz and self.tableTag in ("GSUB", "GPOS"): + if ( + use_hb_repack in (None, True) + and have_uharfbuzz + and self.tableTag in ("GSUB", "GPOS") + ): try: log.debug("serializing '%s' with hb.repack", self.tableTag) return writer.getAllDataUsingHarfbuzz() except (ValueError, MemoryError, hb.RepackerError) as e: - log.error( - "hb.repack failed to serialize '%s', reverting to " - "pure-python serializer; the error message was: %s", - self.tableTag, - e, - ) - return writer.getAllData(remove_duplicate=False) + if use_hb_repack is None: + log.error( + "hb.repack failed to serialize '%s', reverting to " + "pure-python serializer; the error message was: %s", + self.tableTag, + e, + ) + return writer.getAllData(remove_duplicate=False) + # let the error propagate if USE_HARFBUZZ_REPACKER is True + raise return writer.getAllData() except OTLOffsetOverflowError as e: diff --git a/Tests/misc/configTools_test.py b/Tests/misc/configTools_test.py index 844688618..94afb233d 100644 --- a/Tests/misc/configTools_test.py +++ b/Tests/misc/configTools_test.py @@ -57,3 +57,24 @@ def test_options_are_unique(): cfg.get(opt2) with pytest.raises(ConfigUnknownOptionError): cfg.set(opt2, "bar") + + +def test_optional_bool(): + for v in ("yes", "YES", "Yes", "1", "True", "true", "TRUE"): + assert Option.parse_optional_bool(v) is True + + for v in ("no", "NO", "No", "0", "False", "false", "FALSE"): + assert Option.parse_optional_bool(v) is False + + for v in ("auto", "AUTO", "Auto", "None", "none", "NONE"): + assert Option.parse_optional_bool(v) is None + + with pytest.raises(ValueError, match="invalid optional bool"): + Option.parse_optional_bool("foobar") + + assert Option.validate_optional_bool(True) + assert Option.validate_optional_bool(False) + assert Option.validate_optional_bool(None) + assert not Option.validate_optional_bool(1) + assert not Option.validate_optional_bool(0) + assert not Option.validate_optional_bool("1") diff --git a/Tests/subset/subset_test.py b/Tests/subset/subset_test.py index ae9be7e2d..a67a759fa 100644 --- a/Tests/subset/subset_test.py +++ b/Tests/subset/subset_test.py @@ -802,6 +802,8 @@ class SubsetTest: @pytest.mark.parametrize( "installed, enabled, ok", [ + pytest.param(True, None, True, id="installed-auto-ok"), + pytest.param(True, None, True, id="installed-auto-fail"), pytest.param(True, True, True, id="installed-enabled-ok"), pytest.param(True, True, False, id="installed-enabled-fail"), pytest.param(True, False, True, id="installed-disabled"), @@ -839,8 +841,24 @@ class SubsetTest: "--layout-features=*", f"--output-file={subsetpath}", ] - if not enabled: + if enabled is True: + args.append("--harfbuzz-repacker") + elif enabled is False: args.append("--no-harfbuzz-repacker") + # elif enabled is None: ... is the default + + if enabled is True: + if not installed: + # raise if enabled but not installed + with pytest.raises(ImportError, match="uharfbuzz"): + subset.main(args) + return + + elif not ok: + # raise if enabled but fails + with pytest.raises(hb.RepackerError, match="mocking"): + subset.main(args) + return with caplog.at_level(logging.DEBUG, "fontTools.ttLib.tables.otBase"): subset.main(args) @@ -851,14 +869,16 @@ class SubsetTest: subsetfont, self.getpath("expect_harfbuzz_repacker.ttx"), ["GSUB"] ) - if enabled: + if enabled or enabled is None: if installed: assert "serializing 'GSUB' with hb.repack" in caplog.text - else: - assert ( - "uharfbuzz not found, compiling 'GSUB' with pure-python serializer" - ) in caplog.text - else: + + if enabled is None and not installed: + assert ( + "uharfbuzz not found, compiling 'GSUB' with pure-python serializer" + ) in caplog.text + + if enabled is False: assert ( "hb.repack disabled, compiling 'GSUB' with pure-python serializer" ) in caplog.text