[packagers] Re: [svn] r3619 - trunk/rpms/critter

Dag Wieers dag at wieers.com
Wed Oct 5 17:45:54 CEST 2005


On Wed, 5 Oct 2005, C.Lee Taylor wrote:

> Greetings ...
> 
> 	Now this is what I call juicy ... Thanks for the review and the input
> ...
>
> >> -%{!?_dist: %{expand: %%define dist rhfc4}}
> >We do not provide a default 'dist' setting. This is part of a proper
> >build system. It would require us to do a massive update of all SPEC
> > files when FC5 is released...
> 
> >(Besides rhfc4 is not correct as a value :))
> 	Busted ... Cut an paste what I think ... Would be nice, if we could
> have a few templates like Extra with what we think is good and bad ...

In theory that sounds like a good idea. And we do have a few templates 
available in the rpms-directory. But in practice a complex and complete 
SPEC file is more disturbing, and we don't want people to define macros 
when they have no place in there. Sure they don't harm either, but...

I'd prefer to have all our SPEC files in good shape, and just link to a 
few distinctive examples on how this or that is being done. A good policy 
document with rationale is more important in that respect imo.


> >>+%define desktop_vendor rpmforge
> >
> >This was missing. If you don't define a macro and you use it, the macro
> >will appear in the output. In this case the RPM package contained a
> > file called:
>
> 	Okay, this I disagree with, this macro should be in the build system,
> I don't think this should be defined in the SPEC ... Maybe something for FC5 (
> Again, just around the corner )

Well, in essence I agree with you. But in practice, we do want people to 
be able to build packages without requiring our macros per se.

With the advent of an authoritative macros-file for RPMforge, this could 
change and the use of the macros-file could become mandatory. Currently it 
isn't.


> > /usr/share/applications/%{desktop_vendor}-gcipher.desktop
>
> 	I got some of these ...

Let me know, and I'll fix them. They do work like this (which is the main 
reason why they often go unnoticed), but it's not nice.


> >Added the libpng >= 1.2 build requirement so that it properly fails
> >for RH7 and EL2.
>
> 	Great, how did you figure that out?

It didn't build on RH7, configure complained exactly about this.


> > The description should be factual and useful for the user.
>
> 	This I always battle with ... Too much or too little ... I glade that
> somebody else did something with this ...

Well, it should describe the tool and what it can do, not the development 
process, nor the technicalities for building or any other acronyms a 
normal human cannot understand. If it shouldn't matter for the user, it 
has no place in the description.


> > However, a lot of current SPEC files have bad descriptions. It's less
> > important for a good package, but if you are making a description
> > initially, it would be good to make sure it sticks to the point and
> > doesn't suffer from bitrot in the future.
>
> 	Okay, but I think that a little more description is better than shoot
> game or something ... But I understand ...

Sure, I don't object to a better description. But since I haven't played 
it, I can't describe it either. The screenshots couldn't inspire me either 
:)

 
> > Desktop files should go into the %pre section.
>
> 	Okay, I see that some have them there and the spec file I copied did,
> but I was thinking that because it was build, that where it might go ...
> 
> 	Now we should put this into a guide line ... Extend the rpm manual or
> something ...

A list of these things, per section would be nice. If you want to take 
this, go ahead. Dries can give you subversion access so you can fill up a 
webpage.


> > We can do it in 1 line. Do not use a macro where it really makes no
> > difference. It makes the SPEC file more difficult to read.
>
> 	Okay, that is nice I could not work out the install stuff and could
> not find a good example to copy ... I tried a few times, but missing the icon
> and so on, but that might have been something else ...
> 
> 	As for macro, I think it works better with the macro, we use them all
> over the place else ... But it's your call ...

We use them where it makes sense. Where statements are copied from other 
SPEC files. Even in the Source:-tag it doesn't make sense to use it. (That 
would be another cosmetic fix I didn't see)


> > I removed the _without_freedesktop macro, since it doesn't build on
> > RH7 anyway.
>
> 	Okay ... I think I still ask Dries about this ...

It doesn't matter. I first added it too (made the buildreq conditional and 
changed the entries a bit), but when I noticed RH7 didn't want to build 
it, I removed it. Looks much cleaner without, doesn't it ? :)


> > We try to have a list of files in alphabetical order. This makes it
> > more easier to verify if something is missing.
>
>  I will try keep that in mind ... My frist SPEC did not even have anything
> expect the COPYING ...

The more useful documentation files the better. I actually verified and 
couldn't find anything to add.


> > On the other hand, the binary is called critter. Time for upstream to
> > make a decision and end the confusion imo.
>
> 	Well, I copied this from the spec file in the source and some other
> spec file and merged them together with what I thought to be best ... I don't
> like things like "Critical_Mass" and then criticalmass and CriticalMass ...
> It's not consistent, but I did not want to patch the source too much, but
> might be better ...

Package-names should be lowercase, there are a few exceptions. Mostly to 
keep consistency with Red Hat and/or perl-modules. But generally, use 
lowercase !!


> > I just noticed the icon is in /usr/share/icons/, while all other
> > packages use /usr/share/pixmaps/. I'd go for pixmaps since that's
> > what we've used everywhere.
>
> 	Again, I'm not really sure where to put this stuff, it seem to work,
> so I was happy ... But I think the move to pixmaps is better ... Is there a
> something in LSB that states which we should use?

I don't know. But I looked in /usr/share/icons/ and it only contained 
theme-related stuff. So that's definitely not the right place :)


> 	Thanks for the review and details explanation of the changes, I think
> this really help improve my general packages.

I hope it helped other people too. I promised to add and review some other 
packages, but moving, a new girl-friend and some other commitments are 
pushing this work off my priority-list.

Maybe we should allow a few people to commit directly and that will urge 
me and others to comment and sign-off. I've been very protective to what 
goes into subversion, but maybe the sign-off idea changes the attitude 
towards subversion a bit.

Something has to change though.

Kind regards,
--   dag wieers,  dag at wieers.com,  http://dag.wieers.com/   --
[all I want is a warm bed and a kind word and unlimited power]



More information about the packagers mailing list