Bug 617141

Very informal review:

The -docs package contains files that should be in -devel.

The BuildRoot tag is no longer required, so

BuildRoot:     %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

and

rm -rf %{buildroot}

can be removed.

The -vala package should require vala, not vala-devel. Also, I don't think vala-tools is required.

Some questions/ideas:

- As Bruce suggested, please move the {_libdir}/*.so from -doc to -devel
- move /usr/lib/pkgconfig/gupnp-dlna-1.0.pc to -devel
- source url has changed to http://www.gupnp.org/sites/all/files/sources/gupnp-dlna-0.3.0.tar.gz (URL without the package-name as dir-name)
- there has been a version 0.3.0 released (3 days ago)
- rpmlint /home/mrunge/rpmbuild/RPMS/i686/gupnp-dlna-vala-0.2.1-1.fc13.i686.rpm
gupnp-dlna-vala.i686: E: devel-dependency vala-devel
gupnp-dlna-vala.i686: W: no-documentation

Uh, hit "save changes" too early.
description for -devel says Files for development with %{name}., same is for -vala subpackage. What's the difference? I would suggest to move them both to -devel package or to separate them, if possible. Why do you separate them?


I would prefer to change the description of -devel subpackage to something like The %{name}-devel package contains libraries and header files for
developing applications that use %{name}. (rpmdev-newspec-default for -devel subpackages)

Some updates including up to 0.3.0 (with a patch to fix the build):

SPEC: as above
SRPM: http://pbrobinson.fedorapeople.org/gupnp-dlna-0.3.0-1.fc14.src.rpm

koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2416946

Just had the time for a short look. Are you serious with the following lines?

%description docs
Contains libraries and header files for developing applications that use %{name}.

A full review will (hopefully) follow tomorrow.

(In reply to comment #5)
> Just had the time for a short look. Are you serious with the following lines?

Nope. I've updated that and the main description. Files as above.

Package Review
==============

Key:
- = N/A
x = Check
! = Problem
? = Not evaluated

=== REQUIRED ITEMS ===
[x]  Package is named according to the Package Naming Guidelines.
[x]  Spec file name must match the base package %{name}, in the format %{name}.spec.
[x]  Package meets the Packaging Guidelines.
[x]  Package successfully compiles and builds into binary rpms on at least one supported architecture.
Tested on: i386
[x]  Rpmlint output:
[x]  Package is not relocatable.
[-]  Buildroot is correct (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
[x]  Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
[!]  License field in the package spec file matches the actual license.
License type: GPLv2+
[x]  If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc.
[x]  Spec file is legible and written in American English.
[x]  Sources used to build the package matches the upstream source, as provided in the spec URL.
MD5SUM this package    : 5ed39d671c8631a620192c7a7aabaca9
MD5SUM upstream package: 5ed39d671c8631a620192c7a7aabaca9
[x]  Package is not known to require ExcludeArch, OR:
[x]  All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
[-]  The spec file handles locales properly.
[x]  ldconfig called in %post and %postun if required.
[x]  Package must own all directories that it creates.
[x]  Package requires other packages for directories it uses.
[x]  Package does not contain duplicates in %files.
[x]  Permissions on files are set properly.
[x]  Package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[x]  Package consistently uses macros.
[x]  Package contains code, or permissable content.
[x]  Large documentation files are in a -doc subpackage, if required.
[x]  Package uses nothing in %doc for runtime.
[x]  Header files in -devel subpackage, if present.
[-]  Static libraries in -devel subpackage, if present.
[!]  Package requires pkgconfig, if .pc files are present.
[x]  Development .so files in -devel subpackage, if present.
[x]  Fully versioned dependency in subpackages, if present.
[x]  Package does not contain any libtool archives (.la).
[-]  Package contains a properly installed %{name}.desktop file if it is a GUI application.
[x]  Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
[x]  Latest version is packaged.
[x]  Package does not include license text files separate from upstream.
[-]  Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
[x]  Reviewer should test that the package builds in mock.
Tested on: koji, http://koji.fedoraproject.org/koji/taskinfo?taskID=2419260
[x]  Package should compile and build into binary rpms on all supported architectures.
Tested on: i686, x86_64
[?]  Package functions as described.
[x]  Scriptlets must be sane, if used.
[x]  The placement of pkgconfig(.pc) files are correct.
[x]  File based requires are sane.


=== Issues ===
1. IMHO License field should be GPLv2+ and not LGPL...
2. Package -devel must require pkgconfig, because of .pc-files present

If you fix those two issues, I'll approve this package.

> === Issues ===
> 1. IMHO License field should be GPLv2+ and not LGPL...

Fixed

> 2. Package -devel must require pkgconfig, because of .pc-files present

I read the packaging guidelines that rpmbuild will auto detect and add the appropriate pkgconfig dependency

https://fedoraproject.org/wiki/Packaging/Guidelines#Pkgconfig_Files

SPEC: as above
SRPM: http://pbrobinson.fedorapeople.org/gupnp-dlna-0.3.0-2.fc13.src.rpm

(In reply to comment #7)

> === Issues ===
> 1. IMHO License field should be GPLv2+ and not LGPL...

What makes you think so? From my quick observation it seems that LGPLv2+ seems to be correct. Did you spot something that says it is GPLv2+?

There seems to be a third issue: The tarball includes a copy of gst-convenience and seems to use it for building. But this is a separate project:
http://git.collabora.co.uk/?p=user/edward/gst-convenience.git;a=tree

Therefore it violates the "no bundled libaries" guideline:
https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

Peter, you're right regarding pkgconfig-files and the dependency. 

(In reply to comment #9)
> (In reply to comment #7)
> 
> > === Issues ===
> > 1. IMHO License field should be GPLv2+ and not LGPL...
> 
> What makes you think so? From my quick observation it seems that LGPLv2+ seems
> to be correct. Did you spot something that says it is GPLv2+?
> 
As far as I can see, the COPYING files calls itself
  GNU LIBRARY GENERAL PUBLIC LICENSE  Version 2, June 1991
(taken from here:
http://gitorious.org/gupnp/gupnp-vala/blobs/master/COPYING

Looking at e.g. ../BUILD/gupnp-dlna-0.2.1/libgupnp-dlna/gupnp-dlna-discoverer.c
I can not find the word LESSER license, but GPL v2 or any later version.

> There seems to be a third issue: The tarball includes a copy of gst-convenience
> and seems to use it for building. But this is a separate project:
> http://git.collabora.co.uk/?p=user/edward/gst-convenience.git;a=tree
> 
> Therefore it violates the "no bundled libaries" guideline:
> https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

Till is right, I missed that. Can you separate it?

(In reply to comment #10)
> Peter, you're right regarding pkgconfig-files and the dependency. 

Cool. Will leave as is.

> As far as I can see, the COPYING files calls itself
>   GNU LIBRARY GENERAL PUBLIC LICENSE  Version 2, June 1991
> (taken from here:
> http://gitorious.org/gupnp/gupnp-vala/blobs/master/COPYING
> 
> Looking at e.g. ../BUILD/gupnp-dlna-0.2.1/libgupnp-dlna/gupnp-dlna-discoverer.c
> I can not find the word LESSER license, but GPL v2 or any later version.

Speaking with upstream the licensing in wrong and there will be a new upstream release shortly to fix the problem.

> > There seems to be a third issue: The tarball includes a copy of gst-convenience
> > and seems to use it for building. But this is a separate project:
> > http://git.collabora.co.uk/?p=user/edward/gst-convenience.git;a=tree
> > 
> > Therefore it violates the "no bundled libaries" guideline:
> > https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
> 
> Till is right, I missed that. Can you separate it?

Its not meant to be separated, speaking with upstream its a staging library that will be moved to upstream gst stuff. Its covered in this bug https://bugzilla.gnome.org/show_bug.cgi?id=625944 and it will be dropped and the gst dependencies updated once its there.

(In reply to comment #10)

> As far as I can see, the COPYING files calls itself
>   GNU LIBRARY GENERAL PUBLIC LICENSE  Version 2, June 1991
        ^^^^^^^
> (taken from here:
> http://gitorious.org/gupnp/gupnp-vala/blobs/master/COPYING
> 
> Looking at e.g. ../BUILD/gupnp-dlna-0.2.1/libgupnp-dlna/gupnp-dlna-discoverer.c
> I can not find the word LESSER license, but GPL v2 or any later version.

I cannot find the string GPL there, but again "GNU Library General Public"
                                                   ^^^^^^^
This is the older name of the GNU Lesser General Public License:
http://www.gnu.org/licenses/old-licenses/lgpl-2.0.html

(In reply to comment #11)
> (In reply to comment #10)
> > Peter, you're right regarding pkgconfig-files and the dependency. 
> 
> Cool. Will leave as is.
> 
> > As far as I can see, the COPYING files calls itself
> >   GNU LIBRARY GENERAL PUBLIC LICENSE  Version 2, June 1991
> > (taken from here:
> > http://gitorious.org/gupnp/gupnp-vala/blobs/master/COPYING
> > 
> > Looking at e.g. ../BUILD/gupnp-dlna-0.2.1/libgupnp-dlna/gupnp-dlna-discoverer.c
> > I can not find the word LESSER license, but GPL v2 or any later version.
> 
> Speaking with upstream the licensing in wrong and there will be a new upstream
> release shortly to fix the problem.

How are they going to fix this? Will they change from Library to Lesser to plain GPL?

> > > There seems to be a third issue: The tarball includes a copy of gst-convenience
> > > and seems to use it for building. But this is a separate project:
> > > http://git.collabora.co.uk/?p=user/edward/gst-convenience.git;a=tree
> > > 
> > > Therefore it violates the "no bundled libaries" guideline:
> > > https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
> > 
> > Till is right, I missed that. Can you separate it?
> 
> Its not meant to be separated, speaking with upstream its a staging library
> that will be moved to upstream gst stuff. Its covered in this bug
> https://bugzilla.gnome.org/show_bug.cgi?id=625944 and it will be dropped and
> the gst dependencies updated once its there.

This is probably ok then. Have you considered mentioning this in the spec or opening a bug about it once the package is imported to make sure this is not forgotten e.g. in case it is merged in gstreamer but still shipped with gupnp-dlna?

(In reply to comment #12)
                         ^^^^^^^
> This is the older name of the GNU Lesser General Public License:
> http://www.gnu.org/licenses/old-licenses/lgpl-2.0.html

This is now also documented in the Fedora Licensing wiki page:
https://fedoraproject.org/w/index.php?title=Licensing%3AMain&diff=193909&oldid=192725

so from the above I believe the Lesser and the library are the same LGPL license and hence my original license tag was correct. So if I read it all correctly I should put a comment in the spec file regarding the upstream bug and the reason for inclusion gst-convenience and we should be right to go, or did I miss something else?

(In reply to comment #15)
> so from the above I believe the Lesser and the library are the same LGPL
> license and hence my original license tag was correct. So if I read it all
> correctly I should put a comment in the spec file regarding the upstream bug
> and the reason for inclusion gst-convenience and we should be right to go, or
> did I miss something else?

You did not miss anything. With me it is fine if you put in the comment after the import to Fedora. The next step is now for Matthias to approve this review if he agrees with everything.

OK, if we're going the way to report the included library upstream and let them fix the inclusion/parting, 

I APPROVE this package.

Thanks all. I'll add the comment on the initial import.


New Package GIT Request
=======================
Package Name: gupnp-dlna
Short Description: A collection of helpers for building DLNA applications
Owners: pbrobinson
Branches: F-14 F-13
InitialCC:

How long until the bundled lib is no longer needed?
Can you wait to import this until that is fixed?

I worry that if we import it now and something happens never to drop that 
we will be stuck with another package bundling things. ;(

(In reply to comment #19)
> How long until the bundled lib is no longer needed?
> Can you wait to import this until that is fixed?
> 
> I worry that if we import it now and something happens never to drop that 
> we will be stuck with another package bundling things. ;(

speaking with upstream they don't want it to be long but their concern is that the upstream issues might not be (but may also be) resolved by gnome 2.32. So from what they've told me it might or might not be resolved by the current gnome release but will be by the next. So for us we might have it for the F-14 release but it shouldn't be there by F-15 (and if its there for the final f-14 release its likely to be gone after as we update gstreamer regularly).

Well, in order to add it now you would need to request an exception from 
FESCo to allow the bundling. Or you could wait until it's no longer needed.

How would i do that?

File a ticket in the fesco trac: 

https://fedorahosted.org/fesco/newtplticket

Add 'meeting' keyword and we will discuss it at the next meeting (tuesday).

https://fedorahosted.org/fesco/ticket/455

About the bundling worries:

As far as I can see this is the typical way new libraries are developed in GNOME and GStreamer-land. For anyone involved with GNOME, consider this similar to libegg.
The process goes something like this:

First, ship the files as in-package copy in the programs that need it. This allows rapid changes to API, code etc. During this time, the library is not released as a separate product, it's explicitly targeted for copy/paste.

When the API is agreed on and deemed stable, import it into GStreamer's support library collection: gstreamer-plugins-base, which then guarantees API stability.

So the only technical problem is to not get conflicts when header or library files from different projects shipping the code or from when the library finally does get released. And from a quick look at the package, they have put the files into the gupnp-dlna namespace, so everything is fine.

Last but not least, all of this work is happening inside the GStreamer community, so I don't think the typical worries about bundling apply here.

This has been approved by fesco

https://fedorahosted.org/fesco/ticket/455#comment:6

New Package GIT Request
=======================
Package Name: gupnp-dlna
Short Description: A collection of helpers for building DLNA applications
Owners: pbrobinson
Branches: F-14 F-13
InitialCC:

Git done (by process-git-requests).

Built and in rawhide. Added the following line the to spec as requested.
Provides: bundled(gst-convenience)