Bug 212256 - (echo) Review Request: echo-icon-thme - Echo icon theme
Review Request: echo-icon-thme - Echo icon theme
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ray Strode [halfline]
Fedora Package Reviews List
:
Depends On:
Blocks: FC-ACCEPT
  Show dependency treegraph
 
Reported: 2006-10-25 16:44 EDT by Luya Tshimbalanga
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-26 19:39:03 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Luya Tshimbalanga 2006-10-25 16:44:26 EDT
Spec URL: http://www.thefinalzone.com/RPMS/echo.spec
SRPM URL: http://www.thefinalzone.com/RPMS/echo-icon-theme-0.1-2.src.rpm
Description: Contains the Echo icon theme.
Comment 1 David Zeuthen 2006-10-25 17:07:34 EDT
You probably want 

 Requires(post): gtk2 >= 2.9.0

and

 License:        Creative Commons Attribution-ShareAlike 2.5

to be more specific about the name and version of the license. 

Otherwise it looks good to me.
Comment 2 Brian Pepple 2006-10-25 17:36:03 EDT
Also, the package source should be a url.
Comment 3 Michael Rice 2006-10-25 17:46:15 EDT
Your source doesnt match upstream

for Source0: I would use the link to the source, then wget -N the source or use
spectool -g on the spec file and get it.

You should rename the spec to echo-icon-theme.spec as rpmlint complains about
the current name

coreutils isnt needed to require:
http://fedoraproject.org/wiki/Extras/FullExceptionList

personally I think you should use cp and mkdir instead of the macros for them

You seem to be putting the license in 2 times

Once when you do %{__cp} LICENSE $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}
and then again in the %docs section

Maybe a couple other things when I get home and can look it over better. Also I
filed a bug about the creative commons not being in rpmlint:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=211417 just incase you want
to watch for when it is fixed/updated
Comment 4 Michael Rice 2006-10-25 20:06:46 EDT
OK, also I would like to add your BuildRequires are not needed at all, all you
do is unpack a tar ball and copy, the post is the only place anything is needed
like Comment #1 said.

rpmquery -f `which gtk-update-icon-cache `
gtk2-2.8.20-1

I think to require gtk2 would be all thats needed. I dont see a need to require
it being so new.

And in the post/postun shouldnt it be %{_datadir}/icons/Echo instead of echo?
Comment 6 David Zeuthen 2006-10-26 10:15:29 EDT
Also you want to have 

 Requires: redhat-artwork 

as Echo (currently) falls back to Bluecurve.
Comment 7 David Zeuthen 2006-10-26 13:08:41 EDT
Actually we want Echo to be the default in Rawhide which right now means it
needs to go into Core. So I'm changing product to Core and claiming ownership of
this bug. Hope to have it in Rawhide soon!
Comment 8 Luya Tshimbalanga 2006-10-26 14:39:53 EDT
Great thanks, David. 

Following the comments, I have renamed the spec file and cleaned up to follow
Packaging Guidelime.

SPEC: http://www.thefinalzone.com/RPMS/echo-icon-theme.spec
SRPM: http://www.thefinalzone.com/RPMS/echo-icon-theme-0.1-3.src.rpm


Comment 9 Matthias Clasen 2006-10-26 15:29:49 EDT
We have generally not added a gtk2 dependency for running gtk-update-icon-cache
in other packages, and the packaging guidelines also discourage it.

The description should be one or more complete sentences.
Comment 10 Nicolas Mailhot 2006-10-26 15:32:54 EDT
Also, it should not be too hard to find a safe %defattr mask
Comment 11 Nicolas Mailhot 2006-10-26 15:34:28 EDT
and own the root dir (%dir)
Comment 12 Luya Tshimbalanga 2006-10-26 15:51:35 EDT
(In reply to comment #9)
> We have generally not added a gtk2 dependency for running gtk-update-icon-cache
> in other packages, and the packaging guidelines also discourage it.
> 
Should I add coreutils Requires(post) and Requires(postun)? I looked to the link
posted on comment #5 and noticed coreutils is omitted.

> The description should be one or more complete sentences.
Fixed


(In reply to comment #10)
> Also, it should not be too hard to find a safe %defattr mask
Not sure how to proceed as I haven't touched that line for awhile. Could you
give an example?
Comment 13 Nicolas Mailhot 2006-10-26 16:29:26 EDT
You probably want something like %defattr(0644,root,root,0755) 
Comment 14 Luya Tshimbalanga 2006-10-26 16:42:29 EDT
Here is the update

SPEC: http://www.thefinalzone.com/RPMS/echo-icon-theme.spec
SRPM: http://www.thefinalzone.com/RPMS/echo-icon-theme-0.1-4.src.rpm

I removed the Requires(post) and Require(postun) for coreutils following comment #3.
Comment 15 Ray Strode [halfline] 2006-10-26 16:59:56 EDT
It looks pretty good to me.

There isn't much point in including the %build section since it's empty, though,
is there?

In core packages we don't normally add the Requires(pre) and Requires(post) for
gtk2.  instead we only run it gtk-update-icon-cache if it's available.  On the
other hand, since you Require redhat-artwork and it requires gtk2, then I think
the pre and post are right, because you're going to end up with gtk2 installed
anyway, and if that's the case then you want to make sure it gets installed
early enough in the transaction.
Comment 16 Ray Strode [halfline] 2006-10-26 17:15:12 EDT
I wouldn't leave the comment in there about coreutils.  coreutils is just
expected to be available.

Anyway, it looks pretty good.  David, feel free to build it and close the bug
report.
Comment 17 Nicolas Mailhot 2006-10-26 17:42:50 EDT
1. still missing the root dir ownership, will leave dangling dirs when removed.
Please add %dir %{_datadir}/icons/Echo (not too fond of using caps in names BTW)

2. a lot of people like to keep %build, if only to document it was intentionaly
left empty (also I think in some cases pulling it triggers rpm bugs)

3. explicitly copying LICENSE is not necessary, %doc does it for you

4. I would recommend keeping any gtk2 dep out - much better to have a clean spec
people can safely copy. Evetyone will forget why Echo was special soonish
(please add a comment before the redhat-artwork dep)

5. when copying static data it's usually better to preserve original timestamps

[shameless style preference plug, feel free to ignore)
6. you should know it's possible to replace $RPM_BUILD_ROOT with a leaner
%{buildroot}
http://fedoraproject.org/wiki/Packaging/Guidelines#head-f3d77b27a5d29dfc1f5600ef3fc836f2e317badf
Comment 18 David Zeuthen 2006-10-26 18:47:21 EDT
OK, I'm going to import this

 http://people.redhat.com/davidz/echo-icon-theme.spec
 http://people.redhat.com/davidz/echo-icon-theme-0.1-6.srpm

which takes the advice from comment 17 into advice (except for the gtk2 dep).
Comment 19 David Zeuthen 2006-10-26 18:48:08 EDT
OK, I'm going to import this

 http://people.redhat.com/davidz/echo-icon-theme.spec
 http://people.redhat.com/davidz/echo-icon-theme-0.1-6.src.rpm

which takes the advice from comment 17 into advice (except for the gtk2 dep).
Comment 20 David Zeuthen 2006-10-26 19:39:03 EDT
It's now in Core dist CVS and will appear in Rawhide when it's added to dist-fc7.
Comment 21 Ray Strode [halfline] 2006-10-26 20:27:12 EDT
Hey Nick,

a package doesn't have to use %dir to take ownership of a directory.  If the
directory is listed in the file list (without any globs after it) then it
implies %dir (and also implies adding a /* glob)

I agree with everything else you said, I think, except for 6 because both
methods are equivalent and most core packages use the $RPM_BUILD_ROOT syntax.
Comment 22 Jesse Keating 2006-10-26 22:22:30 EDT
package added to collection with davidz as the owner.

Usually the bug stays open until the package has actually been built into a
collection.
Comment 23 David Zeuthen 2006-10-26 22:33:03 EDT
Thanks. I've just built the package into dist-fc7 so it should appear in Rawhide
tomorrow. Sorry about closing the bug early.

For the Cc's that have been tracking this bug: I'll do the libgnome changes
tomorrow as to switch Rawhide over to using Echo by default. Yay!
Comment 24 Ralf Corsepius 2006-10-26 23:36:46 EDT
(In reply to comment #15)
> There isn't much point in including the %build section since it's empty, though,
> is there?
Theoretically no. Practically yes, %build is a must even if it's empty,
to work-around bugs in redhat-rpm-config and rpm.
Comment 25 Nicolas Mailhot 2006-10-27 02:01:04 EDT
(In reply to comment #21)

> a package doesn't have to use %dir to take ownership of a directory.  If the
> directory is listed in the file list (without any globs after it) then it
> implies %dir (and also implies adding a /* glob)

Lots and lots of special cases :) I just wish the auto-dir-own checking in
upstream rpm was in Core

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