Bug 832999

Summary: Review Request: colord-gtk - GTK support library for colord
Product: [Fedora] Fedora Reporter: Richard Hughes <rhughes>
Component: Package ReviewAssignee: Debarshi Ray <debarshir>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: debarshir, notting, package-review
Target Milestone: ---Flags: rhughes: fedora-review?
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-06-27 08:37:00 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Richard Hughes 2012-06-18 11:23:51 UTC
Spec URL: http://people.freedesktop.org/~hughsient/temp/colord-gtk.spec
SRPM URL: http://people.freedesktop.org/~hughsient/temp/colord-gtk-0.1.1-1.fc17.src.rpm
Koji URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=4173465
Description: colord is a support library for colord and provides additional functionality that requires GTK+.
Fedora Account System Username: rhughes

$ rpmlint */colord-gtk*
4 packages and 1 specfiles checked; 0 errors, 0 warnings.

Note: colord-gtk.so used to be shipped in the main colord package, but this made it hard to rebuild a distro (e.g. for ARM) as there was a dependency loop of colord->GTK and GTK->colord. Now we can build in the order: colord, gtk, colord-gtk, gnome-color-manager, etc

Please review. I don't think I need to provide any conflicts for previous versions of colord, as nothing requires colord-gtk that's been built for F18 or older (colord-gtk is pretty new). If you think it's safer to just add a "Conflicts: colord <= 0.1.22" then yell. Thanks.

Comment 1 Debarshi Ray 2012-06-26 11:01:21 UTC
There is a typo in the %description. It says: "colord is a support library for colord ...". Should be "colord-gtk is a ...".

Comment 2 Richard Hughes 2012-06-26 14:25:21 UTC
(In reply to comment #1)
> There is a typo in the %description. It says: "colord is a support library
> for colord ...". Should be "colord-gtk is a ...".

I've replaced it with %{name}, thanks. :)

Comment 3 Debarshi Ray 2012-06-26 15:05:26 UTC
* libcolord-gtk/cd-sample-window.[ch] & client/cd-convert.c are under GPLv2+ according to their respective license notices. Also COPYING has the text of the GPLv2 instead of LGPLv2.

* You don't really need to specify glib2-devel as a BuildRequires because it is going to be pulled in by gtk3-devel, gobject-introspection-devel, colord-devel, etc.. But it is up to you.

* ColordGtk-1.0.gir should be part of the devel package and %{_libdir}/girepository-1.0/ColordGtk-1.0.typelib should be part of the base package.

* The devel package should own %{_datadir}/gir-1.0.

* The devel package should own %{_datadir}/gtk-doc and %{_datadir}/gtk-doc/html, but not %{_datadir}/gtk-doc/data. The other alternative is to Require: gtk-doc, but it is better to avoid it.

* You could mark %{_datadir}/gtk-doc/html/colord-gtk as %doc

* %defattr(-,root,root,-) is not needed these days.
  (http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions)

* It is better to require the base package from the devel package as:
  Requires: %{name}%{?_isa} = %{version}-%{release}
  (http://fedoraproject.org/wiki/Packaging/Guidelines#RequiringBasePackage)

Comment 4 Richard Hughes 2012-06-26 15:13:25 UTC
(In reply to comment #3)
> * libcolord-gtk/cd-sample-window.[ch] & client/cd-convert.c are under GPLv2+
> according to their respective license notices. Also COPYING has the text of
> the GPLv2 instead of LGPLv2.

Both fixed upstream, thanks.

> * You don't really need to specify glib2-devel as a BuildRequires because it
> is going to be pulled in by gtk3-devel, gobject-introspection-devel,
> colord-devel, etc.. But it is up to you.

I'd rather include this, as i can dep on a higher version of glib if I need to in the future.

> * ColordGtk-1.0.gir should be part of the devel package and
> %{_libdir}/girepository-1.0/ColordGtk-1.0.typelib should be part of the base
> package.

My mistake, fixed.

> * The devel package should own %{_datadir}/gir-1.0.

Fixed.

> * The devel package should own %{_datadir}/gtk-doc and
> %{_datadir}/gtk-doc/html, but not %{_datadir}/gtk-doc/data. The other
> alternative is to Require: gtk-doc, but it is better to avoid it.

Fixed.

> * You could mark %{_datadir}/gtk-doc/html/colord-gtk as %doc

Fixed.

> * %defattr(-,root,root,-) is not needed these days.
>   (http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions)

Fixed.

> * It is better to require the base package from the devel package as:
>   Requires: %{name}%{?_isa} = %{version}-%{release}
>   (http://fedoraproject.org/wiki/Packaging/Guidelines#RequiringBasePackage)

Fixed.

Comment 5 Richard Hughes 2012-06-26 15:19:40 UTC
New builds:

Spec URL: http://people.freedesktop.org/~hughsient/temp/colord-gtk.spec
SRPM URL: http://people.freedesktop.org/~hughsient/temp/colord-gtk-0.1.22-1.fc17.src.rpm

Thanks for the review :)

Comment 6 Debarshi Ray 2012-06-26 15:37:26 UTC
+ COPYING has LGPLv3, which is probably just a mistake. In that case omit it from the package for the time being, and restore it when it contains the correct text. Probably with the next upstream release.
  (http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text)


+----------+
| APPROVED |
+----------+

Comment 7 Richard Hughes 2012-06-26 16:17:57 UTC
New Package SCM Request
=======================
Package Name: colord-gtk
Short Description: GTK support library for colord
Owners: rhughes
Branches: f17
InitialCC: rhughes

Comment 8 Gwyn Ciesla 2012-06-26 17:27:31 UTC
Git done (by process-git-requests).

Comment 9 Richard Hughes 2012-06-27 08:37:00 UTC
Built for F17 and rawhide, thanks!