Bug 1623220 - Review Request: python-mmdzanata - Tools for working with translations of modulemd
Summary: Review Request: python-mmdzanata - Tools for working with translations of mod...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Petr Šabata
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1623469
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-08-28 19:05 UTC by Stephen Gallagher
Modified: 2020-06-13 15:45 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-06-13 15:45:38 UTC
Type: ---
psabata: fedora-review+


Attachments (Terms of Use)

Description Stephen Gallagher 2018-08-28 19:05:01 UTC
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

Comment 2 Petr Šabata 2018-08-29 10:34:31 UTC
* 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.

Comment 3 Stephen Gallagher 2018-08-29 11:59:04 UTC
(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.

Comment 4 Stephen Gallagher 2018-08-29 13:00:54 UTC
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.

Comment 6 Petr Šabata 2018-08-30 10:44:28 UTC
* 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.

Comment 7 Stephen Gallagher 2018-08-30 17:13:31 UTC
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.

Comment 8 Petr Šabata 2018-08-31 10:38:49 UTC
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.

Comment 9 Stephen Gallagher 2018-08-31 11:02:41 UTC
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.

Comment 10 Petr Šabata 2018-08-31 11:06:31 UTC
Ack.  Approving.

Comment 11 Gwyn Ciesla 2018-08-31 13:13:24 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/python-mmdzanata

Comment 12 Mattia Verga 2020-06-13 15:45:38 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.