Bug 461131 - Review Request: sim - Simple Instant Messenger
Review Request: sim - Simple Instant Messenger
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Marcela Mašláňová
Fedora Extras Quality Assurance
:
: sim-review (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-04 09:42 EDT by Pavel Alexeev
Modified: 2008-10-21 03:55 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-04 13:29:18 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mmaslano: fedora‑review+
huzaifas: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Pavel Alexeev 2008-09-04 09:42:28 EDT
Spec URL: http://hubbitus.net.ru/rpm/Fedora9/sim/sim.spec
SRPM URL: http://hubbitus.net.ru/rpm/Fedora9/sim/sim-0.9.5-1.SVN20080904rev2258.src.rpm
Description:
SIM - Multiprotocol Instant Messenger

SIM (Simple Instant Messenger) is a plugins-based open-
source instant messenger that supports various protocols
(ICQ, Jabber, AIM, MSN, LiveJournal, Yahoo!). It uses the.
QT library and works on X11 (with optional KDE support).

SIM has countless features, many of them are listed at:
http://sim-im.berlios.de/
Comment 1 Pavel Alexeev 2008-09-04 09:43:54 EDT
*** Bug 411881 has been marked as a duplicate of this bug. ***
Comment 2 Pavel Alexeev 2008-09-04 10:01:28 EDT
rpmlint for spec and source package is silent. But on binary is not:
$ rpmlint sim-0.9.5-1.SVN20080904rev2258.athlon.rpm 
sim.athlon: W: devel-file-in-non-devel-package /usr/lib/libsim.so
sim.athlon: E: zero-length /usr/share/doc/sim-0.9.5/AUTHORS
1 packages and 0 specfiles checked; 1 errors, 1 warnings.

So, I have 3 questions:
1) Why symlink to library is considered as "devel-file"??
2) So, file AUTHORS in upstream SVN has 0 size. Should I exclude them from package?? I think I shouldn't - what if it filling in future.
3) License tag I set to GPLv2 by included in source text of license, but I doubt: On sites (http://sim-im.org/wiki/Main_Page , http://developer.berlios.de/projects/sim-im/ ) it is marked just us "GPL".
Comment 3 Patrice Dumas 2008-09-04 10:13:41 EDT
(In reply to comment #2)

> So, I have 3 questions:
> 1) Why symlink to library is considered as "devel-file"??

It is used to link against the library with -lsim at build time. 
It is not used at runtime. So it should be in devel such that
one have to install the devel package to link against the library.
The file used at runtime should be like 

/usr/lib/libsim.so.?????

and the library should have a 'soname' in the binary ELF headers that
causes the dynamic loader, ld to load the library. 

> 2) So, file AUTHORS in upstream SVN has 0 size. Should I exclude them from
> package?? I think I shouldn't - what if it filling in future.

This one is up to you, leave it in if you prefer to.

> 3) License tag I set to GPLv2 by included in source text of license, but I
> doubt: On sites (http://sim-im.org/wiki/Main_Page ,
> http://developer.berlios.de/projects/sim-im/ ) it is marked just us "GPL".

The source of information, here, are the file headers, and the
README or the like that should state the author's intention.
If there is nothing else that 'this is under the GPL', this means
GPL+
that is any version of the GPL, and not GPLv2.
Comment 4 Marcela Mašláňová 2008-09-04 10:15:05 EDT
1/ rpmlint is quite talkative :) .so is devel file for him.
2/ it's only warning, you can ignore it. Normally install the file in %doc
3/ You need to check license in code and COPYING in source tarball. Maybe you should contact upstream about it.
Comment 5 Pavel Alexeev 2008-09-04 10:48:41 EDT
PD == Patrice Dumas
MM = Marcela Maslanova

(In reply to comment #3, #4)
PD> It is used to link against the library with -lsim at build time. 
PD> It is not used at runtime.
Even as used! I'm just delete (rename) it:
$ sim
sim: error while loading shared libraries: libsim.so.0: cannot open shared object file: No such file or directory

MM> 1/ rpmlint is quite talkative :) .so is devel file for him.
I also think in this case.

PD> one have to install the devel package to link against the library.
PD> The file used at runtime should be like 
PD> 
PD> /usr/lib/libsim.so.?????
In is /usr/lib/libsim.so.0.0.0
with 2 symlinks on it:
/usr/lib/libsim.so.0
and
/usr/lib/libsim.so

PD> This one is up to you, leave it in if you prefer to.
MM> 2/ it's only warning, you can ignore it. Normally install the file in %doc
Ok, I leave it.

PD> The source of information, here, are the file headers, and the
PD> README or the like that should state the author's intention.
PD> If there is nothing else that 'this is under the GPL', this means
PD> GPL+
PD> that is any version of the GPL, and not GPLv2.
MM> 3/ You need to check license in code and COPYING in source tarball.
File COPYING contains text of GPL2. It is a file, about I say "included in source text of license" before.

MM>Maybe you should contact upstream about it.
Off course, in case if we can't make decision here.
Comment 6 Patrice Dumas 2008-09-04 11:29:32 EDT
(In reply to comment #5)
> PD == Patrice Dumas
> MM = Marcela Maslanova
> 
> (In reply to comment #3, #4)
> PD> It is used to link against the library with -lsim at build time. 
> PD> It is not used at runtime.
> Even as used! I'm just delete (rename) it:
> $ sim
> sim: error while loading shared libraries: libsim.so.0: cannot open shared
> object file: No such file or directory

You deleted another file, not the symlink. If you do a 
ldd /usr/bin/sim
you'll see that the .so file isn't used, instead
/usr/lib/libsim.so.0.0.0
is used.

> MM> 1/ rpmlint is quite talkative :) .so is devel file for him.
> I also think in this case.

It is indeed a devel file.

> PD> one have to install the devel package to link against the library.
> PD> The file used at runtime should be like 
> PD> 
> PD> /usr/lib/libsim.so.?????
> In is /usr/lib/libsim.so.0.0.0
> with 2 symlinks on it:
> /usr/lib/libsim.so.0
> and
> /usr/lib/libsim.so

In /usr/lib/libsim.so.0.0.0, there is a soname:
objdump -p /usr/lib/libsim.so.0.0.0 | grep SONAME
should give about
  SONAME               libsim.so.0
The libsim.so.0 is here such that you can have an idea about the 
soname without actually looking at the file binary headers.
The .so is used to link against the library file at build time.

> PD> The source of information, here, are the file headers, and the
> PD> README or the like that should state the author's intention.
> PD> If there is nothing else that 'this is under the GPL', this means
> PD> GPL+
> PD> that is any version of the GPL, and not GPLv2.
> MM> 3/ You need to check license in code and COPYING in source tarball.
> File COPYING contains text of GPL2. It is a file, about I say "included in
> source text of license" before.

If there is only a COPYING, then it is any version of the GPL. See
also question 3 on:
http://fedoraproject.org/wiki/Licensing/FAQ
Comment 7 Pavel Alexeev 2008-09-04 14:17:20 EDT
(In reply to comment #6)
 
> You deleted another file, not the symlink. If you do a 
> ldd /usr/bin/sim
> you'll see that the .so file isn't used, instead
> /usr/lib/libsim.so.0.0.0
> is used.
Indeed /usr/lib/libsim.so.0

> It is indeed a devel file.
So, in case of sim do not used by any other programs it is may just be safely excluded from package.

And why second /usr/lib/libsim.so.0 symlink is not devel??

> If there is only a COPYING, then it is any version of the GPL. See
> also question 3 on:
> http://fedoraproject.org/wiki/Licensing/FAQ
Thank you very much for the link.

According 1 answer of it, I see source. sim/sim.spp (this is file contains main) and few others says:
"This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation; either version 2 of the License, or (at your option) any later version."

So, I change License of package to GPLv2+

P.S. F9 koji build successful - http://koji.fedoraproject.org/koji/taskinfo?taskID=806367
F8 - http://koji.fedoraproject.org/koji/taskinfo?taskID=807153
Comment 9 Marcela Mašláňová 2008-09-05 03:25:19 EDT
Shouldn't you use also this part?
desktop-file-install --vendor="fedora"<>\
--add-category="Network"\
--delete-original\
--dir=$RPM_BUILD_ROOT/%{_datadir}/applications<>\
$RPM_BUILD_ROOT/%{_datadir}/applications/kde/%{name}.desktop

You can follow this guide lines:
https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop

I'm not sure if I have correct the category.
Comment 10 Pavel Alexeev 2008-09-05 11:03:17 EDT
Marcela Maslanova, thank you. Before I include desktop file as regular file. I think this is more correct way.

http://hubbitus.net.ru/rpm/Fedora9/sim/sim-0.9.5-3.SVN20080904rev2258.src.rpm
Comment 11 Patrice Dumas 2008-09-05 11:40:08 EDT
The %release seems, at first glance to be incorrect, please see:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages
Comment 12 Patrice Dumas 2008-09-05 11:40:59 EDT
Sorry, it should better be
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#PreReleasePackages
Comment 13 Pavel Alexeev 2008-09-05 12:26:40 EDT
I'm understand what Release should start from 0. and followed by "spec releases", but not completely understand about svn/cvs information in release:

It is meant what version should be:
sim-0.9.5-0.3.20080904svn ?
Or I can add revision number like this:
sim-0.9.5-0.3.20080904svn2258rev ?
Comment 14 Patrice Dumas 2008-09-05 12:35:57 EDT
both are correct. There is a limit in the number of character, if
I remember well, however, to be verified.
Comment 15 Pavel Alexeev 2008-09-05 15:44:22 EDT
How I can verify its limit?
And where it is? Rpm built ok, koji built too. May be bodhi has this limitation? I did not hear about similar problem.
Comment 17 Marcela Mašláňová 2008-09-23 05:35:50 EDT
I rebuilt your srpm with one error. I assume zip should be added into build requires.

http://koji.fedoraproject.org/koji/getfile?taskID=838397&name=build.log
Comment 18 Pavel Alexeev 2008-09-23 13:08:44 EDT
Yes, you are right, thank you.

http://koji.fedoraproject.org/koji/taskinfo?taskID=838884
Comment 19 Marcela Mašláňová 2008-09-24 04:54:14 EDT
Please attach the final srpm for another check. 

If noone other will be interested in review, I can do it. I'm really looking forward to have sim in repo instead of building it myself ;-)
Comment 20 Pavel Alexeev 2008-09-24 05:16:26 EDT
An(In reply to comment #19)
> Please attach the final srpm for another check. 

I have been mistaked what original src.rpm also available from koji. Sory.

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

Separate spec url is old ( http://hubbitus.net.ru/rpm/Fedora9/sim/sim.spec )

> If noone other will be interested in review, I can do it. I'm really looking
> forward to have sim in repo instead of building it myself ;-)
As I can see, none of fedora‑review flag set on this request, so, I think you can review it if you want.
Comment 21 Marcela Mašláňová 2008-09-24 07:43:31 EDT
rpmlint find some problems:
sim.i386: E: zero-length /usr/share/doc/sim-0.9.5/AUTHORS
You can remove AUTHORS from rpm, before packaging.

sim.i386: W: incoherent-version-in-changelog 0.9.5-0.4.20080923svn2261rev 0.9.5-0.3.20080923svn2261rev
You've forgotten change release macron in the head of specfile.

sim.src:131: W: macro-in-%changelog _datadir
sim.src:134: W: macro-in-%changelog _libdir
The macro in changelog shouldn't be used.
Comment 22 Marcela Mašláňová 2008-09-24 08:07:22 EDT
- package meets naming guidelines OK
- package meets packaging guidelines OK
- license (GPLv2+ ) OK, text in %doc, matches source OK
- spec file legible, in am. english OK probably
- source matches upstream OK
- package compiles on devel (x86) OK
- no missing BR OK
- no unnecessary BR OK
- no locales OK
- not relocatable OK
- owns all directories that it creates OK
- no duplicate files OK
- permissions ok OK
- %clean ok OK
- macro use consistent OK
- no need for -docs OK
- nothing in %doc affects runtime OK

Source should be changed on something which could be downloaded from home page of project.
Comment 23 Pavel Alexeev 2008-09-24 13:36:22 EDT
(In reply to comment #21)
> rpmlint find some problems:
> sim.i386: E: zero-length /usr/share/doc/sim-0.9.5/AUTHORS
> You can remove AUTHORS from rpm, before packaging.
Please see discussion about it from comment #2 of this review. We make decision leave it in package because it is upstream file and it may be filled in future.

Do you have another opinion?

> sim.i386: W: incoherent-version-in-changelog 0.9.5-0.4.20080923svn2261rev
> 0.9.5-0.3.20080923svn2261rev
> You've forgotten change release macron in the head of specfile.
Excuse me, what??? I'm not see this:
$ rpmlint sim-0.9.5-0.3.20080923svn2261rev.src.rpm
sim.src:131: W: macro-in-%changelog _datadir
sim.src:134: W: macro-in-%changelog _libdir
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

$ rpm -q rpmlint
rpmlint-0.84-2.fc10.noarch

About prerelease naming package my question leaved without answer, so, you are think it is not correct version-release "0.9.5-0.4.20080923svn2261rev"?

In any case, I think it fixed now. Please check.

> sim.src:131: W: macro-in-%changelog _datadir
> sim.src:134: W: macro-in-%changelog _libdir
> The macro in changelog shouldn't be used.
Ok, this is my inadvertence. Fixed.

(In reply to comment #22)
> Source should be changed on something which could be downloaded from home page
> of project.
What should be here in case of SVN build (svn checkout, no source download from upstrim)??


http://hubbitus.net.ru/rpm/Fedora9/sim/sim-0.9.5-0.5.20080923svn2261rev.src.rpm
Comment 24 Marcela Mašláňová 2008-09-25 04:21:18 EDT
Ok, the AUTHORS are not important. Rpmlint is touchy.

>> sim.i386: W: incoherent-version-in-changelog 0.9.5-0.4.20080923svn2261rev
>> 0.9.5-0.3.20080923svn2261rev
>> You've forgotten change release macron in the head of specfile.
>Excuse me, what??? I'm not see this:
Hm, I probably mixed up old spec with your package. Never mind.

The length of name:
https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages
You need remove rev. It's date + 16 characters maximally.

Source:
In this case source must be commented, how you get it. Something like "svn export ... -r %{rev} ...; tar ..."
Comment 25 Marcela Mašláňová 2008-09-25 04:36:32 EDT
And also I see sim-debuginfo.i386: E: empty-debuginfo-package.

Maybe you're stripping the debug.
https://fedoraproject.org/wiki/Packaging/Debuginfo
Comment 26 manuel wolfshant 2008-09-25 08:28:40 EDT
I see absolutely no use for including an empty Authors file and eating an inode for it. If/when the authors decide to add real content to that file, you'll need to modify the spec anyway. Does it take more that 15 seconds to add the file in the %doc line ?

  my .2c
Comment 27 Marcela Mašláňová 2008-09-26 03:38:49 EDT
Well, I wouldn't pack empty files and probably remove them in cleaning phase, but that's depend on maintainer. He can have approved package after fix of release.
Comment 28 Pavel Alexeev 2008-09-28 11:00:08 EDT
(In reply to comment #24)

> The length of name:
> https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages
> You need remove rev. It's date + 16 characters maximally.
Why remove? "svn2261rev" has only 10 characters from 16 allowed??

> Source:
> In this case source must be commented, how you get it. Something like "svn
> export ... -r %{rev} ...; tar ..."
Ok, I'm add this.

(In reply to comment #25)
> And also I see sim-debuginfo.i386: E: empty-debuginfo-package.
> 
> Maybe you're stripping the debug.
> https://fedoraproject.org/wiki/Packaging/Debuginfo
Yes it is. 
"make install-strip" replaced by "make install"

(In reply to comment #26)
> I see absolutely no use for including an empty Authors file and eating an inode
> for it. If/when the authors decide to add real content to that file, you'll
> need to modify the spec anyway. Does it take more that 15 seconds to add the
> file in the %doc line ?
> 
>   my .2c
Hmmm... IMHO, one inode have no sense in performance or anywhere. Indeed 15 seconds already was be spend to add it, and delete also take more 15 secs...
But main my argument is - this is file from upstream source package and it is file, what is recommended to put into %doc.

(In reply to comment #27)
> Well, I wouldn't pack empty files and probably remove them in cleaning phase,
> but that's depend on maintainer. He can have approved package after fix of
> release.
Thanks for the choice. I'm going to keep the file here.

http://koji.fedoraproject.org/koji/taskinfo?taskID=847700

http://hubbitus.net.ru/rpm/Fedora9/sim/sim-0.9.5-0.6.20080923svn2261rev.src.rpm
Comment 29 Marcela Mašláňová 2008-09-29 05:10:56 EDT
Ok, the release number is according to guide lines. Let's build it!
Comment 30 Pavel Alexeev 2008-09-29 09:45:41 EDT
New Package CVS Request
=======================
Package Name: sim
Short Description: Simple Instant Messenger
Owners: hubbitus
Branches: F-8 F-9 
InitialCC:
Comment 31 Huzaifa S. Sidhpurwala 2008-09-30 00:28:19 EDT
cvs done
Comment 32 Patrice Dumas 2008-10-02 13:14:06 EDT
I still have some comments.

Instead of %define with_kde, please use bcond_with or bcond_without.

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?

You should not repeat the summary in the %description.

The checkout instructions are not enough, you should add the command 
that allows you to do the archive.

The line with
CFLAGS="$RPM_OPT_FLAGS" CXXFLAGS="$RPM_OPT_FLAGS"
seems unuseful to me

And you use $LOCALFLAGS but it is not set??

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

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

You install icons, so it is likely that a post script is missing.

Remove the Distribution tag.

Why the BuildRequires autoconf, automake?

Also remove gcc and gcc-c++ from BuildRequires, please, they are in the
minimal buildroot. I also thought that zip was there too, but I am
too lazy to check.

openssl Requires should be picked up automatically.





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

* The BuildRequires line that is very long could be split in 2 lines

* 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.
Comment 33 Pavel Alexeev 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@x-www F-9]$ cvs up -dPA
cvs update: Updating .

[pasha@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
Comment 34 Patrice Dumas 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@x-www F-9]$ cvs up -dPA
> cvs update: Updating .
> 
> [pasha@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
Comment 35 Patrice Dumas 2008-10-03 18:12:08 EDT
rpmlint says:

sim.i386: E: invalid-directory-reference /usr/lib/sim/forward.la (line 17)


In general the .la files are not useful in fedora. Is it different
here? They also trigger tons of dubious Provides like 
libtool(/usr/lib/libsim.la) libtool(/usr/lib/sim/__homedir.la)
Comment 36 Pavel Alexeev 2008-10-03 18:59:36 EDT
(In reply to comment #34)
> (In reply to comment #33)
> %{?dist} is missing !!!!
Thank you very much!!

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

> > > 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
Link is very usefull. Thanks again. I'm add icon update instructions.

> 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.
I'm do not firstly positioning this for any besides Fedora. Well, I'll have this on record.

> 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.
It is used from
make -f admin/Makefile.common
after %setup

admin/Makefile.common is upstream file, so I not see any reason copy/past content of it into spec.

> > > 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?
This check zip in ./configure, and fails if it is not present. Please, see buildlog with this report, if you want more details.


> It is not written, but rpm uses sonames for library dependencies, 
> which means that you should never have to put library packages
> requires explicitly.
openssl package also provides few binaries, AFAIK.

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


I hope build it.
Comment 37 Patrice Dumas 2008-10-03 20:21:00 EDT
(In reply to comment #36)

> > 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.
> I'm do not firstly positioning this for any besides Fedora. Well, I'll have
> this on record.

I am only saying that for the Distribution tag; but in general it is
better to avoid using fedora name without good reason, 
 http://fedoraproject.org/wiki/PackageMaintainers/Packaging_Tricks#Avoiding_using_fedora_or_redhat

> > 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.
> It is used from
> make -f admin/Makefile.common
> after %setup
>
> admin/Makefile.common is upstream file, so I not see any reason copy/past
> content of it into spec.

Ok. There is chance that there are things that are not needed
that are done, but, yes, using  what upstream provides is right.
But then there is this error:
 /usr/bin/autoconf: line 519: echo: write error: Broken pipe

> This check zip in ./configure, and fails if it is not present. Please, see
> buildlog with this report, if you want more details.

It is enough. But it deserves a comment in the spec file in 
my opinion. Looking at the build, it looks like a zip archive is done.
 
> openssl package also provides few binaries, AFAIK.

If you need openssl for the binaries, please add a comment.
Comment 38 Pavel Alexeev 2008-10-03 20:43:01 EDT
(In reply to comment #37)
> But then there is this error:
>  /usr/bin/autoconf: line 519: echo: write error: Broken pipe

I assume it is may be safely ignored. It is not break build

> > This check zip in ./configure, and fails if it is not present. Please, see
> > buildlog with this report, if you want more details.
> 
> It is enough. But it deserves a comment in the spec file in 
> my opinion. Looking at the build, it looks like a zip archive is done.
Adding BR zip is already written in changelog.

> > openssl package also provides few binaries, AFAIK.
> If you need openssl for the binaries, please add a comment.
I doubt. But I will investigate it later.


And now I have one more question. I add icons handling scripts to %post/%postun section, as wrote before, but got warning on resulting rpm:
sim.i386: E: postin-without-ldconfig /usr/lib/libsim.so.0.0.0

Why?
Now I move /sbin/ldconfig down, from -p option and it seems worked. But What is switch "-p" in %post* macroses? I can not find any relevant info about it.
Comment 39 Pavel Alexeev 2008-10-03 21:02:08 EDT
And please exceuse me, more question.
Keep in mind what Fedora 10 early branch now available ( http://article.gmane.org/gmane.linux.redhat.fedora.devel/93732 ) does it means I need request new brunch F-10 for package here??
Comment 40 Fedora Update System 2008-10-04 13:26:45 EDT
sim-0.9.5-0.10.20080923svn2261rev.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/sim-0.9.5-0.10.20080923svn2261rev.fc9
Comment 41 Fedora Update System 2008-10-04 13:26:49 EDT
sim-0.9.5-0.10.20080923svn2261rev.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/sim-0.9.5-0.10.20080923svn2261rev.fc8
Comment 42 Patrice Dumas 2008-10-04 17:25:26 EDT
(In reply to comment #38)
> (In reply to comment #37)
> > But then there is this error:
> >  /usr/bin/autoconf: line 519: echo: write error: Broken pipe
> 
> I assume it is may be safely ignored. It is not break build

I think that it is worth investigating. But it is up to you.

> Adding BR zip is already written in changelog.

I would have preferred a comment next to the BuildRequires, but
do as you please.
 
> And now I have one more question. I add icons handling scripts to %post/%postun
> section, as wrote before, but got warning on resulting rpm:
> sim.i386: E: postin-without-ldconfig /usr/lib/libsim.so.0.0.0
> 
> Why?
> Now I move /sbin/ldconfig down, from -p option and it seems worked. But What is
> switch "-p" in %post* macroses? I can not find any relevant info about it.

It is explained in
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Syntax
Comment 43 Pavel Alexeev 2008-10-05 15:21:30 EDT
Patrice Dumas thanks for the link.
Comment 44 Patrice Dumas 2008-10-06 17:28:09 EDT
Shouldn't kde supportr be build in the default case?
That would mean using

%bcond_with kde

Now the %post -p isn't used anymore, you have to add explicitely:

Requires(postun): /sbin/ldconfig
Requires(post): /sbin/ldconfig




A suggestion: in the spec file, lines are not cut at 80 columns, though it is quite convenient when editing in a console since this is the default width, and also it is in general more readable, longer lines are, in my opinion, less
easy to read. Of course this doesn't apply to long lines that are long because of an usrl or a string that shouldn't be cut. The suggestion would be to cut at 80 columns more systematically. This is only a suggestion.
Comment 45 Pavel Alexeev 2008-10-06 18:49:15 EDT
(In reply to comment #44)
> Shouldn't kde supportr be build in the default case?
> That would mean using
> 
> %bcond_with kde
Yes, off course. Why we may want disable kde support by default?

> Now the %post -p isn't used anymore
By link what you provided befor it is mentioned as alternative.

> you have to add explicitely:
> 
> Requires(postun): /sbin/ldconfig
> Requires(post): /sbin/ldconfig
Hm... Sure? Din not /sbin/ldconfig generic??


> A suggestion: in the spec file, lines are not cut at 80 columns, though it is
> quite convenient when editing in a console since this is the default width, and
> also it is in general more readable, longer lines are, in my opinion, less
> easy to read. Of course this doesn't apply to long lines that are long because
> of an usrl or a string that shouldn't be cut. The suggestion would be to cut at
> 80 columns more systematically. This is only a suggestion.

In most cases, in package descriptions and other I'm wrap text in width of 80 characters...
I will try to be careful in the future.
Comment 46 Patrice Dumas 2008-10-06 19:03:11 EDT
(In reply to comment #45)
> (In reply to comment #44)
> > Shouldn't kde supportr be build in the default case?
> > That would mean using
> > 
> > %bcond_with kde
> Yes, off course. Why we may want disable kde support by default?

Crap, I said the reverse of the correct thing. To have it defaulted to true, it should be:

%bcond_without kde

> > Now the %post -p isn't used anymore
> By link what you provided befor it is mentioned as alternative.

It is not a problem, in fact you have to do it that way.

> > you have to add explicitely:
> > 
> > Requires(postun): /sbin/ldconfig
> > Requires(post): /sbin/ldconfig
> Hm... Sure? Din not /sbin/ldconfig generic??

Think about install in chroots and more importantly installation order.

> In most cases, in package descriptions and other I'm wrap text in width of 80
> characters...
> I will try to be careful in the future.

In %description it is not a suggestion, it is a must, but you made it right. But there are other places in th espec where you don't do it (in the tags part, in %changelog...). Still it is only a suggestion in these places.
Comment 47 Fedora Update System 2008-10-07 05:47:13 EDT
sim-0.9.5-0.10.20080923svn2261rev.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 48 Fedora Update System 2008-10-07 05:49:19 EDT
sim-0.9.5-0.10.20080923svn2261rev.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 49 Marcela Mašláňová 2008-10-08 09:43:01 EDT
Another thing :) 

I found out that last update change location of settings and history from ~/.kde/share/apps into ~/.sim. That's upstream change or you did it only in Fedora?
Thanks
Comment 50 Patrice Dumas 2008-10-08 11:51:41 EDT
I guess that it is the consequence using bcond_with instead of bcond_without
which means that kde support is not compiled in. Hopefully next build
will revert to using kde.
Comment 51 Pavel Alexeev 2008-10-08 12:41:54 EDT
(In reply to comment #50)
> I guess that it is the consequence using bcond_with instead of bcond_without
> which means that kde support is not compiled in.
I thought it is true. In any case, I did not change any related setting.

> Hopefully next build will revert to using kde.
Shure. I build it now. It will be available shortly.
Comment 52 Patrice Dumas 2008-10-11 07:00:46 EDT
The (post*) Requires for /sbin/ldconfig are still missing
Comment 53 Fedora Update System 2008-10-12 08:57:42 EDT
sim-0.9.5-0.14.20080923svn2261rev.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/sim-0.9.5-0.14.20080923svn2261rev.fc9
Comment 54 Fedora Update System 2008-10-12 08:57:48 EDT
sim-0.9.5-0.14.20080923svn2261rev.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/sim-0.9.5-0.14.20080923svn2261rev.fc8
Comment 55 Pavel Alexeev 2008-10-12 08:59:57 EDT
Patrice Dumas, I add this, if you want.
Comment 56 Patrice Dumas 2008-10-12 09:19:09 EDT
It is not 'if I want', it is a blocker.
Comment 57 Pavel Alexeev 2008-10-12 16:59:52 EDT
It is not blocker - sim already in fedora updated many time. However, in any case it is also already added and updates submited (see below).
Comment 58 Patrice Dumas 2008-10-12 17:11:31 EDT
(In reply to comment #57)
> It is not blocker - sim already in fedora updated many time.

It should not have been accepted with some blocking issues that were
still present when it was approved, but went unnoticed. Having the 
package built doesn't mean that it is in shape for fedora. There
are still many packages from the former fedora core that are not in 
good shape, also.

> However, in any
> case it is also already added and updates submited (see below).

It seems to be fixed in cvs, right.
Comment 59 Fedora Update System 2008-10-15 22:01:40 EDT
sim-0.9.5-0.14.20080923svn2261rev.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 60 Fedora Update System 2008-10-15 22:06:17 EDT
sim-0.9.5-0.14.20080923svn2261rev.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 61 Vladimir Ermakov 2008-10-17 14:01:33 EDT
using SIM with GNOME
after update
no icq-icons(green flowers) on contact-list
Comment 62 Pavel Alexeev 2008-10-17 15:18:17 EDT
Vladimir Ermakov to which version update? From which version?
Please can you open separate bug for this and provide more info??
Comment 63 Vladimir Ermakov 2008-10-17 20:56:32 EDT
version - current
sim-0.9.5-0.14.20080923svn2261rev.fc9.i386
Comment 64 Pavel Alexeev 2008-10-20 17:56:17 EDT
Vladimir Ermakov, please, chek files:
ls -l /usr/share/apps/sim/icons

Try change theme in SIM-settings.

P.S. Very thank you for feedback, but please, stringently ask to continue speaking for non-review theme open new bug report!
Comment 65 Vladimir Ermakov 2008-10-21 03:01:59 EDT
# ls -l /usr/share/apps/sim/icons
total 1760
-rw-r--r-- 1 root root   4957 2008-09-23 14:42 additional.jisp
-rw-r--r-- 1 root root   5505 2008-09-23 14:42 amibulb.jisp
-rw-r--r-- 1 root root  13881 2008-09-23 14:42 apple_ichat.jisp
-rw-r--r-- 1 root root 138163 2008-09-23 14:42 gants.jisp
-rw-r--r-- 1 root root  71029 2008-10-12 16:15 GPL-Icons.jisp
-rw-r--r-- 1 root root 126566 2008-09-23 14:42 icq5.1.jisp
-rw-r--r-- 1 root root  25632 2008-09-23 14:42 icq5.jisp
-rw-r--r-- 1 root root  52970 2008-09-23 14:42 icqlite.jisp
-rw-r--r-- 1 root root   8195 2008-09-23 14:42 lovenmoney.jisp
-rw-r--r-- 1 root root  63201 2008-09-23 14:42 msn.jisp
-rw-r--r-- 1 root root  56658 2008-09-23 14:42 sim2.jisp
-rw-r--r-- 1 root root 251987 2008-09-23 14:42 SIM-icons-Crystal-full.jisp
-rw-r--r-- 1 root root 258805 2008-09-23 14:42 SIM-icons-Nuvola-full.jisp
-rw-r--r-- 1 root root 246814 2008-09-23 14:42 SIM-icons-XP-full.jisp
-rw-r--r-- 1 root root  65843 2008-09-23 14:42 sim.jisp
-rw-r--r-- 1 root root  19730 2008-09-23 14:42 smiles.jisp
-rw-r--r-- 1 root root 183691 2008-10-12 16:18 weather.jisp
-rw-r--r-- 1 root root 121850 2008-09-23 14:42 yahoo.jisp


please view screenshot
http://img378.imageshack.us/img378/6458/simscreenshotnewconnectsj7.jpg
Comment 66 Pavel Alexeev 2008-10-21 03:55:13 EDT
Stop flame!!

Please, if you want solve problem - open new bug.
I will not futher answer here on this.

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