https://bugzilla.redhat.com/show_bug.cgi?id=2115102 --- Comment #1 from Adam Williamson <awilliam@xxxxxxxxxx> --- I'm not a sponsor so cannot do the official review, but a couple of unofficial notes: * I'd be concerned about `%define _unpackaged_files_terminate_build 0`. I can't think of many cases where that's a good idea for a Fedora spec. Why is it here? What packages are showing up as 'unpackaged'? Is it because of the `%exclude %{_bindir}/%{name}` - you don't want that file packaged, for some reason? I would usually handle that by just deleting it at the end of `%install`, which avoids the need for `_unpackaged_files_terminate_build` and a `%exclude`. * Why is unmangled_version defined but never used? Does a macro use it? I don't know that any of the standard Python macros do. If it's not needed it should be removed. If it's needed for some reason, why not just define it as %{version} since they're the same? Saves updating it twice unless they diverge. * SOURCE should not be upper-case. Fedora packages usually go with Source0, but that's just a style note. * Defining `name`, `version` and `release` then just using them in `Name:`, `Version:` and `Release:` is a bit odd. If you just fill those fields in normally - `Name: pylero`, `Version: `0.0.3`, `Release: 1` - then %name, %version and %release get defined for you. These kinds of 'define blocks' ahead of the start of the spec are usually used to set things that aren't part of the core RPM spec, like %github_version and stuff. * `1` is wrong for the release value, anyway. It needs to be `1%{?dist}`. * For a new package, SPDX-style License: is recommended now. See https://lists.fedoraproject.org/archives/list/devel@xxxxxxxxxxxxxxxxxxxxxxx/message/6GKXXE7AMBB76IBRMCTU3L57GLCCZL55/ for more on this. * `Group:` and `Vendor:` are not used by Fedora and should not be in the spec. * I don't think there's any need to set `Prefix:` either, though the guidelines don't explicitly address that. * The Python guidelines say "Packages MUST use the automatic Python run-time dependency generator...Dependencies covered by the generators SHOULD NOT be repeated in the .spec file". Check if the dependency generator picks up suds and click; if so, those requirements aren't needed. The requirement for `python3-%{name} == %{version}-%{release}` is probably fine if they really need a strict version tie like that. * I see upstream has tests, but you aren't running them in this package build, you should run upstream tests during package build if at all possible. I'd think the buildrequires on suds and click wouldn't actually be necessary if you're not running the tests, too - but don't take out the buildrequires, add the tests. :D This is a much bigger point and not required to pass review, but I'd recommend looking at the more modern Python macros and example spec in the guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_empty_spec_file . The automatic build dependency and pyproject-based build/install/check stuff is cool for modern-style Python projects that use the improved approaches to packaging (in the upstream sense) from the last decade or so. On a quick look at upstream it doesn't seem to have been updated for a lot of the more modern Python packaging stuff, it's still pretty old-school (setup.py, no pyproject.toml). If you're interested in updating the upstream project, then for a guide to some of that stuff, see https://hynek.me/articles/sharing-your-labor-of-love-pypi-quick-and-dirty/ , though it's aging a bit, and https://realpython.com/pypi-publish-python-package/ , which I just found but looks good. It also has a good list of links to the relevant PEPs. I would add to those that using setuptools_scm to determine package content from which files are tracked by git - instead of having to do it separately - is convenient, and makes it easy to include tests in tarballs, which you really should do so they can be run in downstream package builds like this one ;). Only issue with that is it'll get dragged into package builds unnecessarily if you use the automatic build requirements generator, see https://src.fedoraproject.org/rpms/fedfind/blob/rawhide/f/fedfind.spec#_36 for how I work around that. -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2115102 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
