https://bugzilla.redhat.com/show_bug.cgi?id=2115102 --- Comment #2 from Wayne Sun <gsun@xxxxxxxxxx> --- (In reply to Adam Williamson from comment #1) > 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`. Yeah, I want to exclude one file for the rpm release, without the `%exclude` it will fail build with: Checking for unpackaged file(s): /usr/lib/rpm/check-files /home/waynesun/rpmbuild/BUILDROOT/pylero-0.0.3-1.fc36.x86_64 error: Installed (but unpackaged) file(s) found: /usr/bin/pylero RPM build errors: Installed (but unpackaged) file(s) found: /usr/bin/pylero > * 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}`. I'll update this four parts. > * 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. The project is using the MIT license and listed in the spec which satisfied with the SPDX license expression. > * `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'll update this three parts. > * 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 The test in the upstream project have issue with running as it's not unit test so I have to exclude them, I'll try remove the buildrequires for suds and click. > > > > 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. I started with the new modern-style spec first but have to downgrade to 201x python rpm build spec as the modern-style does not work on epel8 which is a release target. > > 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. I'll check more on this but the epel8 still a blocker to upgrade the spec. Thanks for the review -- 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
