Bug 1275517 - Review Request: python-wand - python bindings for ImageMagick
Review Request: python-wand - python bindings for ImageMagick
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-27 03:00 EDT by Dennis Chen
Modified: 2015-11-23 15:06 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-11-08 17:22:30 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rdieter: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Dennis Chen 2015-10-27 03:00:18 EDT
Spec URL: http://unintel.tk/packaging/python-wand.spec
SRPM URL: http://unintel.tk/packaging/python-wand-0.4.1-1.fc22.src.rpm
Description: Wand is a ctypes-based simple ImageMagick binding for Python. It doesn't cover all functionalities of MagickWand API currently. It works on Python 2.6, 2.7, 3.2, 3.3, and PyPy.
Fedora Account System Username: barracks510

This is my first package and I need a sponsor. A link to the koji build is here:

http://koji.fedoraproject.org/koji/taskinfo?taskID=11594764
Comment 1 Randy Barlow 2015-10-27 22:10:11 EDT
Hello Dennis!

My review here is informal (as I am also currently seeking to become a package maintainer), but here are a few notes:

0) You've put a BuildRequires on python3-setuptools in the section that deals with Python 2 (probably a typo).

1) Python 3 systems will get Requires ImageMagick twice - I think the second Requires line is redundant.

rpmlint only complains about a few spellings, but I think they are all OK so I wouldn't worry about them. Nice work!
Comment 2 Rex Dieter 2015-10-27 23:15:31 EDT
Welcome Dennis, I'd be happy to help review this (and sponsor you).

Per Randy's comment's:

0.  this is actually ok, it's already wrapped inside the conditional
%if %{with python3}
...
%endif

1.  MUST: however, putting runtime dep here is wrong, you really want the 
Requires: ImageMagick
down under
%package -n python3-wand

for cleanliness, and since this subpkg is already wrapped by %if %{with python3} itself, you could move both the runtime and build python3-related deps here, to look something like:

%if %{with python3}
%package     -n python3-wand
Summary:        Ctypes-based simple MagickWand API binding for Python
BuildRequires:  python3-devel
BuildRequires:  python3-setuptools
Requires:       ImageMagick
%description -n python3-wand
Wand is a ctypes-based simple ImageMagick binding for Python. It doesn't cover
all functionalities of MagickWand API currently. It works on Python 2.6, 2.7,
3.2, 3.3, and PyPy.
%endif


naming: ok

2. SHOULD consider omitting from %description the imo mostly-needless phrase:
"It works on Python 2.6, 2.7, 3.2, 3.3, and PyPy."

sources: kinda ok, but

3.  SHOULD see https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Git_Tags
about how to rename the source tarball, to use something like:
https://github.com/dahlia/wand/archive/%{version}.tar.gz#/%{name}-%{version}.tar.gz
instead

license: ok

macros: ok, mostly...

4.  SHOULD omit from %install:
rm -rf $RPM_BUILD_ROOT
it is deprecated and no longer needed.

scriptets: ok (n/a)


Please fix all MUST items, and consider addressing SHOULD's, and we should be good to go.
Comment 3 Randy Barlow 2015-10-27 23:22:21 EDT
(In reply to Rex Dieter from comment #2)
> 0.  this is actually ok, it's already wrapped inside the conditional
> %if %{with python3}
> ...
> %endif

Hi Rex! I should have been clearer - the python3-setuptools is actually repeated. It appears first outside the conditional statement, and then again inside it. I suspect the first one was supposed to be python2-setuptools.
Comment 4 Rex Dieter 2015-10-28 08:57:02 EDT
Ah, I misinterpreted that detail, thanks.  Let's amend point 0,

0.  MUST adjust the initial
BuildRequires:  python3-setuptools
present in the top/main pkg to:
BuildRequires:  python2-setuptools
Comment 5 Dennis Chen 2015-10-28 11:10:59 EDT
Hi Rex and Randy! Thanks for the feedback on the package.

I have update the SPEC file and RPM to fix all the MUSTS and the SHOULDS.

SPEC: http://unintel.tk/packaging/python-wand.spec
SRPM: http://unintel.tk/packaging/python-wand-0.4.1-2.fc22.src.rpm

Here is a link to the Koji build: 
http://koji.fedoraproject.org/koji/taskinfo?taskID=11613811
Comment 6 Upstream Release Monitoring 2015-10-28 11:13:05 EDT
barracks510's scratch build of python-wand-0.4.1-2.fc22.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11613811
Comment 7 Rex Dieter 2015-10-28 11:22:16 EDT
APPROVED and sponsored, welcome to fedora.  Please let me know if have any questions or need help with anything else.


Next steps,
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner



One other optional/non-blocker item I just noticed:

5. SHOULD remove 
BuildRequires: python-setuptools
and
BuildRequires: python3-setuptools

turns out these are already implicitly pulled in by base python packages, and afaict, are currently not recommended to explicitly list per guidelines at:
https://fedoraproject.org/wiki/Packaging:Python#BuildRequires
Comment 8 Fedora Update System 2015-10-28 16:20:56 EDT
python-wand-0.4.1-2.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-e15101edc9
Comment 9 Fedora Update System 2015-10-28 16:37:49 EDT
python-wand-0.4.1-2.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2015-98115ca1a3
Comment 10 Fedora Update System 2015-10-28 16:38:35 EDT
python-wand-0.4.1-2.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-fd9e8a4e7a
Comment 11 Fedora Update System 2015-11-01 01:58:32 EST
python-wand-0.4.1-2.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update python-wand'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-e15101edc9
Comment 12 Fedora Update System 2015-11-01 11:50:39 EST
python-wand-0.4.1-2.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'yum --enablerepo=epel-testing update python-wand'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-fd9e8a4e7a
Comment 13 Fedora Update System 2015-11-01 19:27:01 EST
python-wand-0.4.1-2.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update python-wand'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-98115ca1a3
Comment 14 Fedora Update System 2015-11-08 17:22:28 EST
python-wand-0.4.1-2.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
Comment 15 Fedora Update System 2015-11-09 19:23:23 EST
python-wand-0.4.1-2.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.
Comment 16 Fedora Update System 2015-11-23 15:06:34 EST
python-wand-0.4.1-2.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.

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