[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 #33 from Pavel Alexeev <pahan at hubbitus.spb.su>  2008-10-03 16:30:56 EDT ---
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

I'm do it by docs -
http://fedoraproject.org/wiki/PackageMaintainers/Join#Tag_Or_Update_Your_Branches
and http://fedoraproject.org/wiki/PackageMaintainers/NewPackageProcess
Please help me - what I do wrong?

(In reply to comment #32)
> I still have some comments.
> 
> 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.

> The  kdebase >= 3.0.0 BuildRequires is strange. Shouldn't it be
> kdebase-devel >= 3.0.0? And I don't really see the reason why it 
> shouldn't also be set on fedora > 8?
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.

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

> You should not repeat the summary in the %description.
Ok, thanks.

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

> The line with
> CFLAGS="$RPM_OPT_FLAGS" CXXFLAGS="$RPM_OPT_FLAGS"
> seems unuseful to me
> 
> And you use $LOCALFLAGS but it is not set??
Removed. But it do have sense.

> Remove the # Setup for parallel builds, use instead 
> make %{?_smp_mflags}
Done.

> You should not do 
> rm -rf $RPM_BUILD_DIR/%{name}-%{version}
> in %clean.
Ok.


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

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

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

> Also remove gcc and gcc-c++ from BuildRequires, please, they are in the
> minimal buildroot.
Ok.

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

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

> 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

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

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


http://hubbitus.net.ru/rpm/Fedora9/sim/sim-0.9.5-0.7.20080923svn2261rev.src.rpm

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