https://bugzilla.redhat.com/show_bug.cgi?id=2114603 --- Comment #7 from Jonathan Wright <jonathan@xxxxxxxxxxxxx> --- (In reply to Miro Hrončok from comment #2) > Spec sanity check: > > > > %{?!python3_pkgversion:%global python3_pkgversion 3} > Per our IRC chat...rpmdev-newspec -t python > > > > %global srcname kgb > > Setting and using this IMHO makes the spec file harder to read (I cannot for > example copy-paste the URL to my web browser) with very little benefit. It's > not like I can copy-paste the entire spec, change this macro, and ship that > as another package, or is it? I've seen it in a few other places as standard practice. It's not really needed or beneficial here. > > > # requested upstream add license https://github.com/beanbaginc/kgb/issues/10 > > Source1: LICENSE > > Where is this file coming from in the meantime? PyPI lists MIT on the package and it is controlled by upstream and their other projects are all MIT. I pulled a copy of their MIT license from https://github.com/beanbaginc/diffx/blob/master/LICENSE which is a project that pulls this as a dependency. I suspect they'll have the license added to the repo before this hits our prod repos. The dev has been very responsive. > > > BuildRequires: python%{python3_pkgversion}-devel > > BuildRequires: python%{python3_pkgversion}-setuptools > > # Test dependencies: > > BuildRequires: python3dist(pytest) > > BuildRequires: python3dist(six) > > > You choose to make the spec file more complex by using the > %{python3_pkgversion} macro instead of literally using 3. But then you use > python3dist(pytest) directly, discarding any potential benefit of using > %{python3_pkgversion} in the first place. What is your motivation for using > %{python3_pkgversion}? What EPEL versions and Python versions in that EPEL > do you target here? Is it needed, or is it just copy-pasted from some other > spec file? More rpmdev-newspec -t python defaults. > > %{?python_provide:%python_provide python3-%{srcname}} > > This is deprecated and SHOULD NOT be used. See the note at the end of > https://docs.fedoraproject.org/en-US/packaging-guidelines/Python_201x/ > #_the_py_provides_macro More rpmdev-newspec -t python defaults > > %if 0%{?fedora} || 0%{?rhel} > 8 > > ... > > %if 0%{?el8} > > The spec seems to mix two kinds of conditionals that mean the same in this > context. I highly recommend choosing one and sticking to it. The old/new > Python packaging style %id/%else is already hard to follow as it is. I missed this when simplifying conditionals to just check 0?%{elX}. It was supposed to match the other two. > > %if 0%{?fedora} || 0%{?rhel} > 8 > > %generate_buildrequires > > %pyproject_buildrequires > > %endif > > What is the benefit of using this, considering the package already needs to > list all the deps for EPEL 8? This does properly install some deps, pip for example. I'll see about doing a PR upstream to get all the deps listed properly so the static BuildRequires can be removed. pip is grabbed on EPEL8 as a dep for something else (one of the other buildrequires in epel8 must be requiring it), but it's not on Fedora, so this take care of it on Fedora. > > # move local module folder out of the way so pytest doesn't use the wrong one > > %pytest --pyargs %{srcname} > > The comment does not seem to correspond to the actual usage. Is it outdated? Fixed. Was a leftover from trying to figure out how to work around a testing issue (thanks for your help with that BTW). (In reply to Miro Hrončok from comment #3) > > %{python3_sitelib}/%{srcname}-%{version}.dev* > > This is also a bit weird. Why dev? I see the package is recognized as > 7.0.dev0 by Python tools. Is that a packaging bug? Should this be plain 7.0? I see your comment about upstream issue in git. I'll PR it probably. (In reply to Miro Hrončok from comment #4) > (In reply to Miro Hrončok from comment #3) > > > %{python3_sitelib}/%{srcname}-%{version}.dev* > > > > This is also a bit weird. Why dev? I see the package is recognized as > > 7.0.dev0 by Python tools. Is that a packaging bug? Should this be plain 7.0? > > When I unpack the tarball and run: > > $ python -c 'import setuptools.build_meta; > setuptools.build_meta.prepare_metadata_for_build_wheel(".")' > > I get: > > $ grep ^Version: kgb.dist-info/METADATA > Version: 7.0.dev0.dev > > > The build log is also full of: > > /usr/lib/python3.10/site-packages/pkg_resources/__init__.py:116: > PkgResourcesDeprecationWarning: 7.0.dev0.dev is an invalid version and will > not be supported in a future release Guess this is an upstream issue that needs to be fixed? They've got it published as a full on release. I'll submit a PR to github and use PyPI for now. Fixed all spec mentioned issues and versioning issue. The spec file indeed looks a ton cleaner now. Thank you for the feedback. Spec URL: https://jonathanspw.fedorapeople.org/python-kgb.spec SRPM URL: https://jonathanspw.fedorapeople.org/python-kgb-7.0-2.fc36.src.rpm -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2114603 _______________________________________________ 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
