[Bug 2115102] Review Request: python-pylero - Python wrapper for the Polarion WSDL API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux