[ttx_diff] Build otl-normalizer binary explicitly #1057
Conversation
| @@ -616,6 +610,19 @@ def delete_things_we_must_rebuild(rebuild: str, fontmake_ttf: Path, fontc_ttf: P | |||
| os.remove(path) | |||
|
|
|||
|
|
|||
| # returns the path to the compiled binary | |||
| def build_crate(manifest_path: Path): | |||
There was a problem hiding this comment.
We already have logic to find the root fontc directory. We already assume the paths of crates. It would be more consistent with the existing code to just presume you'll find the binary at root / "target" / "release" / the_name.
resources/scripts/ttx_diff.py
Outdated
| @@ -627,6 +634,8 @@ def main(argv): | |||
| sys.exit("Expected to be at the root of fontc") | |||
| fontc_manifest_path = root / "fontc" / "Cargo.toml" | |||
| otl_norm_manifest_path = root / "otl-normalizer" / "Cargo.toml" | |||
| otl_bin_path = build_crate(otl_norm_manifest_path) | |||
| assert otl_bin_path is not None, "failed to build otl-normalizer?" | |||
There was a problem hiding this comment.
Suggest asserting it's a file like fontmake_ttf below
resources/scripts/ttx_diff.py
Outdated
| # returns the path to the compiled binary | ||
| def build_crate(manifest_path: Path): | ||
| cmd = ['cargo', 'build', '--release', '--manifest-path', str(manifest_path)] | ||
| run(cmd, None, check=True) |
There was a problem hiding this comment.
that is that None doing as second positional parameter? Can we get rid of it or use a named keyword argument instead?
Is it stdin? It's not clear from subprocess.run signature
https://docs.python.org/3.13/library/subprocess.html#subprocess.run
The full function signature is largely the same as that of the Popen constructor - most of the arguments to this function are passed through to that interface. (timeout, input, check, and capture_output are not.)
There was a problem hiding this comment.
this isn't calling subprocess.run, it's calling a run fn defined in this file (but yes, can use named args, i'll also rename our run fn in a followup PR)
There was a problem hiding this comment.
ahah bad choice of name then 😅
There was a problem hiding this comment.
if it's passing working_dir=None and the entire reason of existence of ttx_diff.run() is to call subprocess.run with the given cwd=working_dir, then it's better to avoid the wrapper altogether and simply use subproces.run directly?
There was a problem hiding this comment.
I think the main purpose is that it also logs the command before running.
| @@ -142,17 +142,12 @@ def run_normalizer_gpos(cargo_manifest_path: Path, font_file: Path): | |||
|
|
|||
| # we had a bug where this would sometimes hang in mysterious ways, so we may | |||
| # call it multiple times if it fails | |||
| def try_normalizer_gpos(cargo_manifest_path: Path, font_file: Path, out_path: Path): | |||
| def try_normalizer_gpos(normalizer_bin: Path, font_file: Path, out_path: Path): | |||
| if not out_path.is_file(): | |||
| cmd = [ | |||
| "timeout", | |||
There was a problem hiding this comment.
since you're already using subprocess.run, I suggest you get rid of this external GNU command and use use the timeout parameter, which should be equivalent and more portable
https://docs.python.org/3.13/library/subprocess.html#subprocess.run
There was a problem hiding this comment.
sure, I'll do that in a separate PR.
cmyr
force-pushed
the
ttx-use-normalizer-bin
branch
from
e2ba01d
to
ff2c4d1
Compare
Invoking this through cargo has some overhead, and involves possible conjestion (I believe only one cargo process at a time will run in a given package or workspace) and I would also like to rule out one more possible complication that could contribute to getting SIGKILL'd.
cmyr
force-pushed
the
ttx-use-normalizer-bin
branch
from
ff2c4d1
to
3c5d520
Compare
Invoking this through cargo has some overhead, and involves possible conjestion (I believe only one cargo process at a time will run in a given package or workspace) and I would also like to rule out one more possible complication that could contribute to getting SIGKILL'd.
longer term, in order to manage the growing complexity of ttx_diff and to support the very different cases of 'CI on many fonts running on a transient host' and 'me at my desk comparing one source' I'd like to tease apart the building vs diffing bits of ttx_diff, and one part of that will mean the diffing bits shouldn't need to know where the
fontcrepo is located; we'll just pass in the path to the binary itself.