search

googlefonts / ots-python

Python wheels for the OpenType Sanitizer
BSD 3-Clause "New" or "Revised" License
9 stars 7 forks source link

add an option to use system-provided `ots-sanitize` #18

Open Apteryks opened 2 years ago

Apteryks commented 2 years ago

For distributions already carrying opentype-sanitizer, it makes little sense to build and use this bundled copy.

musicinmybrain commented 2 years ago

I’m not sure what the best way to allow this upstream would be (or I would have offered a PR). I’m pretty confident that what I do downstream in Fedora Linux to avoid bundling the executable is not the right general solution. Still, I’m dropping a link to these downstream hacks in case it is useful.

anthrotype commented 2 years ago

The whole point of this package is to bundle the ots-sanitize binary and make it available to install with pip. If one already has the ots-sanitize binary installed and available on PATH, one may as well just call it with subprocess.run(["ots-sanitize", ...]) -- which is exactly what this python wrapper is doing by the way. We could change the code to first check if the ots-sanitize binary is bundled then use that, otherwise try to call whatever the ots-sanitize command that is found on PATH, if that would help your use case. But then, again, this module won't be doing anything more than calling that as a subprocess.

Apteryks commented 2 years ago

It wasn't obvious to me that python-opentype-sanitizer was only some means to distribute a C program bundled as a Python package. I thought it provided some extras on top, so I packaged it when I saw that the amiri font (https://www.amirifont.org/) depended on it (python-opentype-sanitizer).

rsheeter commented 2 years ago

We could change the code to first check if the ots-sanitize binary is bundled

Wouldn't that make the version of OTS you get unpredictable even when using the same version of this package? - I'd hate to install this one one system, run it, install the same version on another system, run it, and have the results not match.

anthrotype commented 2 years ago

it's just a convenient way to install a pre-compiled executable using python's own packaging ecosystem

musicinmybrain commented 2 years ago

But then, again, this module won't be doing anything more than calling that as a subprocess.

That’s pretty much what I want to accomplish (and am doing downstream-only at the moment) as a distribution packager: support Python programs that use the PyPI package (for the reasons stated), rather than having to patch them all individually to call the CLI tool directly. It’s easier to patch the Python wrapper in one place to use the system-wide executable.

musicinmybrain commented 2 years ago

Wouldn't that make the version of OTS you get unpredictable even when using the same version of this package? - I'd hate to install this one one system, run it, install the same version on another system, run it, and have the results not match.

Yes, this option really only makes sense for use in Linux distributions and similar, where the Python library and the external executable can be updated together. For example, the RPM spec file in Fedora Linux has an exact-version dependency:

Requires:       opentype-sanitizer = %{version}

which ensures the two packages remain synchronized.

anthrotype commented 2 years ago

then you can simply patch this line to be wherever is your downstream packaged executable, no?

https://github.com/googlefonts/ots-python/blob/main/src/python/ots/__init__.py#L6

musicinmybrain commented 2 years ago

then you can simply patch this line to be wherever is your downstream packaged executable, no?

https://github.com/googlefonts/ots-python/blob/main/src/python/ots/__init__.py#L6

Combined with with patching out the custom cmdclass and ext_modules in setup.py and the meson and ninja in pyproject.toml, that would probably be a reasonable approach. It might be slightly simpler than the symlink-based approach I’m using now, if there isn’t some awkward detail I’m missing.

Apteryks commented 2 years ago

then you can simply patch this line to be wherever is your downstream packaged executable, no?

https://github.com/googlefonts/ots-python/blob/main/src/python/ots/__init__.py#L6

That's basically what I did in GNU Guix: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=9e29cf8d73c39fa65a9d4a107b5db614cb88062a; still, it's not great that packagers have to find that themselves then patch out some boilerplate in setup.py. Perhaps it could use a OTS_SANITIZE environment variable to bake a default path at build time, and further honor OTS_SANITIZE at run time for completion sake.