Bug 653378 - Review Request: gtk-theme-engine-clearlooks - Clearlooks GTK+ theme engine
Review Request: gtk-theme-engine-clearlooks - Clearlooks GTK+ theme engine
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Matthias Clasen
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-15 05:17 EST by Cosimo Cecchi
Modified: 2011-01-25 12:32 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-01-25 12:32:32 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mclasen: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Comment 1 Matthias Clasen 2010-11-16 08:55:16 EST
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 09:13:09 EST
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 08:38:46 EST
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 09:06:45 EST
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 09:07:20 EST
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 12:46:43 EST
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 12:57:06 EST
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 16:40:06 EST
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 07:02:27 EST
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 08:59:34 EST
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.