https://bugzilla.redhat.com/show_bug.cgi?id=2210561
Maxwell G maxwell@gtmx.me changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value Blocks| |177841 (FE-NEEDSPONSOR) CC| |maxwell@gtmx.me
--- Comment #1 from Maxwell G maxwell@gtmx.me --- Congratulations on your first package submission! Here are some comments.
General comments -----------------------
Upstream's pyproject.toml has
``` [build-system] requires = ["poetry>=1.3"] build-backend = "poetry.masonry.api" ```
I'd use poetry-core as the build backend:
``` [build-system] requires = ["poetry-core>=1.3"] build-backend = "poetry.core.masonry.api" ```
----
The maximum version pins in
``` [tool.poetry.dependencies] python = ">=3.9,<3.12" prettytable = ">=0.7.2,<3.8" ```
are also a problem for Fedora integration. You should relax them unless you're absolutely sure a certain version won't work. We only have one version of everything, so packages cannot be so picky or the package will become uninstallable when a dependency is updated or block the dependency update all together. Pinning Python versions are especially problematic; the Fedora Python maintainers are already preparing for the Python 3.12 rebuild.
Specfile comments ---------------------------
Group: Applications/Engineering
The Group tag should not be used in Fedora.
----
BuildRequires: python3dist(poetry) BuildRequires: python3dist(tox-current-env)
Please remove these. %generate_buildrequires automatically handles these BuildRequires.
----
You should evenly space specfile sections to make the file more readable. There's two newlines between %prep and %generate_buildrequires, so there should also be two newlines between %build and %install and %install and %check and so on. Two newlines between specfile sections is a general convention.
----
You run `%pyproject_buildrequires -t` and `%tox`, but you don't actually have any tox envs configured in tox.ini. You should either properly configure tox to run your unit tests or entirely remove those invocations.
----
``` # Fix command name in manpage ln -s hstsparser.py hstsparser
# This runs outside the venv, so --version will return the default help2man -N ./hstsparser -o %{buildroot}%{_mandir}/man1/hstsparser.1 --version-string='%{version}' rm -f hstsparser ```
Building of manpages should happen in %build not %install. You can output the file to ./hstsparser.1 in the current directory during %build and then copy that file to %{buildroot}%{_mandir}/man1 during %install.
Also, I'm not sure what
``` # This runs outside the venv, so --version will return the default ```
means. Fedora RPM builds do not use venvs.
Maxwell's Python Review Checklist -----------------------
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
- [x] The License tag reflects the package contents and uses the correct identifiers. - [x] The license text is included and marked with %license. - [x] The package builds successfully in mock. - [x] The package is installable (checked by fedora-review). - [x] There are no relevant rpmlint errors.
- [x] The package runs tests in %check.
The package runs %pyproject_check_import and %tox. Running only %pyproject_check_import is enough to satisfy this guideline, but %tox shouldn't be run if it's NOOP. See the note above.
- [x] The latest version is packaged or packaging an earlier version is justified. - [-] Libraries: The package name has a `python3-` prefix and uses the canonical project name - [x] Applications: The package follows the general Naming Guidelines for applications - [x] The pyproject macros are used. - [x] There are no bundled libraries.
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor