Bug 1034341 - Review Request: python-gstreamer1 - Python bindings for GStreamer
Summary: Review Request: python-gstreamer1 - Python bindings for GStreamer
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Peter Lemenkov
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 995232 1019403
TreeView+ depends on / blocked
 
Reported: 2013-11-25 16:11 UTC by Simon Farnsworth
Modified: 2013-12-28 02:07 UTC (History)
9 users (show)

Fixed In Version: python-gstreamer1-1.1.90-1.fc20
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-12-28 02:07:21 UTC
lemenkov: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Simon Farnsworth 2013-11-25 16:11:17 UTC
Spec URL: http://90.155.96.198/sfarnsworth/gstreamer1-python.spec
SRPM URL: http://90.155.96.198/sfarnsworth/gstreamer1-python-1.1.90-1.fc19.src.rpm
Description: This module contains PyGObject overrides to make it easier to write
applications that use GStreamer 1.x in Python.
Fedora Account System Username: farnz

Thomas Spura suggested that I work on this, as I've been packaging git snapshots of gst-python for my employer, and submitted the spec file I've been using to a bug he's monitoring.

This is my first package for upstream Fedora - I will need a sponsor to help me get this into Fedora once it's good enough.

I've confirmed with the aid of koji that the package builds for f20 and f21 tags. It will not build for older versions of Fedora, as GStreamer 1.2 isn't packaged until Fedora 20 - Fedora 19 has GStreamer 1.0.

Comment 1 Simon Farnsworth 2013-11-25 16:18:32 UTC
Blast - just realised I filed this using my work Bugzilla account, but I intend to do packaging from my personal account (to ensure that I don't just vanish on you).

Comment 2 Simon Farnsworth 2013-11-25 16:34:20 UTC
I should note that I'm now CC'd on this bug from my personal account, so review here will be taken into account.

Comment 3 Antonio 2013-11-25 16:36:08 UTC
Hi Simon and welcome.

- I would change Summary in "Python bindings for GStreamer1"
- Why not a python-site? macro instead of "%{_libdir}/python?.?/site-packages" ?

See http://fedoraproject.org/wiki/Packaging:Python#Macros

Comment 4 Simon Farnsworth 2013-11-25 17:53:24 UTC
Other gstreamer1 packages[1] use "GStreamer" not "GStreamer1" in the summary; I'm copying them for consistency. Is it worth breaking consistency for this one package?

I've updated the spec and the SRPM to use %{python-sitearch} for the binary locations, as they depend on the .so, and to be . I've also switched the Python dependency names as suggested on that wiki page to be explicit about the use of Python 2.

Upstream have pointed out that their next release will be able to build for Python 3 as well as Python 2, from the same source; when they make their next release, I will change this SRPM to build two binary RPMs.

I've filed a bug upstream about the wrong FSF address[2], and applied the patch to the SRPM for now, edited to only patch files that are present in the prerelease I'm packaging.

Finally, I've put the new spec file and SRPM in the same place as before:

http://90.155.96.198/sfarnsworth/gstreamer1-python.spec
http://90.155.96.198/sfarnsworth/gstreamer1-python-1.1.90-1.fc19.src.rpm

[1] I checked everything found in http://koji.fedoraproject.org/koji/search?match=glob&type=package&terms=gstreamer1*

[2] https://bugzilla.gnome.org/show_bug.cgi?id=715182

Comment 5 Simon Farnsworth 2013-11-25 18:04:30 UTC
"and to be" should have been "and to be explicit about what I'm packaging, rather than using a glob".

Comment 6 Gwyn Ciesla 2013-12-06 15:42:14 UTC
I have this that I made before I saw this bug, if it helps.  If not, I could review yours.  Up to you.

SRPM: http://fedorapeople.org/~limb/review/gstreamer1-python/gstreamer1-python-1.1.90-1.fc20.src.rpm
SPEC: http://fedorapeople.org/~limb/review/gstreamer1-python/gstreamer1-python.spec

Comment 7 Simon Farnsworth 2013-12-07 12:35:41 UTC
Whichever would be the best way to get this package maintained in upstream Fedora. I have to have it in my employer's in-house fork of Fedora, hence sharing what I'm doing with upstream (my life is easier if the divergence between upstream Fedora and the in-house fork is minimised).

I notice that your dependency list is larger than mine; I'm not sure why you're pulling in pygtk2 and gstreamer1-plugins-base in Requires (neither of them should be necessary to make this work - pygtk2 has been replaced by pygobject3 bindings, and gstreamer1-plugins-base isn't needed unless you want to use one of the plugins it contains).

You also have what look to me like outdated BuildRequires; libX11-devel, pygtk2-devel and gstreamer1-plugins-base-devel shouldn't be needed, and the release candidate versions need gstreamer1-devel >= 1.2.0.

With these differences explained away (or fixed if they're mistakes), your package looks good.

Comment 8 Simon Farnsworth 2013-12-11 15:41:10 UTC
With my package, I'm hitting an rpmlint error that I can't seem to fix; it claims that GstPbutils.py has the wrong FSF address after my patch is applied. When I inspect the file manually, it's correct, and matches the address in other files.

Any thoughts? This is the only thing I can find (with the aid of fedora-review) that causes my package to not meet the standard reivew criteria.

Comment 9 Christopher Meng 2013-12-12 04:06:53 UTC
dated FSF address problem should bve handled by upstream at least IMO.

You can ignore this warning but please report to upstream and let them fix it.

Also %{_libdir}/python?.?/ is incorrect.

Please, "rpm -E %{python2_sitearch}"

No need to add version constraint like >= 2.3, it's too old..

Comment 10 Simon Farnsworth 2013-12-12 10:17:32 UTC
I've sent a patch upstream for the dated FSF address, which has been applied to git. I've also cut that patch down and applied it in my package (Patch0: 0001-Update-FSF-address.patch). This still isn't enough to get rpmlint happy; it seems to think that

# You should have received a copy of the GNU Lesser General Public
# License along with this program; if not, write to the
# Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
# Boston, MA 02111-1307, USA.

in GstPbutils.py is an out of date address, but that 

# You should have received a copy of the GNU Lesser General Public
# License along with this program; if not, write to the
# Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
# Boston, MA 02111-1307, USA.

in Gst.py is not. I can't see what the difference is, and I've checked http://www.fsf.org/about/contact/ to confirm that it's a current address.

I was using %{python_sitearch}; as this is currently a Python 2 only package, I've updated that to %{python2_sitearch}.

What version of Python can I depend on python2-devel being? I'm currently depending on python2-devel >= 2.5, as (while I'm only packaging for Fedora 20 and above), RHEL 5 has Python 2.4, and I understood that I should make any future EPEL rebuild of this package as simple as possible.

I have, however, removed the explicit Requires for python2, as python(abi) brings in the right version of Python anyway. pygobject3 and gstreamer1 >= 1.2.0 have to stay, as the automatic dependencies don't bring them in.

I've updated the packages in the same location:

http://90.155.96.198/sfarnsworth/gstreamer1-python.spec
http://90.155.96.198/sfarnsworth/gstreamer1-python-1.1.90-1.fc19.src.rpm

Comment 11 Michael Schwendt 2013-12-12 11:07:53 UTC
See

  https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address

which mentions that file COPYING must not be patched.
rpmlint's regexp for finding wrong FSF addresses is this:

  fsf_wrong_address_regex = re.compile('(675\s+Mass\s+Ave|59\s+Temple\s+Place|Franklin\s+Steet|02139|02111-1307)', re.IGNORECASE)

The 02111-1307 should be 02110-1301 according to the Wiki.

Comment 12 Simon Farnsworth 2013-12-12 11:47:06 UTC
I wonder why it didn't complain about Gst.py as well, then.

Thanks Michael - I've sent upstream a new patch (https://bugzilla.gnome.org/show_bug.cgi?id=720317), and fixed up my local patch to not patch COPYING, just the source files.

As usual, packages updated in the same location.

http://90.155.96.198/sfarnsworth/gstreamer1-python.spec
http://90.155.96.198/sfarnsworth/gstreamer1-python-1.1.90-1.fc19.src.rpm

Comment 13 Michael Schwendt 2013-12-12 12:01:19 UTC
> I wonder why it didn't complain about Gst.py as well, then.

I've debugged it. It only peeks at the first 1024 bytes of each file. So, sort of a bug or "by design". ;-)

Comment 14 Simon Farnsworth 2013-12-12 12:21:14 UTC
Makes sense - I'm now down to just complaints about COPYING (and upstream have applied my patches), so I believe that the package should now pass review.

Comment 15 Dan Fruehauf 2013-12-14 11:40:21 UTC
Simon, things look already pretty spotless, especially if Michael is reviewing your stuff :)

I wonder though why you wouldn't just include in %files:
%{python2_sitearch}/gi/overrides/*

Can save a few good lines and make things more consistent and less sensitive to upstream changes (it's even recommended - https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25files_basics)

Comment 16 Peter Lemenkov 2013-12-14 13:22:10 UTC
I need this since my mediaplayer of choice (foobnix) moves to gstreamer1, so I'm going to sponsor Simon, and review this.

Comment 17 Peter Lemenkov 2013-12-14 13:24:35 UTC
Unblocking FE-NEEDSPONSOR - I've just sponsored Simon.

Comment 18 Simon Farnsworth 2013-12-14 13:57:16 UTC
(In reply to Dan Fruehauf from comment #15)
> Simon, things look already pretty spotless, especially if Michael is
> reviewing your stuff :)
> 
> I wonder though why you wouldn't just include in %files:
> %{python2_sitearch}/gi/overrides/*
> 
> Can save a few good lines and make things more consistent and less sensitive
> to upstream changes (it's even recommended -
> https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25files_basics)

I'll do that once Peter's done his review.

Peter, I'm going to change %files from:

%files
%doc AUTHORS COPYING ChangeLog NEWS README
%{python2_sitearch}/gi/overrides/GstPbutils.py
%{python2_sitearch}/gi/overrides/GstPbutils.pyc
%{python2_sitearch}/gi/overrides/GstPbutils.pyo
%{python2_sitearch}/gi/overrides/Gst.py
%{python2_sitearch}/gi/overrides/Gst.pyc
%{python2_sitearch}/gi/overrides/Gst.pyo
%{python2_sitearch}/gi/overrides/_gi_gst.so

to

%files
%doc AUTHORS COPYING ChangeLog NEWS README
%{python2_sitearch}/gi/overrides/*

after your review, unless you recommend that I don't.

Comment 19 Michael Schwendt 2013-12-14 14:25:41 UTC
Hmm, I haven't had a look at the spec file before...

> Name:           gstreamer1-python

Since it's a Python module to make it easier to use GStreamer within Python, it should following the %parent-%child naming guidelines for Python add-on packages, shouldn't it?

  Name: python-gstreamer1

https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.28General.29
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.28python_modules.29


> Requires:       gstreamer1 >= 1.2.0

It already depends on the libstreamer SONAME automatically, and a comment should explain why this explicit dependency with minimum version is needed:
https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

Since the dep should be arch-specific, add %{?_isa} to it: gstreamer1%{?_isa}

[...]

Using wildcards in %files lists can be convenient, but may be more fragile. In this case,

  %{python2_sitearch}/gi/overrides/*

includes "anything" and could silently (and without causing a build failure) drop parts of the API and break dependencies, whereas

  %{python2_sitearch}/gi/overrides/GstPbutils.py*
  %{python2_sitearch}/gi/overrides/Gst.py*
  %{python2_sitearch}/gi/overrides/_gi_gst.so

would be a safer compromise. It would ensure that those files must be available.

Comment 20 Simon Farnsworth 2013-12-14 14:45:08 UTC
(In reply to Michael Schwendt from comment #19)
> Hmm, I haven't had a look at the spec file before...
> 
> > Name:           gstreamer1-python
> 
> Since it's a Python module to make it easier to use GStreamer within Python,
> it should following the %parent-%child naming guidelines for Python add-on
> packages, shouldn't it?
> 
>   Name: python-gstreamer1
> 
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.
> 28General.29
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.
> 28python_modules.29
> 
I was following the example of gstreamer-python, its gstreamer 0.10 equivalent. I'll rename it to avoid having to rename later.
> 
> > Requires:       gstreamer1 >= 1.2.0
> 
> It already depends on the libstreamer SONAME automatically, and a comment
> should explain why this explicit dependency with minimum version is needed:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
> 
> Since the dep should be arch-specific, add %{?_isa} to it: gstreamer1%{?_isa}
> 
I'll add the comment and %{?_isa} - it doesn't build with less than 1.2.0, and I don't test it against older GStreamer, even though it claims the libgstreamer 1.0 SONAME.

> [...]
> 
> Using wildcards in %files lists can be convenient, but may be more fragile.
> In this case,
> 
>   %{python2_sitearch}/gi/overrides/*
> 
> includes "anything" and could silently (and without causing a build failure)
> drop parts of the API and break dependencies, whereas
> 
>   %{python2_sitearch}/gi/overrides/GstPbutils.py*
>   %{python2_sitearch}/gi/overrides/Gst.py*
>   %{python2_sitearch}/gi/overrides/_gi_gst.so
> 
> would be a safer compromise. It would ensure that those files must be
> available.

I'll make that change instead of the wide open wildcard.

New package at:
Spec URL: http://90.155.96.198/sfarnsworth/python-gstreamer1.spec
SRPM URL: http://90.155.96.198/sfarnsworth/python-gstreamer1-1.1.90-1.fc19.src.rpm

Comment 21 Simon Farnsworth 2013-12-16 16:22:46 UTC
I've now done a koji scratch build, just to show that it builds on all primary arches: http://koji.fedoraproject.org/koji/taskinfo?taskID=6301332

Comment 22 Michael Schwendt 2013-12-16 23:37:32 UTC
With regard to the Python add-on naming guidelines, feel free to follow this FPC ticket: https://fedorahosted.org/fpc/ticket/375

Comment 23 Peter Lemenkov 2013-12-17 14:34:14 UTC
Ok, here is my 

REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

+ rpmlint is almost silent

sulaco ~/rpmbuild/SPECS: rpmlint ../RPMS/ppc/gstreamer1-python-* ../SRPMS/gstreamer1-python-1.1.90-1.fc21.src.rpm 
gstreamer1-python.ppc: E: incorrect-fsf-address /usr/lib/python2.7/site-packages/gi/overrides/GstPbutils.py
gstreamer1-python.ppc: E: incorrect-fsf-address /usr/share/doc/gstreamer1-python/COPYING

^^^ These issues should be fixed upstream.

3 packages and 0 specfiles checked; 2 errors, 0 warnings.
sulaco ~/rpmbuild/SPECS: 


+ The package is named according to the  Package Naming Guidelines. I think we should stay with gstreamer1-* scheme rather than use python-* one. However I don't have any preferenceы here so feel free to choose whatever naming scheme you want (don't forget to add Provides: <> for the other one) .
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license (LGPLv2 or later).
+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided in the spec URL.

sulaco ~/rpmbuild/SOURCES: sha256sum gst-python-1.1.90.tar.bz2*
542c9f9936c95cc419da7af9c004ad34066793baba67c052f0c08c42459f5eef  gst-python-1.1.90.tar.bz2
542c9f9936c95cc419da7af9c004ad34066793baba67c052f0c08c42459f5eef  gst-python-1.1.90.tar.bz2.1
sulaco ~/rpmbuild/SOURCES:

+ The package successfully compiles and builds into binary rpms on at least one primary architecture. See koji link above.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
0 No shared library files in some of the dynamic linker's default paths.
+ The package does NOT bundle copies of system libraries.
0 The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
0 No C/C++ header files.
0 No static libraries.
0 No pkgconfig(.pc) files.
0 The package doesn't contain library files without a suffix (e.g. libfoo.so) in some of the dynamic linker's default paths.
0 No devel sub-package.
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
+ All filenames in rpm packages are valid UTF-8.


I don't see any other issues so this package is 

APPROVED.

Comment 24 Michael Schwendt 2013-12-17 14:37:55 UTC
> The package is named according to the  Package Naming Guidelines.
> I think we should stay with gstreamer1-* scheme rather than use python-* one.

What is the rationale?

The package is a Python Module that extends Python with an API for GStreamer.

It would be good, if you replied to the thread on packaging list (from October) or the newer FPC ticket.

Comment 25 Peter Lemenkov 2013-12-17 14:40:24 UTC
(In reply to Michael Schwendt from comment #24)
> > The package is named according to the  Package Naming Guidelines.
> > I think we should stay with gstreamer1-* scheme rather than use python-* one.
> 
> What is the rationale?

Because other gstreamer packets named so.

> It would be good, if you replied to the thread on packaging list (from
> October) or the newer FPC ticket.

I don't want to participate in the bikeshedding contest.

Comment 26 Simon Farnsworth 2013-12-17 14:54:45 UTC
I'm going to go with the python-gstreamer1 name (to match perl-GStreamer and rubygem-gstreamer, which are the other two language bindings for GStreamer-0.10) and add Provides: gstreamer1-python for the benefit of people migrating from the old GStreamer-0.10 name.

When upstream release a Python3 compatible version, I'll amend this package to produce python-gstreamer1 (Provides: gstreamer1-python) and python3-gstreamer1.

Comment 27 Peter Lemenkov 2013-12-17 14:56:48 UTC
(In reply to Simon Farnsworth from comment #26)
> I'm going to go with the python-gstreamer1 name (to match perl-GStreamer and
> rubygem-gstreamer, which are the other two language bindings for
> GStreamer-0.10) and add Provides: gstreamer1-python for the benefit of
> people migrating from the old GStreamer-0.10 name.
> 
> When upstream release a Python3 compatible version, I'll amend this package
> to produce python-gstreamer1 (Provides: gstreamer1-python) and
> python3-gstreamer1.

Please update this issue's title to reflect a new package's name and continue with SCM request:

https://fedoraproject.org/wiki/Package_SCM_admin_requests

Comment 28 Simon Farnsworth 2013-12-17 14:58:40 UTC
New Package SCM Request
=======================
Package Name: python-gstreamer1
Short Description: Python bindings for GStreamer
Owners: farnz
Branches: f20
InitialCC:

Comment 29 Gwyn Ciesla 2013-12-17 15:08:33 UTC
There was an error, please submit a Package Change request for f20.

Comment 30 Simon Farnsworth 2013-12-17 15:11:25 UTC
Package Change Request
======================
Package Name: python-gstreamer1
New Branches: f20
Owners: farnz
InitialCC: 

As per Jon Ciesla's comment - want the error fixed so that I can start building this for f20 as well as Rawhide.

Comment 31 Michael Schwendt 2013-12-17 15:17:04 UTC
The "Short Description" in comment 28 did not match the title of this ticket. Typcially, it must match.

I don't understand comment 29, because a Change request is only for existing packages. This is a new package, however.

Comment 32 Simon Farnsworth 2013-12-17 15:19:22 UTC
Bug title updated - the Change request is because pkgdb created the devel branch happily, but not the f20 branch.

Comment 33 Gwyn Ciesla 2013-12-17 15:21:49 UTC
Git done (by process-git-requests).

Comment 34 Fedora Update System 2013-12-17 16:22:06 UTC
python-gstreamer1-1.1.90-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/python-gstreamer1-1.1.90-1.fc20

Comment 35 Fedora Update System 2013-12-19 07:14:54 UTC
python-gstreamer1-1.1.90-1.fc20 has been pushed to the Fedora 20 testing repository.

Comment 36 Fedora Update System 2013-12-28 02:07:21 UTC
python-gstreamer1-1.1.90-1.fc20 has been pushed to the Fedora 20 stable repository.


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