Bug 212256 (echo)

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.

Also, the package source should be a url.

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

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?

Please respect
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda

Also you want to have 

 Requires: redhat-artwork 

as Echo (currently) falls back to Bluecurve.

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!

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

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.

Also, it should not be too hard to find a safe %defattr mask

and own the root dir (%dir)

(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?

You probably want something like %defattr(0644,root,root,0755) 

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.

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.

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.

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

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).

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).

It's now in Core dist CVS and will appear in Rawhide when it's added to dist-fc7.

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.

package added to collection with davidz as the owner.

Usually the bug stays open until the package has actually been built into a
collection.

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!

(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.

(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