Bug 1392089 - Review Request: python-zeroconf: Pure Python Multicast DNS Service Discovery Library
Summary: Review Request: python-zeroconf: Pure Python Multicast DNS Service Discovery ...
Keywords:
Status: CLOSED DUPLICATE of bug 1401337
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Athos Ribeiro
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: python-zeroconf (view as bug list)
Depends On:
Blocks: IoT 1392090
TreeView+ depends on / blocked
 
Reported: 2016-11-04 19:32 UTC by Peter Robinson
Modified: 2016-12-21 04:22 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-12-21 04:22:24 UTC
Type: Bug
Embargoed:
athoscribeiro: fedora-review?


Attachments (Terms of Use)

Comment 1 Athos Ribeiro 2016-11-05 03:26:33 UTC
Would you use the template on https://fedoraproject.org/wiki/Packaging:Python#Example_common_spec_file?
It would really improve readability with all those macros set there.

As pointed in https://fedoraproject.org/wiki/Packaging:Python#Reviewer_checklist , %python_provide macro must be used.

Are there any reasons for not running upstream's test suite under %check?
I see there's one here https://github.com/jstasiak/python-zeroconf/blob/master/test_zeroconf.py

Also, are there any reasons for not including the README file under %doc?

Comment 2 Athos Ribeiro 2016-11-05 03:49:43 UTC
There are also some missing Requires:

Comment 3 Athos Ribeiro 2016-11-16 21:09:20 UTC
*** Bug 1395341 has been marked as a duplicate of this bug. ***

Comment 4 Peter Robinson 2016-11-17 11:27:30 UTC
(In reply to Athos Ribeiro from comment #2)
> There are also some missing Requires:

Feel free to actually list them explicitly, I'll get back to this next week, I've been dealing with f25 release so that's taken priority.

Comment 6 Peter Robinson 2016-12-20 07:48:15 UTC
Please continue the review or should I get someone else to continue it?

Comment 7 Athos Ribeiro 2016-12-20 18:37:00 UTC
Hi Peter,

I am sorry for the delay here, I have been travelling in the past few weeks and probably missed the emails on this bug.

Please, see bug 1401337 and [1]

python-zeroconf was reviewed and approved during our review here, but the maintainer only included a python3-zeroconf package and we do need a python2-zeroconf for bug 1392090

We can either check if the maintainer is willing to add python2-zeroconf to the python-zeroconf package or rename this package to python2-zeroconf, remove the python3-zeroconf subpackage and proceed with the review.

I believe the former would be preferred, and if you agree, I would even contact the maintainer and send him a patch to include python2-zeroconf. Else, here is the review of the package:

I only found 2 issues here:

1 - the python3-zeroconf owns
/usr/lib/python3.5/site-packages/__pycache__
which belongs to system-python-libs. This issue appears in a few python3 packages when we use wildcards like %{python3_sitelib}/*

Since the python3 subpackage would be removed, this should be ignored.

2 - spec file line 2 reads:
%define with_tests 0

Guidelines sugest we use %global instead, as you can see in [2]. Note that this is not a must.

Other than that, the python2 package would be ready, if that's how you'd like to proceed.

[1] https://admin.fedoraproject.org/pkgdb/package/rpms/python-zeroconf/
[2] https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define

Comment 8 Peter Robinson 2016-12-21 01:29:26 UTC
> Please, see bug 1401337 and [1]
> 
> python-zeroconf was reviewed and approved during our review here, but the
> maintainer only included a python3-zeroconf package and we do need a
> python2-zeroconf for bug 1392090

That bug was submitted after this one, this one should have taken precidence.

> We can either check if the maintainer is willing to add python2-zeroconf to
> the python-zeroconf package or rename this package to python2-zeroconf,
> remove the python3-zeroconf subpackage and proceed with the review.

No it needs to be added to the other one. GRRRRRR

Comment 9 Athos Ribeiro 2016-12-21 03:53:04 UTC
(In reply to Peter Robinson from comment #8)
> > Please, see bug 1401337 and [1]
> > 
> That bug was submitted after this one, this one should have taken precidence.

Yes, they probably forgot to check bugzilla for a review request before submitting it... maybe we should have a bot checking for duplicated review requests.

> > We can either check if the maintainer is willing to add python2-zeroconf to
> > the python-zeroconf package or rename this package to python2-zeroconf,
> > remove the python3-zeroconf subpackage and proceed with the review.
> 
> No it needs to be added to the other one. GRRRRRR

OK. You said (in the other review request) you would address this, so I suppose I do not need to contact the maintainer to include a python2-zeroconf subpackage.

Comment 10 Peter Robinson 2016-12-21 04:22:24 UTC
> OK. You said (in the other review request) you would address this, so I
> suppose I do not need to contact the maintainer to include a
> python2-zeroconf subpackage.

I will, I've requested co-maintainer, if they don't respond in a reasonable time I'll just push my needed changes anyway.

Marking as duplicate even though it should be the other way around.

*** This bug has been marked as a duplicate of bug 1401337 ***


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