Bug 1034341

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).

I should note that I'm now CC'd on this bug from my personal account, so review here will be taken into account.

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

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

"and to be" should have been "and to be explicit about what I'm packaging, rather than using a glob".

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

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.

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.

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..

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

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.

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

> 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". ;-)

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.

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 need this since my mediaplayer of choice (foobnix) moves to gstreamer1, so I'm going to sponsor Simon, and review this.

Unblocking FE-NEEDSPONSOR - I've just sponsored Simon.

(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.

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.

(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

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

With regard to the Python add-on naming guidelines, feel free to follow this FPC ticket: https://fedorahosted.org/fpc/ticket/375

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.

> 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.

(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.

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.

(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

New Package SCM Request
=======================
Package Name: python-gstreamer1
Short Description: Python bindings for GStreamer
Owners: farnz
Branches: f20
InitialCC:

There was an error, please submit a Package Change request for f20.

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.

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.

Bug title updated - the Change request is because pkgdb created the devel branch happily, but not the f20 branch.

Git done (by process-git-requests).

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

python-gstreamer1-1.1.90-1.fc20 has been pushed to the Fedora 20 testing repository.

python-gstreamer1-1.1.90-1.fc20 has been pushed to the Fedora 20 stable repository.