Bug 698692 - Review Request: grilo-plugins - Plugins for the Grilo framework
Summary: Review Request: grilo-plugins - Plugins for the Grilo framework
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kalev Lember
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-04-21 14:34 UTC by Bastien Nocera
Modified: 2011-11-14 22:24 UTC (History)
3 users (show)

Fixed In Version: grilo-plugins-0.1.15-4.fc15
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-07-19 07:05:52 UTC
Type: ---
Embargoed:
kalevlember: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Bastien Nocera 2011-04-21 14:34:10 UTC
Spec URL: http://people.fedoraproject.org/~hadess/grilo/grilo-plugins.spec
SRPM URL: http://people.fedoraproject.org/~hadess/grilo/grilo-plugins-0.1.15-1.src.rpm
Description: Grilo is a framework that provides access to different sources of
multimedia content, using a pluggable system.
This package contains plugins to get information from theses sources:
- Apple Trailers
- Bookmarks
- Filesystem
- Flickr
- Gravatar
- Jamendo
- Last.fm (for album arts)
- Local metadata (album arts and thumbnails)
- Metadata Store
- Podcasts
- Shoutcast
- Tracker
- UPnP
- Vimeo
- Youtube

Comment 1 Bastien Nocera 2011-05-04 14:24:29 UTC
Grilo itself is built in F15 and rawhide, removing the dependency. Anyone interested in reviewing the package?

Comment 3 Kalev Lember 2011-05-20 13:35:11 UTC
Taking for review.

1) The BuildRoot tag, the 'rm -rf $RPM_BUILD_ROOT' at the beginning of %install section, the whole %clean section, and the %defattr lines are no longer needed in current Fedora releases and you could remove them, if you want to.

2) You are removing %{_libdir}/grilo-0.1/*.a in %install section after building them, but it might be cleaner to avoid building the static libs in the first place. Does configure --disable-static work for this package?

3) Some of the directories aren't owned by any package and that would cause empty directories to be left behind when uninstalling the package.
Either (a) grilo, or  (b) all grilo's plugin packages should own these dirs:
%{_libdir}/grilo-0.1/
%{_datadir}/grilo-0.1/
%{_datadir}/grilo-0.1/plugins/

4) The source URL currently looks like this:
http://ftp.gnome.org/pub/GNOME/sources/grilo-plugins/0.1/grilo-plugins-%{version}.tar.bz2

It might be nice to automatically figure out the '0.1' directory name, so that it would only be necessary to update the Version: field when building updates. I have used shell scripting in a some packages to accomplish that; not sure if it's something you'd want to use here.

At the top of the spec file I've put this:
# first two digits of version
%define release_version %(echo %{version} | awk -F. '{print $1"."$2}')

... and in the Source0 tag, instead of the hardcoded '0.1' goes %{release_version} macro.

Comment 4 Bastien Nocera 2011-05-20 13:41:27 UTC
(In reply to comment #3)
> Taking for review.
> 
> 1) The BuildRoot tag, the 'rm -rf $RPM_BUILD_ROOT' at the beginning of %install
> section, the whole %clean section, and the %defattr lines are no longer needed
> in current Fedora releases and you could remove them, if you want to.

Done.

> 2) You are removing %{_libdir}/grilo-0.1/*.a in %install section after building
> them, but it might be cleaner to avoid building the static libs in the first
> place. Does configure --disable-static work for this package?

That works indeed.

> 3) Some of the directories aren't owned by any package and that would cause
> empty directories to be left behind when uninstalling the package.
> Either (a) grilo, or  (b) all grilo's plugin packages should own these dirs:
> %{_libdir}/grilo-0.1/
> %{_datadir}/grilo-0.1/
> %{_datadir}/grilo-0.1/plugins/

I'll add those directories in grilo itself.

> 4) The source URL currently looks like this:
> http://ftp.gnome.org/pub/GNOME/sources/grilo-plugins/0.1/grilo-plugins-%{version}.tar.bz2
> 
> It might be nice to automatically figure out the '0.1' directory name, so that
> it would only be necessary to update the Version: field when building updates.
> I have used shell scripting in a some packages to accomplish that; not sure if
> it's something you'd want to use here.

Pretty much every single GNOME tarball has that exact same problem.

> At the top of the spec file I've put this:
> # first two digits of version
> %define release_version %(echo %{version} | awk -F. '{print $1"."$2}')
> 
> ... and in the Source0 tag, instead of the hardcoded '0.1' goes
> %{release_version} macro.

But I've done that now.

Comment 6 Kalev Lember 2011-05-20 14:21:40 UTC
(In reply to comment #4)
> > 1) The BuildRoot tag, the 'rm -rf $RPM_BUILD_ROOT' at the beginning of %install
> > section, the whole %clean section, and the %defattr lines are no longer needed
> > in current Fedora releases and you could remove them, if you want to.
> 
> Done.

I'm just nitpicking here, it's not very important, but ... looks like you missed the the BuildRoot definition near the top of the spec file and the %clean section, these can also be removed.


> > 4) The source URL currently looks like this:
[...]
> Pretty much every single GNOME tarball has that exact same problem.

Yeah. If we can figure out a good way to do it, it's something that other GNOME packages could also use.

Comment 7 Kalev Lember 2011-05-20 14:22:32 UTC
Fedora review grilo-plugins-0.1.15-3.src.rpm 2011-05-20

+ OK
! needs attention

koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3083534

$ rpmlint grilo-plugins \
          grilo-plugins-0.1.15-3.src.rpm \
          grilo-plugins-debuginfo-0.1.15-3.x86_64.rpm
grilo-plugins.src: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
grilo-plugins.src: W: spelling-error %description -l en_US fm -> FM, Fm, gm
grilo-plugins.src: W: spelling-error %description -l en_US metadata -> meta data, meta-data, metatarsal
grilo-plugins.x86_64: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
grilo-plugins.x86_64: W: spelling-error %description -l en_US fm -> FM, Fm, gm
grilo-plugins.x86_64: W: spelling-error %description -l en_US metadata -> meta data, meta-data, metatarsal
3 packages and 0 specfiles checked; 0 errors, 6 warnings.

+ rpmlint warnings are harmless and can be ignored
+ The package is named according to Fedora packaging guidelines
+ The spec file name matches the package base name
+ 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 spec file matches the actual license
+ The package contains the license file (COPYING)
+ Spec file is written in American English
+ Spec file is legible
+ Upstream sources match sources in the srpm. md5sum:
  29efa89f3842787c21271a7513a9a1ab  grilo-plugins-0.1.15.tar.bz2
  29efa89f3842787c21271a7513a9a1ab  Download/grilo-plugins-0.1.15.tar.bz2
+ The package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane
n/a The spec file MUST handle locales properly
n/a ldconfig in %post and %postun
+ Package does not bundle copies of system libraries
n/a Package isn't relocatable
! Package currently doesn't own all directories it creates,
  but it's going to be fixed in grilo package as per comment #4
+ No duplicate files in %files
+ Permissions are properly set
+ Consistent use of macros
+ The package must contain code or permissible content
n/a Large documentation files should go in -doc subpackage
+ Files marked %doc should not affect package
n/a Header files should be in -devel
n/a Static libraries should be in -static
n/a Library files that end in .so must go in a -devel package
  It's a plugin package, not applicable.
n/a -devel must require the fully versioned base
+ Packages should not contain libtool .la files
n/a Packages containing GUI apps must include %{name}.desktop file
+ Directory ownership sane
+ Filenames are valid UTF-8


Remaining issues:
 - add the missing directories to grilo package;
 - take a look at comment #6 if you want to remove BuildRoot and %clean
 - I'd suggest to add the %{?dist} macro to Release

These can all be fixed before importing the package.

APPROVED

Comment 8 Bastien Nocera 2011-05-20 14:37:16 UTC
grilo-0.1.15-3.fc16 is built, so I'll add it to the same update.

http://people.fedoraproject.org/~hadess/grilo/grilo-plugins.spec
and
http://people.fedoraproject.org/~hadess/grilo/grilo-plugins-0.1.15-4.fc15.src.rpm

Comment 9 Bastien Nocera 2011-05-20 14:39:14 UTC
New Package SCM Request
=======================
Package Name: grilo-plugins
Short Description: Plugins for the Grilo framework
Owners: hadess
Branches: f15 el6
InitialCC:

Comment 10 Kalev Lember 2011-05-20 14:52:12 UTC
If you're building for el6, I believe it needs the %clean section:
https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean

Comment 11 Kevin Fenzi 2011-05-20 22:30:00 UTC
Git done (by process-git-requests).

Comment 12 Bastien Nocera 2011-05-22 02:00:15 UTC
(In reply to comment #10)
> If you're building for el6, I believe it needs the %clean section:
> https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean

I don't want to build for el6, so I really wouldn't worry.

Comment 13 Bastien Nocera 2011-05-22 02:01:45 UTC
Huh, I see that the request included el6. I don't know why I copied that from the example page. I don't intend on building it for RHEL 6. Ever.

Comment 14 Fedora Update System 2011-05-22 02:24:12 UTC
grilo-plugins-0.1.15-4.fc15,grilo-0.1.15-3.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/grilo-plugins-0.1.15-4.fc15,grilo-0.1.15-3.fc15

Comment 15 Fedora Update System 2011-05-25 02:33:20 UTC
grilo-plugins-0.1.15-4.fc15, grilo-0.1.15-3.fc15 has been pushed to the Fedora 15 testing repository.

Comment 16 Fedora Update System 2011-11-14 22:24:40 UTC
grilo-plugins-0.1.15-4.fc15, grilo-0.1.15-3.fc15 has been pushed to the Fedora 15 stable repository.


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