Spec URL: https://sgallagh.fedorapeople.org/packagereview/python-mmdzanata/python-mmdzanata.spec SRPM URL: https://sgallagh.fedorapeople.org/packagereview/python-mmdzanata/python-mmdzanata-0.4-1.fc29.src.rpm Description: Provides a library and tools for dealing with translatable strings in modulemd documents. Fedora Account System Username:sgallagh
Spec URL: https://sgallagh.fedorapeople.org/packagereview/python-mmdzanata/python-mmdzanata.spec SRPM URL: https://sgallagh.fedorapeople.org/packagereview/python-mmdzanata/python-mmdzanata-0.5-1.fc29.src.rpm COPR Build: https://copr.fedorainfracloud.org/coprs/sgallagh/mmdzanata/ This version properly includes the COPYING file referenced in the source header comments.
* I don't see the need for that runtime dependency on setuptools. What's the point? * Missing runtime dependency on python3-gobject-base or equivalent. * All files state "Copyright (C) 2017-2018 Stephen Gallagher", yet the first upstream commit is from a week ago. * Does cli.py need the shebang line? * As an executable, mmdzanata could have a manpage.
(In reply to Petr Šabata from comment #2) > * I don't see the need for that runtime dependency on setuptools. > What's the point? That was added automatically by pyp2rpm; it's probably not needed. > > * Missing runtime dependency on python3-gobject-base or equivalent. Ah, right. Good catch. > > * All files state "Copyright (C) 2017-2018 Stephen Gallagher", yet the > first upstream commit is from a week ago. Oops, copy-paste error. > > * Does cli.py need the shebang line? It does not need one, it's not meant to be directly executable. It's wrapped by console_scripts in setup.py. > > * As an executable, mmdzanata could have a manpage. It probably should, but I haven't written one yet and this isn't a blocker for initial review IMHO.
Spec URL: https://sgallagh.fedorapeople.org/packagereview/python-mmdzanata/python-mmdzanata.spec SRPM Url: https://sgallagh.fedorapeople.org/packagereview/python-mmdzanata/python-mmdzanata-0.6-1.fc29.src.rpm Copr Build: https://copr.fedorainfracloud.org/coprs/sgallagh/mmdzanata/package/python-mmdzanata/ This review now depends on BZ #1623469 to provide the tool needed to generate the requested manpages. I've dropped the setuptools runtime dep, added python3-gobject-base, fixed the copyright headers, dropped the shebang line and added the manpage.
Spec URL: https://sgallagh.fedorapeople.org/packagereview/python-mmdzanata/python-mmdzanata.spec SRPM URL: https://sgallagh.fedorapeople.org/packagereview/python-mmdzanata/python-mmdzanata-0.6-2.fc29.src.rpmCopr Build: https://copr.fedorainfracloud.org/coprs/sgallagh/mmdzanata/package/python-mmdzanata/ Previous build failed missing some BuildRequires. This fixes that.
* I still see the runtime dep on setuptools. * Build deps on click-man and pygobject are listed twice. * I suppose this method of manpage generation loads the files, so that's why you need all those build deps. Is that right? In that case you should also add python(requests) as a build dep. * I confirm the shebang and copyright changes.
Spec URL: https://sgallagh.fedorapeople.org/packagereview/python-mmdzanata/python-mmdzanata.spec SRPM URL: https://sgallagh.fedorapeople.org/packagereview/python-mmdzanata/python-mmdzanata-0.7-1.fc29.src.rpm COPR Build: https://copr.fedorainfracloud.org/coprs/sgallagh/mmdzanata/package/python-mmdzanata/ I fixed the runtime dep on setuptools. Yes, the manpage generation needed the builddeps to function. I'm not sure why it didn't need requests, but I've added it for future-proofing. I replaced most of the runtime deps with the %{?python_enable_dependency_generator} macro, but left the pygobject one because that can't be present in setup.py (I don't think) and therefore auto-detected. I added a python2 subpackage to support pungi in more places.
Almost there! %{description} isn't an expandable macro. If you want to re-use the description text, define something else and use that. The current result is you have the literal "%{description}" as your package description. Also not sure whether you need line 74, doesn't the next step overwrite it? Though I haven't tried that, just thinking.
Spec URL: https://sgallagh.fedorapeople.org/packagereview/python-mmdzanata/python-mmdzanata.spec SRPM URL: https://sgallagh.fedorapeople.org/packagereview/python-mmdzanata/python-mmdzanata-0.7-2.fc29.src.rpm COPR Build: https://copr.fedorainfracloud.org/coprs/sgallagh/mmdzanata/package/python-mmdzanata/ I fixed the description. Regarding the binary directory purge, that's future-proof in case for some reason the python2 build ever produces different content from the python3 one. It was put there by pyp2rpm and I don't see a good reason to remove it.
Ack. Approving.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/python-mmdzanata
This package was approved and imported in repositories and it was later retired, but this review ticket was never closed. I'm closing it now.