Bug 447477 - Review Request: qgtkstyle - Qt style rendering using GTK+ themes
Summary: Review Request: qgtkstyle - Qt style rendering using GTK+ themes
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Kofler
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: Qt44
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-05-20 01:22 UTC by Shawn Starr
Modified: 2008-07-03 22:39 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-07-03 22:39:41 UTC
Type: ---
Embargoed:
kevin: fedora-review+
tcallawa: fedora-cvs+


Attachments (Terms of Use)

Description Shawn Starr 2008-05-20 01:22:00 UTC
Spec URL: http://www.sh0n.net/spstarr/fedora/qgtkstyle/result/qgtkstyle.spec
SRPM URL: http://www.sh0n.net/spstarr/fedora/qgtkstyle/result/qgtkstyle-0.0-0.1.svn597.fc10.src.rpm

Mock Results: http://www.sh0n.net/spstarr/fedora/qgtkstyle/result/

Description: This is a Qt style rendered using GTK to give a native 
appearence for Qt applications running on the GNOME desktop.

Comment 1 Shawn Starr 2008-05-20 01:22:27 UTC
License: GPLv2 only

Comment 2 Kevin Kofler 2008-05-20 03:30:08 UTC
Huh, their Google Code page http://code.google.com/p/qgtkstyle/ claims it's 
GPLv3, yet the code says GPLv2. I guess we should get the Trolltech folks to 
slap the standard Qt GPLv[23] header on it, but it's not a blocker, GPLv2 is an 
acceptable license of course.

Comment 3 Rex Dieter 2008-05-22 19:23:07 UTC
Probably want to drop the hard-coded:
Requires:	gtk2

Comment 4 Kevin Kofler 2008-05-25 05:35:35 UTC
I'm currently reviewing this. First remark: as this requires Qt 4.4, it cannot 
be pushed to F9 (or F8) before Qt 4.4 is pushed. You can certainly request the 
branches in advance though, just not build them.

Comment 5 Kevin Kofler 2008-05-25 06:17:10 UTC
MUST Items:
+ rpmlint output OK:
qgtkstyle.src: W: mixed-use-of-spaces-and-tabs (spaces: line 11, tab: line 21)
Harmless, trivial to fix, but not a blocker.
qgtkstyle.i386: W: no-documentation
There's no documentation to package, not even a COPYING file.
! not entirely named and versioned according to the Package Naming Guidelines:
  The naming guidelines state that the date of the checkout should be 
specified. So the alphatag should be 20080523svn or 20080523svn610, not just 
svn610.
+ spec file name matches base package name
+ Packaging Guidelines:
  + License GPLv2 OK, matches actual license
  + No known patent problems
  + No emulator, no firmware, no binary-only or prebuilt components
  + Complies with the FHS
  + proper changelog, tags, BuildRoot, Requires, BuildRequires, Summary, 
Description
  ! but please use qt4-devel and qt4-x11 for the BRs/Reqs so this also builds 
on F8 (the F9+ packages Provide the qt4-* names)
  + no non-UTF-8 characters
  + no documentation from upstream to include
  + RPM_OPT_FLAGS are used
  + debuginfo package is valid
  + no static libraries nor .la files
  + no duplicated system libraries
  + no rpaths (at least on i386)
  + no configuration files, so %config guideline doesn't apply
  + no init scripts, so init script guideline doesn't apply
  + no GUI executables, so no .desktop file needed
  + ... and thus no desktop-file-install needed either
  + no timestamp-clobbering file commands
  + _smp_mflags used
  + scriptlets are valid
  + not a web application, so web application guideline doesn't apply
  + no conflicts
+ complies with all the legal guidelines
+ no license in the checkout to include as %doc
* skipping "source matches upstream" test because this is a SVN checkout
! bad: .svn directory included in the tarball
+ builds on at least one arch (F10 i386 mock, tested by submitter)
+ no known non-working arches, so no ExcludeArch needed
+ no missing BuildRequires
+ no translations, so translation/locale guidelines don't apply
+ ldconfig correctly called in %post and %postun
+ package not relocatable
+ ownership correct (no package-specific directories which would have to be 
owned, doesn't own directories owned by another package)
+ no duplicate files in %files
+ permissions correct, defattr used correctly
+ %clean section present and correct
+ macros used where possible
+ no non-code content
+ no large documentation files, so no -doc package needed
+ no %doc files, so no %doc files required at runtime
+ no header files which would need to be in a -devel subpackage
+ no static libraries, so no -static package needed
+ no .pc files, so no Requires: pkgconfig needed
+ no devel symlinks which would need to be in a -devel subpackage
+ %{_qt4_plugindir}/styles/libgtkstyle.so plugin (NOT a symlinks) is correctly 
NOT in -devel
+ no -devel package, so "-devel should require %{name} = %{version}-%{release}" 
is irrelevant
+ no .la files
+ no GUI executables, so no .desktop file needed
+ buildroot is deleted at the beginning of %install
+ all filenames are valid UTF-8

SHOULD Items:
! upstream includes no COPYING file, they should, maybe try talking to them?
+ no translations for description and summary provided by upstream
+ package builds in mock (F10 i386, tested by submitter)
* not tested if the package builds on all architectures
* not tested functionality because I don't have Qt 4.4 yet
+ scriptlets are sane
+ no subpackages other than -devel, so "Usually, subpackages other than devel 
should require the base package using a fully versioned dependency." is 
irrelevant
+ no .pc files, so "placement of .pc files" is irrelevant
+ no file dependencies


MUST FIX:
* You have to use 20080523svn or 20080523svn610, not just svn610, for the 
alphatag.

SHOULD FIX:
* Please use qt4-devel and qt4-x11 for the BRs/Reqs (instead of qt-devel and 
qt-x11) so this also builds on F8 (once Qt 4.4 gets pushed there). The F9+ 
packages Provide the qt4-* names, so no conditionals are needed.
* Excluding the .svn directory from your tarball would be nice, though that's a 
bit of a nitpick.
* It would be nice if you could talk to upstream about 1. including a COPYING 
file and 2. using the standard Trolltech "GPLv2 or GPLv3" boilerplate instead 
of the old GPLv2 only they used (also given that their Google Code page claims 
GPLv3 as the license), but this isn't a requirement for inclusion.

Fix the one MUST FIX issue (and please also at least the first SHOULD FIX issue 
while you are at it) and I'll approve this.

Comment 6 Shawn Starr 2008-05-27 19:23:18 UTC
Fixed:

+ You have to use 20080523svn or 20080523svn610, not just svn610, for the 
alphatag.
+ Please use qt4-devel and qt4-x11 for the BRs/Reqs (instead of qt-devel and 
qt-x11) so this also builds on F8 (once Qt 4.4 gets pushed there). The F9+ 
packages Provide the qt4-* names, so no conditionals are needed.

* Not fixed
- .svn directory in tarball, would be nice to have this removed automagically.
- It would be nice if you could talk to upstream about 1. including a COPYING 
file and 2. using the standard Trolltech "GPLv2 or GPLv3" boilerplate instead 
of the old GPLv2 only they used (also given that their Google Code page claims 
GPLv3 as the license), but this isn't a requirement for inclusion.

We can deal with upstream issues as we test this stuff out. It's alpha :)

Comment 7 Shawn Starr 2008-05-27 19:23:56 UTC
Specs/mock output in same url as above

Comment 8 Kevin Kofler 2008-05-27 19:36:59 UTC
Looks OK now. APPROVED.

Comment 9 Shawn Starr 2008-05-27 19:38:46 UTC
Fixed %changelog section coming but no other changes

Comment 10 Shawn Starr 2008-05-27 19:50:46 UTC
New Package CVS Request
=======================
Package Name: qgtkstyle
Short Description: Qt style rendering using GTK+ themes
Owners: spstarr, kkofler
Branches: F-9
InitialCC: spstarr, kkofler
Cvsextras Commits: yes

Comment 11 Shawn Starr 2008-05-27 19:51:09 UTC
New Package CVS Request
=======================
Package Name: qgtkstyle
Short Description: Qt style rendering using GTK+ themes
Owners: spstarr, kkofler
Branches: F-8, F-9
InitialCC: spstarr, kkofler
Cvsextras Commits: yes

Comment 12 Tom "spot" Callaway 2008-05-27 20:42:02 UTC
cvs done.


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