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 1392090 - Review Request: pychromecast - Python library to communicate with the Google Chromecast
Summary: Review Request: pychromecast - Python library to communicate with the Google ...
Keywords:
Status: CLOSED RAWHIDE
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:
Depends On: 1209685 1392089 1401337
Blocks: IoT
TreeView+ depends on / blocked
 
Reported: 2016-11-04 19:33 UTC by Peter Robinson
Modified: 2016-12-22 04:23 UTC (History)
2 users (show)

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

Attachments (Terms of Use)

Description Peter Robinson 2016-11-04 19:33:41 UTC 
SPEC: https://pbrobinson.fedorapeople.org/pychromecast.spec
SRPM: https://pbrobinson.fedorapeople.org/pychromecast-0.7.7-1.fc25.src.rpm

Description:
Library for Python 2 and 3 to communicate with the Google Chromecast. It
currently supports:

-  Auto discovering connected Chromecasts on the network
-  Start the default media receiver and play any online media
-  Control playback of current playing media
-  Implement Google Chromecast api v2
-  Communicate with apps via channels
-  Easily extendable to add support for unsupported namespaces
-  Multi-room setups with Audio cast devices
Comment 1 Athos Ribeiro 2016-11-05 04:05:29 UTC 
I am not sure if we can use this name for the package. See https://fedoraproject.org/wiki/Packaging:Naming?rd=Packaging:NamingGuidelines#Outdated_Python_package_naming_conventions

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 including the README file under %doc?

Any comments on why you decided to use github instead of pypi for sources? I am just curious since you used pypi for the other python package.
Comment 2 Peter Robinson 2016-12-02 16:46:54 UTC 
(In reply to Athos Ribeiro from comment #1)
> I am not sure if we can use this name for the package. See
> https://fedoraproject.org/wiki/Packaging:Naming?rd=Packaging:
> NamingGuidelines#Outdated_Python_package_naming_conventions

The binary packages are called python2- and python3- so I don't see what the issue is.
Comment 3 Peter Robinson 2016-12-02 17:05:39 UTC 
Updated, spec as before

SRPM: https://pbrobinson.fedorapeople.org/pychromecast-0.7.7-2.fc25.src.rpm
Comment 4 Peter Robinson 2016-12-20 07:48:04 UTC 
Please continue the review or should I get someone else to continue it?
Comment 5 Athos Ribeiro 2016-12-20 18:44:19 UTC 
Hi Peter,

This is not a problem in this package, but I'd suggedt to be careful with wildcards in python3 packages, since sometimes your package may end up owning %{python3_sitelib}/__pycache__, which belongs to system-python-libs

spec file line 2 reads:
%define with_tests 0

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

The Summary tag ends with period a period. Please see [2].

After your feedback on those points I will consider this review done, sice the package looks good.

We would still need to find a solution for bug 1392089 in order to include python2-zeroconf in fedora, since this bug depends on it.

[1] https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define
[2] https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections
Comment 6 Peter Robinson 2016-12-21 01:45:04 UTC 
> spec file line 2 reads:
> %define with_tests 0
> 
> Guidelines sugest we use %global instead, as you can see in [1]. Note that
> this is not a must.
> 
> The Summary tag ends with period a period. Please see [2].

Those two are minor quirks I would fix on commit. Do you want me to update them?
 
> After your feedback on those points I will consider this review done, sice
> the package looks good.
> 
> We would still need to find a solution for bug 1392089 in order to include
> python2-zeroconf in fedora, since this bug depends on it.

I'll deal with that.
Comment 7 Athos Ribeiro 2016-12-21 03:41:01 UTC 
> Those two are minor quirks I would fix on commit. Do you want me to update
> them?

Not really, I trust you will address those.

python2-zeroconf is still not in Fedora and bug 1392089 on which this bug depends on, might not solve the issue, as pointed out in that bug. I know you are an experienced packager and I do trust your judgement on how to solve the issue.

The package looks good and this review is complete. Since all dependencies are not in Fedora yet, see [1] for reference.

[1] https://fedoraproject.org/wiki/Packaging:ReviewGuidelines#A_note_on_dependencies
Comment 8 Peter Robinson 2016-12-21 03:55:42 UTC 
Fixed those two points locally, requested maint on the other package
Comment 9 Gwyn Ciesla 2016-12-21 13:37:28 UTC 
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/pychromecast
Comment 10 Peter Robinson 2016-12-22 04:23:09 UTC 
Pushed. Global and summary fixed.

Thanks for the review!
Note You need to log in before you can comment on or make changes to this bug.