[Bug 461131] Review Request: sim - Simple Instant Messenger

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=461131





--- Comment #34 from Patrice Dumas <pertusus at free.fr>  2008-10-03 17:37:12 EDT ---
(In reply to comment #33)
> Excuse me for the deadline. On this day I'm several times try build sim...
> But I'm do not able tag package on branches except of devel:
> [pasha at x-www F-9]$ cvs up -dPA
> cvs update: Updating .
> 
> [pasha at x-www F-9]$ LANG=C make tag
> cvs tag  -c sim-0_9_5-0_6_20080923svn2261rev
> ERROR: The tag sim-0_9_5-0_6_20080923svn2261rev is already applied on a
> different branch
> ERROR: You can not forcibly move tags between branches
> sim-0_9_5-0_6_20080923svn2261rev:devel:hubbitus:1222767214
> cvs tag: Pre-tag check failed
> cvs [tag aborted]: correct the above errors first!
> make: *** [tag] Error 1

%{?dist} is missing !!!!

> > Instead of %define with_kde, please use bcond_with or bcond_without.
> You think? I'm do not suppose this for buildtime config. But it seams as good
> suggestion. I do that.

It is much more convenient than plain define.

> I'm do not remember why I'm write it... May be you a right. But on fedora > 8
> it should be kdebase3-devel >= 3.0.0. I add this BR.

Right.

> And now I think over necessity of adding Requires: kdebase...

I think not, it should be picked automatically using the soname.

> > The checkout instructions are not enough, you should add the command 
> > that allows you to do the archive.
> It seems excessive, but I'm add this.

You can do make dist, or tar directly, it should be possible to 
redo exactly like you. Also it may be possible to recreate configure
offline.

> > You install icons, so it is likely that a post script is missing.
> What script you keep in mind?

http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GTK.2B_icon_cache

> > Remove the Distribution tag.
> Removed. Does it make troubles?

It could. And besides this spec is not only for fedora, it is better 
if it is reusable, be it only in EPEL. And it is at best unuseful.

> > Why the BuildRequires autoconf, automake?
> Because it is SVN build. We 

Maybe, but it isn't used during the build? If it is used, I think
it is much better to call autoconf/automake/autoreconf explicitly.
Looking at the log, it is used during make -f admin/Makefile.common
Would certinly be better to call it explicitly, especially since
it does strange things, maybe when different versions are installed:

+ make -f admin/Makefile.common
/usr/bin/autoconf: line 519: echo: write error: Broken pipe
*** automake (GNU automake) 1.7.9 found.


> > I also thought that zip was there too, but I am too lazy to check.
> No, zip is not. See report about it before in this review.

Oh, unzip is there but not zip. Do you really need zip during 
build?

> > openssl Requires should be picked up automatically.
> Where it is written?

It is not written, but rpm uses sonames for library dependencies, 
which means that you should never have to put library packages
requires explicitly.

> > Some suggestions, feel free not to use these:
> > 
> > * the TABs look bad in my editor, maybe you could either use only spaces
> >   or use tabs more consistently
> In my editor (mcedit) my tab look correct. I'm assume convert tabs into spaces
> for your editor you may by one command, like this: sed -i 's/\t/   /g' sim.spec

I know, but this spec is your spec :-). Not a big deal. (For the
record my editor is vim).

> > * The BuildRequires line that is very long could be split in 2 lines
> But it is not required, right?

Nope, that's why I put it in the suggestions.

> > * remove from the description the text:
> > 
> >  SIM has countless features, many of them are listed at:
> >  http://sim-im.berlios.de/
> > 
> > since the URL is already a rpm tag.
> No. This is "historical" text :)
> I do not want remove them.

I don't really get the historical argument, but this is a suggestion,
so no problem if you prefer to have this 'historical text'.


Another suggestion, 
#Rm symlink, which seems as development.
should certainly be rephrased as something like
# rm symlink since we don't support developping with sim

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.