Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
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.