Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 206761 - Review Request: kadu-theme - themes for Kadu
Summary: Review Request: kadu-theme - themes for Kadu
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dominik 'Rathann' Mierzejewski
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-09-16 10:32 UTC by Michał Bentkowski
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-09-17 12:33:36 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Michał Bentkowski 2006-09-16 10:32:42 UTC
Spec URL: http://ecik.zspswidwin.pl/kadu/kadu-theme.spec
SRPM URL: http://ecik.zspswidwin.pl/kadu/kadu-theme-0.5.0-1.src.rpm
Description: Set of themes for Kadu. Previously, these themes was included into
main Kadu spec, but I decided to split them out.

Comment 1 Dominik 'Rathann' Mierzejewski 2006-09-16 18:28:15 UTC
I'll take this review.
Where does the version number come from?

Comment 2 Dominik 'Rathann' Mierzejewski 2006-09-16 19:05:40 UTC
1. package meets naming and packaging guidelines, but the origin of the
version number isn't clear, please explain.
2. specfile is properly named, is cleanly written but doesn't use macros
consistently:
%define _themesdir      /usr/share/kadu/themes

You should use %{_datadir} here. And since you never seem to use %{_themesdir}
without /icons, why not
%define _kaduiconsdir %{_datadir}/kadu/themes/icons ?
3. dist tag is present.
4. build root is correct.
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
5. can't verify if license field matches the actual license.
* Crystal theme is - according to README - based on Crystal SVG icons from
everaldo.com, but I can't find any license info there except:
http://www.everaldo.com/legal.html , which is definitely NOT open-source
compatible. It's used in KDE though, so I imagine this is open-source licensed,
but still requires an explanation (maybe README.Fedora?)
* Glass theme has NO license information inside the tarballs
* Nuvola theme is LGPL according to Copyright, but full license text is NOT
included (and at least that Copyright file should be in %files)
6. source files match upstream:
023085edabaf6a1b844fe6b5fc9315f9  kadu-theme-crystal-16.tar.bz2
57852ff3d3fd0063a642fcc173f7fa29  kadu-theme-crystal-22.tar.bz2
c3beb753222b96dad46f3adf230eb3e1  kadu-theme-glass_16.tar.gz
9ee70ca873fd0f22b2b83be133964d89  kadu-theme-glass_22.tar.gz
586cc6ff9ba62f0fdd7c7c1adf229efb  kadu-theme-nuvola-16.tar.gz
7a17b4881141b346c6268ef25c284613  kadu-theme-nuvola-22.tar.gz
7. latest version is being packaged.
8. BuildRequires are proper.
9. didn't check if package builds in mock.
10. rpmlint is more or less silent:
W: kadu-theme-crystal16 no-documentation
W: kadu-theme-crystal22 no-documentation
W: kadu-theme-glass16 no-documentation
W: kadu-theme-glass22 no-documentation
W: kadu-theme-nuvola16 no-documentation
W: kadu-theme-nuvola22 no-documentation
11. final provides and requires are sane:
kadu-theme-crystal16 = 0.5.0-1
=
kadu
kadu-theme-crystal22 = 0.5.0-1
=
kadu
kadu-theme-glass16 = 0.5.0-1
=
kadu
kadu-theme-glass22 = 0.5.0-1
=
kadu
kadu-theme-nuvola16 = 0.5.0-1
=
kadu
kadu-theme-nuvola22 = 0.5.0-1
=
kadu
12. no shared libraries are present.
13. package is not relocatable.
14. owns the directories it creates.
15. doesn't own any directories it shouldn't.
16. no duplicates in %files.
17. file permissions are appropriate.
18. %clean is present.
19. %check is absent and no test suite.
20. no scriptlets present.
21. permitted content.
22. documentation is small, so no -docs subpackage is necessary.
23. %docs are not necessary for the proper functioning of the package.
24. no headers.
25. no pkgconfig files.
26. no libtool .la droppings.
27. not a GUI app in itself, so doesn't require a .desktop entry
28. not a web app.

1. and 2. are easily fixable, so I'm mostly concerned with 5. Please contact
upstream about this. If this is split from an already accepted package, I wonder
why this wasn't caught before...


Comment 3 Michał Bentkowski 2006-09-16 20:48:44 UTC
(In reply to comment #2)
> 1. package meets naming and packaging guidelines, but the origin of the
> version number isn't clear, please explain.

Previously, themes was included into main package and they have to get bigger
version release (for example, in repo is available 
kadu-theme-crystal22-0.5.0-20060808svn)

> 2. specfile is properly named, is cleanly written but doesn't use macros
> consistently:
> %define _themesdir      /usr/share/kadu/themes
> You should use %{_datadir} here. And since you never seem to use %{_themesdir}
> without /icons, why not
> %define _kaduiconsdir %{_datadir}/kadu/themes/icons ?

%{_datadir} issue is fixed. I don't use kaduiconsdir macro, because I assume
that there will be another themes than only icons, in future.

> 5. can't verify if license field matches the actual license.
> * Crystal theme is - according to README - based on Crystal SVG icons from
> everaldo.com, but I can't find any license info there except:
> http://www.everaldo.com/legal.html , which is definitely NOT open-source
> compatible. It's used in KDE though, so I imagine this is open-source 
licensed,

I have noticed legal section on www.evaraldo.com is not open-source compatible,
but the actual icons license I found here:
http://commons.wikimedia.org/wiki/Image:Crystal_Clear_action_1downarrow.png

> * Glass theme has NO license information inside the tarballs

Glass license is written down here: 
http://www.kadu.net/forum/viewtopic.php?t=6815&highlight=glass

> * Nuvola theme is LGPL according to Copyright, but full license text is NOT
> included (and at least that Copyright file should be in %files)

I have included all licenses in %doc.



Comment 5 Dominik 'Rathann' Mierzejewski 2006-09-16 21:59:47 UTC
This is a more authoritative information source for Crystal's LGPL-ness, I'd
say: http://www.kde-look.org/content/show.php?content=8341

Please contact upstream about missing license files!

APPROVED.


Comment 6 Michał Bentkowski 2006-09-17 12:33:36 UTC
Branch has been created and package built succesfully.
Closing.


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