Bug 653378 - Review Request: gtk-theme-engine-clearlooks - Clearlooks GTK+ theme engine
Summary: Review Request: gtk-theme-engine-clearlooks - Clearlooks GTK+ theme engine
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Matthias Clasen
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2010-11-15 10:17 UTC by Cosimo Cecchi
Modified: 2011-01-25 17:32 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2011-01-25 17:32:32 UTC
Type: ---
mclasen: fedora-review+
j: fedora-cvs+

Attachments (Terms of Use)

Comment 1 Matthias Clasen 2010-11-16 13:55:16 UTC
builds fine in mock.
rpmlint output:

$ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/gtk-theme-engine-clearlooks-*.rpm
gtk-theme-engine-clearlooks.src:13: W: unversioned-explicit-obsoletes gtk3-engines
gtk-theme-engine-clearlooks.src: W: no-cleaning-of-buildroot %install
gtk-theme-engine-clearlooks.src: W: no-cleaning-of-buildroot %clean
gtk-theme-engine-clearlooks.src: W: no-buildroot-tag
gtk-theme-engine-clearlooks.src: W: no-%clean-section
gtk-theme-engine-clearlooks.x86_64: W: obsolete-not-provided gtk3-engines
gtk-theme-engine-clearlooks.x86_64: W: devel-file-in-non-devel-package /usr/lib64/pkgconfig/gtk-theme-engine-clearlooks-3.pc
3 packages and 0 specfiles checked; 0 errors, 7 warnings.

The obsoletes is questionable anyway, in my opinion. We should leave it out.
The .pc file indeed needs to go into a sucky, single-file -devel package. I'm going to advocate for dropping it from the upstream tarball, since it is useless.

Comment 2 Matthias Clasen 2010-11-16 14:13:09 UTC
package name: ok
spec file name: ok
packaging guidelines: minor issues:
 - the explicit dep on gtk3 should be dropped, since it will get pulled in
   by library deps anyway.
 - as mentioned, the .pc file needs to go in a -devel package (until we get
   rid of it upstream)
license: ok
license field: copying says GPLv3...
license file: should be included, after verifying what license was intended here
spec language: ok
spec readable: ok
upstream sources: ok
buildable: ok
excludearch: ok
build requires: ok
locale handling: ok
ldconfig: ok
system libraries: ok
relocatable: ok
directory ownership: must own %{_datadir}/gtk-theme-engine-clearlooks
duplicate files: ok
file permissions: ok
macro use: ok
permissible content: ok
large docs: ok
doc content: ok
headers: ok
static libs: ok
shared libs: ok
-devel deps: ok
libtool archives: the .la file needs to be dropped
gui apps: ok
duplicate ownership: ok
utf8 filenames: ok

Comment 3 Cosimo Cecchi 2010-11-18 13:38:46 UTC
Matthias: thanks for the review!

Updated spec: http://people.gnome.org/~cosimoc/gtk-theme-engine-clearlooks-pkg/gtk-theme-engine-clearlooks.spec

New SRPM: http://people.gnome.org/~cosimoc/gtk-theme-engine-clearlooks-pkg/gtk-theme-engine-clearlooks-2.91.3-2.fc15.src.rpm

Some comments...

License: I double-checked and it seems that upstream has no COPYING file at all. We should probably provide a patch for upstream with it and add it to %doc for the next upstream release. Anyway, I assumed the gtk-engines COPYING was valid for this package too, and that's LGPLv2; the source code headers say LGPLv2 too. Where did you find GPLv3 mentioned for this package?

Obsoletes: if I understood correctly how spec files work, it's actually required to obsolete gtk3-engines, at least in its current form, as both ship the same libclearlooks.so & friends files. The best approach for this would be probably to provide again a patch for upstream which removes the clearlooks engine from gtk-engines, so we can still package both without conflicts.

Comment 4 Matthias Clasen 2010-11-18 14:06:45 UTC
For the license: since this is basically all just copied clearlooks code, it certainly needs to stay under the same license as gtk3-engines, and the failure to include COPYING in git just gives us that automake bug where it just makes up a license. I'll get the license added upstream.

For the obsoletes, what needs to happen is that we remove clearlooks from gtk3-engines at the same time that we introduce this package. We can probably make the transition a bit smoother by having this package conflict with gtk3-engines <= {current version}. The coming merge of the gtk-style-context branch will make gtk3-engines obsolete anyway (at least until somebody ports the engines).

Comment 5 Matthias Clasen 2010-11-19 14:07:20 UTC
I have now

- added the correct license upstream
- removed clearlooks from the gtk3-engines package

So I think we are almost ready to go here. Some changes I would still like to see:

1) %doc AUTHORS NEWS README should be in the main package, not in -devel
2) add a comment next to it explaining that the COPYING in the tarball is an automake artifact, and the correct license file has been added upstream

Comment 7 Matthias Clasen 2010-11-19 17:46:43 UTC
I don't think that Obsoletes is right.
I'd just make that a Conflicts.
Everything else looks good now.

I'll approve it, please remove or replace the Obsoletes before building it.

Comment 8 Cosimo Cecchi 2010-11-19 17:57:06 UTC
Ok, I just removed the Obsoletes then, as we already have a new gtk3-engines package without clearlooks in rawhide.

Comment 9 Matthias Clasen 2010-11-19 21:40:06 UTC
I already set approved+, so feel free to request git (don't forget to ask for an f14 branch, otherwise the initial import will fail)

Comment 10 Cosimo Cecchi 2010-11-20 12:02:27 UTC
New Package SCM Request
Package Name: gtk-theme-engine-clearlooks
Short Description: Clearlooks GTK+ theme engine
Owners: cosimoc
Branches: f14 f15
InitialCC: cosimoc

Comment 11 Jason Tibbitts 2010-11-22 13:59:34 UTC
It is too early to request f15 branches; I have made only the f14 branch.

Git done (by process-git-requests).

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