Bug 2040624 - Review Request: python-psautohint - Python wrapper for Adobe's PostScript autohinter
Summary: Review Request: python-psautohint - Python wrapper for Adobe's PostScript aut...
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2022-01-14 10:07 UTC by David King
Modified: 2022-08-28 00:45 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2022-08-28 00:45:18 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description David King 2022-01-14 10:07:48 UTC
Spec URL: https://amigadave.fedorapeople.org/python-psautohint-2.3.1-1.fc36.src.rpm
SRPM URL: https://amigadave.fedorapeople.org/python-psautohint-2.3.1-1.fc36.src.rpm
Description: A standalone version of AFDKO’s autohinter.
Fedora Account System Username: amigadave

Comment 1 David King 2022-01-14 10:08:21 UTC
Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=81230885

Comment 2 Ben Beasley 2022-01-15 13:47:13 UTC
Spec URL: https://amigadave.fedorapeople.org/python-psautohint.spec

Comment 3 Ben Beasley 2022-01-15 15:13:48 UTC
This is not quite true:

> # Test suite needs dependencies not in Fedora.

----

In fact, we can add:

> %global data_commit 1e4c5061d328105c4dcfcb6fdbc27ec49b3e9d23
> # This contains bundled fonts for testing, but they are *not* installed
> Source1:        https://github.com/adobe-type-tools/psautohint-testdata/archive/%{data_commit}/psautohint-testdata-%{data_commit}.tar.gz

and

> BuildRequires:  /usr/bin/tx

(which reduces the number of skipped tests)

and then to make sure all the functionality is available by default, in python3-psautohint:

> Recommends:     /usr/bin/tx

Then, in %prep:

> %autosetup -p1 -n %{tarball_name}-%{version}
> rm -vrf tests/integration/data
> %setup -q -n %{tarball_name}-%{version} -T -D -b 1
> mv ../psautohint-testdata-%{data_commit}/ tests/integration/data
> # Allow newer versions of testing dependencies
> sed -r -i 's/("pytest[^[:blank:]]*) >=[^"]+/\1/' setup.py
> # Drop pytest-randomly, which doesn’t seem to be mandatory,
> # and pytest-cov, since we do not need coverage analysis
> sed -r -i '/pytest-(randomly|cov)/d' setup.py

The only missing dependency was python3dist(pytest-randomly), which as mentioned above doesn’t actually seem to be required. Then:

> %pyproject_buildrequires -x testing

and

> k="${k-}${k+ and }not test_hashmap_old_version'
> %pytest -k "${k-}"

At least in a local mock build on x86_64, that results in all the tests running and passing, except for the one I skipped. I haven’t looked into why it fails.

Comment 4 Ben Beasley 2022-01-15 15:16:26 UTC
> k="${k-}${k+ and }not test_hashmap_old_version'
> %pytest -k "${k-}"

This could just be

> %pytest -k 'not test_hashmap_old_version'

I’m used to using the shell variable pattern so it’s easy to add and remove skips in packages where this is frequently required.

Comment 5 Kalev Lember 2022-07-28 11:17:16 UTC
I would simplify this a bit and call the package just 'psautohint' and not python-psautohint because it's just an executable and not a python library. See https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_application_naming

Comment 6 Ben Beasley 2022-07-28 13:16:37 UTC
(In reply to Kalev Lember from comment #5)
> I would simplify this a bit and call the package just 'psautohint' and not
> python-psautohint because it's just an executable and not a python library.
> See
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/
> #_application_naming

This is a fair point. There actually is a usable Python API, e.g. [1], but there is a good argument that the package exists primarily to provide the executable. I think I would find it acceptable either way, but lean toward application naming.

[1] https://github.com/adobe-type-tools/psautohint/blob/0a1e514a3cb86c6593e4f05814d70afc390f7e69/python/psautohint/__init__.py

Comment 7 Ben Beasley 2022-07-28 13:17:38 UTC
David, are you still working on this package?

Comment 8 Kalev Lember 2022-07-28 13:52:28 UTC
(In reply to Ben Beasley from comment #6)
> (In reply to Kalev Lember from comment #5)
> > I would simplify this a bit and call the package just 'psautohint' and not
> > python-psautohint because it's just an executable and not a python library.
> > See
> > https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/
> > #_application_naming
> 
> This is a fair point. There actually is a usable Python API, e.g. [1], but
> there is a good argument that the package exists primarily to provide the
> executable. I think I would find it acceptable either way, but lean toward
> application naming.
> 
> [1]
> https://github.com/adobe-type-tools/psautohint/blob/
> 0a1e514a3cb86c6593e4f05814d70afc390f7e69/python/psautohint/__init__.py

Oh, that's true, and it's actually installed as well, I think -- I totally missed it at first because the python library must be hidden behind %files -n python3-psautohint -f %{pyproject_files} and not explicitly listed in %files.

Anyway, maybe in that case it would make sense to have a split where:

Source package name: psautohint

Two binary packages:
psautohint (/usr/bin/*)
python3-psautohint (python library)

Comment 9 Ben Beasley 2022-07-28 14:54:37 UTC
(In reply to Kalev Lember from comment #8)
> Oh, that's true, and it's actually installed as well, I think -- I totally
> missed it at first because the python library must be hidden behind %files
> -n python3-psautohint -f %{pyproject_files} and not explicitly listed in
> %files.
> 
> Anyway, maybe in that case it would make sense to have a split where:
> 
> Source package name: psautohint
> 
> Two binary packages:
> psautohint (/usr/bin/*)
> python3-psautohint (python library)

This is good advice in general (especially coming from a C/C++ perspective) but I am not sure if it is worth separating them in this case. The implementations of the binaries live within the library and the actual scripts are trivial generated entry points of only a few lines each. There is little to be saved by not installing them when only the library is required. I suppose one could %exclude the __main__.py file from the library and package it with the binaries. That’s probably the right way to separate the two, but I don’t know if it would be worth it; that file is only ~30kB.

It’s pretty common for primarily-application Python packages to also provide a Python library. The Provides like “python3dist(psautohint)” are generated automatically (assuming proper dist-info metadata), and the rest can be handled by adding an explicit “%py_provides python3-psautohint” if there is not an explicit library subpackage.

It’s also pretty common for primarily-library Python packages to provide executable entry points without a separate subpackage—usually without a virtual Provides for the equivalent application name, although I don’t believe it would be incorrect to add one in cases where it’s clear what that name would be.

Obviously, the dividing line between these two types of packages is fuzzy and subjective. There are plenty of cases where there doesn’t seem to be one clearly-correct answer, and there are plenty of examples of important existing packages that probably should have chosen the other naming scheme.

Comment 10 Kalev Lember 2022-07-28 15:20:26 UTC
Fair enough. I guess in this case it's mainly an executable package then? Upstream's naming (psautohint instead of python-psautohint) seems to hint that this may be the case.

I'm pretty sure David is packaging it for cantarell fonts and cantarell fonts build system uses the executable there and not the library.

Comment 11 Package Review 2022-08-28 00:45:18 UTC
This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.


Note You need to log in before you can comment on or make changes to this bug.