Bug 832999 - Review Request: colord-gtk - GTK support library for colord
Review Request: colord-gtk - GTK support library for colord
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Debarshi Ray
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-18 07:23 EDT by Richard Hughes
Modified: 2012-06-27 04:37 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-06-27 04:37:00 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rhughes: fedora‑review?
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Richard Hughes 2012-06-18 07:23:51 EDT
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 07:01:21 EDT
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 10:25:21 EDT
(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 11:05:26 EDT
* 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 11:13:25 EDT
(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 11:19:40 EDT
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 11:37:26 EDT
+ 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 12:17:57 EDT
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 13:27:31 EDT
Git done (by process-git-requests).
Comment 9 Richard Hughes 2012-06-27 04:37:00 EDT
Built for F17 and rawhide, thanks!

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