Bug 193867 - Review Request: klamav - Clam Anti-Virus on the KDE Desktop
Summary: Review Request: klamav - Clam Anti-Virus on the KDE Desktop
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Chitlesh GOORAH
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-06-02 14:02 UTC by Andy Shevchenko
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-09-28 16:28:33 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
rpmbuild versus mock's build.log (172.96 KB, patch)
2006-09-04 16:26 UTC, Chitlesh GOORAH
no flags Details | Diff

Description Andy Shevchenko 2006-06-02 14:02:20 UTC
Spec URL: ftp://andriy.asplinux.com.ua/pub/people/andy/extras/klamav.spec
SRPM URL: ftp://andriy.asplinux.com.ua/pub/people/andy/extras/klamav-0.37-1.src.rpm
Description:
ClamAV Anti-Virus protection for the KDE desktop.

Comment 1 Chitlesh GOORAH 2006-07-11 10:17:24 UTC
It looks good to me, but however the following needs to be solved:

1.
ERROR   0001: file '/usr/bin/klamav' contains a standard rpath '/usr/lib' in
[/usr/lib:/usr/lib/qt-3.3/lib]
error: Bad exit status from /var/tmp/rpm-tmp.9887 (%install)

You have a rpath error, you solve this add --disable-rpath in %configure

2.
your package is a gui, and hence you need to install the .desktop properly in
the menus:

see http://fedoraproject.org/wiki/Packaging/Guidelines#desktop

here ,
desktop-file-install --vendor fedora \
    --add-category X-Fedora \
    --add-category Utilities \
    --delete-original \
    --dir ${RPM_BUILD_ROOT}%{_datadir}/applications/ \
    ${RPM_BUILD_ROOT}%{_datadir}/applnk/Utilities/%{name}.desktop
would be appropriate

then in %files it will be %{_datadir}/applications/fedora-%{name}.desktop rather
than %{_datadir}/applnk/Utilities/klamav.desktop

don't forget to add desktop-file-utils as BuildRequires

3.
you can remove %define qtdir		%(echo ${QTDIR}) and --with-qt-dir="%{qtdir}" from
%configure

4. Icons

You need to update the gtk+ icon cache as decribed here
http://fedoraproject.org/wiki/ScriptletSnippets#head-fc74f078205565f961f6d836b77c3428619c689d
though its a package for kde.

%post
touch --no-create %{_datadir}/icons/hicolor || :
if [ -x %{_bindir}/gtk-update-icon-cache ]; then
   %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
fi

%postun
touch --no-create %{_datadir}/icons/hicolor || :
if [ -x %{_bindir}/gtk-update-icon-cache ]; then
   %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
fi

Correct all this afterwards, Ill make a full review and approved it if needed :)

Comment 2 Andy Shevchenko 2006-07-11 11:03:23 UTC
Thanks for review.

Updated file here:
SRPM URL: ftp://andriy.asplinux.com.ua/pub/people/andy/extras/klamav-0.37-
2.src.rpm

* Tue Jul 11 2006 Andy Shevchenko <andriy.ua> 0.37-2
- adjust spec according to Fedora Extras review:
  - remove --with-qt-dir, add --disable-rpath
  - BRs: desktop-file-utils
  - place desktop file properly
  - update gtk icon cache


Comment 3 Chitlesh GOORAH 2006-07-11 12:10:05 UTC
MUST Items:
- MUST: rpmlint's output is clean
- MUST: The package is named according to the Package Naming Guidelines.
- MUST: The spec file name matches the base package %{name}
- MUST: The package meets the Packaging Guidelines.
- MUST: The package is licensed (GPL) with an open-source compatible license and
meet other legal requirements as defined in the legal section of Packaging
Guidelines.
- MUST: The License field in the package spec file matches the actual license.
- MUST: the source package includes the text of the license(s) in its own file,
then that file, containing the text of the license(s) for the package is
included in %doc.
- MUST: The spec file must be written in American English.
- MUST: The spec file for the package is be legible. 

****
- MUST: The sources used to build the package must matches the upstream source,
as provided in the spec URL.

You can use
http://kent.dl.sourceforge.net/sourceforge/klamav/klamav-0.37-source.tar.gz

****
- MUST: The package successfully compiles and builds into binary rpms on at
least i386.
- MUST: All build dependencies is listed in BuildRequires.
- MUST: The spec file handles locales properly.
- MUST: If the package does not contain shared library files located in the
dynamic linker's default paths
- MUST: the package is not designed to be relocatable
- MUST: the package owns all directories that it creates.
- MUST: the package does not contain any duplicate files in the %files listing.
- MUST: Permissions on files are set properly.
- MUST: The package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
- MUST: The package consistently uses macros, as described in the macros section
of Packaging Guidelines.
- MUST: The package contains code, or permissable content. This is described in
detail in the code vs. content section of Packaging Guidelines.
- MUST: There are no Large documentation files
- MUST: %doc does not affect the runtime of the application. To summarize: If it
is in %doc, the program must run properly if it is not present.
- MUST: There are no Header files or static libraries 
- MUST: The package does not contain library files with a suffix 
- MUST: Package does NOT contain any .la libtool archives
- MUST: Package containing GUI applications includes a %{name}.desktop file, and
that file must be properly installed with desktop-file-install in the %install
section.
- MUST: Package does not own files or directories already owned by other packages. 

SHOULD Items:

 - SHOULD: The source package does include license text(s) as COPYING
 - SHOULD: mock builds succcessfully in i386.
 - SHOULD: The reviewer tested that the package functions as described. A
package should not segfault instead of running, for example.
 - SHOULD: No scriptlets were used, those scriptlets must be sane. 
 - SHOULD: No subpackages present.

Correct Source and post an updated srpm, Ill approve it

Comment 4 Chitlesh GOORAH 2006-07-11 15:04:34 UTC
one more thing i forgot to say:

%build
CFLAGS="${RPM_OPT_FLAGS}" \
CXXFLAGS="${RPM_OPT_FLAGS}" \
%configure --disable-rpath
%{__make}

use %{__make} %{?_smp_mflags}

Comment 5 Andy Shevchenko 2006-07-11 15:12:20 UTC
Updated file here:
ftp://andriy.asplinux.com.ua/pub/people/andy/extras/klamav-0.37-3.src.rpm

* Tue Jul 11 2006 Andy Shevchenko <andriy.ua> 0.37-3
- fix Source0 URL
- change patch1 to patch0
- use smp if possible when make

P.S. The klamav-X.Y-source isn't same klamav-X.Y. First of their is a couple of 
sources (klamav + dazuko) at one tarball.


Comment 6 Paul Howarth 2006-07-11 15:16:56 UTC
(In reply to comment #4)
> %build
> CFLAGS="${RPM_OPT_FLAGS}" \
> CXXFLAGS="${RPM_OPT_FLAGS}" \
> %configure --disable-rpath

There is no need to set CFLAGS and CXXFLAGS like this. The %configure macro
already includes those definitions.

Try:
$ rpm --eval '%configure'
to see for yourself.



Comment 7 Andy Shevchenko 2006-07-12 07:29:10 UTC
Sure.
I've updated package.
New file here:
ftp://andriy.asplinux.com.ua/pub/people/andy/extras/klamav-0.37-4.src.rpm


Comment 8 Chitlesh GOORAH 2006-07-13 15:35:17 UTC
Did you try the rpm ?

Each time, Im falling on :

"Update Process died unexpectedly! Did you kill it manually?"

This should be fixed.

Comment 9 Andy Shevchenko 2006-07-19 09:37:28 UTC
Yes, I did.

Can you describe step to step what you do?
I can't reproduce this on my PC.

Comment 10 Andy Shevchenko 2006-08-09 12:02:28 UTC
Ping?

I've updated to last release. Package you can found here:
ftp://andriy.asplinux.com.ua/pub/people/andy/extras/klamav-0.38-1.src.rpm

Comment 11 Chitlesh GOORAH 2006-08-23 18:32:15 UTC
Sorry, I was on vacation.

With your latest release, on launching klamav, I'm falling on the error I
mentioned before.

http://www.flickr.com/photo_zoom.gne?id=223058851&size=o

Same for updating the database :(

Comment 12 Andy Shevchenko 2006-08-31 10:24:14 UTC
Please, receive the output of following commands:

pkgs=`rpm -qa | grep clam`; echo $pkgs; rpm -V $pkgs
ldd /usr/bin/klamav


P.S. On fresh updated fc6 the issue isn't reproduced.


Comment 13 Chitlesh GOORAH 2006-09-02 10:16:08 UTC
**** The "Update Process died unexpectedly! Did you kill it manually?" bug and
solution

chitlesh(~)[0]$pkgs=`rpm -qa | grep clam`; echo $pkgs; rpm -V $pkgs
clamav-data-0.88.4-1.fc5 clamav-lib-0.88.4-1.fc5 clamav-0.88.4-1.fc5
clamav-devel-0.88.4-1.fc5

chitlesh(~)[0]$klamav
/bin/bash: freshclam: command not found
QLayout "unnamed" added to Klamav "KlamAV ", which already has a layout
/bin/bash: freshclam: command not found

I was missing clamav-update
Add clamav-update as Requires.


* rpmlint issues

chitlesh(~)[0]$rpmlint /home/chitlesh/rpmbuild/RPMS/i386/klamav-0.38-1.i386.rpm
W: klamav dangling-symlink /usr/share/doc/HTML/en/klamav02/common
/usr/share/doc/HTML/en/common
W: klamav symlink-should-be-relative /usr/share/doc/HTML/en/klamav02/common
/usr/share/doc/HTML/en/common
W: klamav non-executable-in-bin /usr/bin/ScanWithKlamAV 0644
E: klamav non-executable-script /usr/bin/ScanWithKlamAV 0644

chitlesh(~)[0]$rpmlint
/home/chitlesh/rpmbuild/RPMS/i386/klamav-debuginfo-0.38-1.i386.rpm
E: klamav-debuginfo script-without-shellbang
/usr/src/debug/klamav-0.38/src/klammail/client.c
E: klamav-debuginfo script-without-shellbang
/usr/src/debug/klamav-0.38/src/klammail/options.c
E: klamav-debuginfo script-without-shellbang
/usr/src/debug/klamav-0.38/src/klammail/options.h
E: klamav-debuginfo script-without-shellbang
/usr/src/debug/klamav-0.38/src/klammail/clamdmail.c
E: klamav-debuginfo script-without-shellbang
/usr/src/debug/klamav-0.38/src/klammail/treewalk.c

chitlesh(~)[0]$rpmlint /home/chitlesh/rpmbuild/SRPMS/klamav-0.38-1.src.rpm
chitlesh(~)[0]$                                                               

You should check rpmlint on all your *.rpm

**** Using %{buildroot} and %{optflags} vs  $RPM_BUILD_ROOT and $RPM_OPT_FLAGS
- Mixed use is found. Please unify the usage.

You could use 
%{__rm} -rf %{buildroot}
instead of
%{__rm} -rf "${RPM_BUILD_ROOT}"


**** Correct and update the spec file and srpm, i'll check them again

Comment 14 Andy Shevchenko 2006-09-03 04:43:46 UTC
* Sat Sep 02 2006 Andy Shevchenko <andriy.ua> 0.38-2
- require freshclam for correct DB update
- fix rpmlint claim:
  - pack ScanWithKlamAV as executable
  - do not use absolute link
- use $RPM_BUILD_ROOT and $RPM_OPT_FLAGS in all places

Updated file here:
ftp://andriy.asplinux.com.ua/pub/people/andy/extras/klamav-0.38-2.src.rpm


Comment 15 Chitlesh GOORAH 2006-09-04 16:25:12 UTC
 * rpmlint warnings were not corrected

chitlesh(~)[1]$rpmlint
/var/lib/mock/fedora-5-i386-core/result/klamav-0.38-2.fc5.i386.rpm
W: klamav dangling-relative-symlink /usr/share/doc/HTML/en/klamav02/common ../common
chitlesh(~)[1]$rpmlint /var/lib/mock/fedora-5-i386-core/result/klamav-
klamav-0.38-2.fc5.i386.rpm            klamav-0.38-2.fc5.src.rpm            
klamav-debuginfo-0.38-2.fc5.i386.rpm
chitlesh(~)[1]$rpmlint
/var/lib/mock/fedora-5-i386-core/result/klamav-debuginfo-0.38-2.fc5.i386.rpm
E: klamav-debuginfo script-without-shellbang
/usr/src/debug/klamav-0.38/src/klammail/client.c
E: klamav-debuginfo script-without-shellbang
/usr/src/debug/klamav-0.38/src/klammail/options.c
E: klamav-debuginfo script-without-shellbang
/usr/src/debug/klamav-0.38/src/klammail/options.h
E: klamav-debuginfo script-without-shellbang
/usr/src/debug/klamav-0.38/src/klammail/clamdmail.c
E: klamav-debuginfo script-without-shellbang
/usr/src/debug/klamav-0.38/src/klammail/treewalk.c


* I've compared the rpmbuild output with the mock's build.log, it seems that
when building with mock it generates more warnings than with rpmbuild. Do you
know why? see attachment

Comment 16 Chitlesh GOORAH 2006-09-04 16:26:46 UTC
Created attachment 135503 [details]
rpmbuild versus mock's build.log

Comment 17 Andy Shevchenko 2006-09-04 17:04:45 UTC
1. 'common' link is a usual link for the KDE HTML documentation.
I see two ways to resolve issue:
 - leave as is
 - make empty common directory and own in in the %files
What solution do you advise?

2. Is it correct to check debuginfo packages by rpmlint? 

3. About warnings. I guess the different compilators and/or different CFLAGS is 
used when compiled by mock and by rpmbuild. You may compare the cflags in such 
cases.
The attached warnings are not affect to the result binary. 



Comment 18 Chitlesh GOORAH 2006-09-04 22:58:49 UTC
(In reply to comment #17)
> 1. 'common' link is a usual link for the KDE HTML documentation.
> I see two ways to resolve issue:
>  - leave as is
>  - make empty common directory and own in in the %files
> What solution do you advise?
> 
The rpmlint warning is safe to ignore; %_docdir/HTML/en/common is owned by
kdelibs, which is a dependency (via libkdecore.so.4).

Hence, leave as is

> 2. Is it correct to check debuginfo packages by rpmlint? 

you should check all rpms.

update your srpm and spec file

Comment 19 Chitlesh GOORAH 2006-09-24 18:34:50 UTC
ping ?

Comment 20 Andy Shevchenko 2006-09-25 08:04:03 UTC
Sorry for later answer, I was busy by my work.
About debuginfo package. I have no ideas to it. Why rpmlint complains to *.[ch] 
files? 

Comment 21 Chitlesh GOORAH 2006-09-25 21:24:56 UTC
#1: remove zlib-devel as BuildRequires
since curl-devel requires openssl-devel and the later requires zlib-devel

#2: replace "Requires: %{_bindir}/freshclam" by "Requires: clamav-update"

#3: In %post and %postun
replace

if [ -x %{_bindir}/gtk-update-icon-cache ]; then
    %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
fi

by

%{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :

since "|| :" causes the command to exit with a successful exit status whether or
not the command worked.

#4  debug rpm:
chitlesh(SPECS)[0]$rpmlint
/home/chitlesh/rpmbuild/RPMS/i386/klamav-debuginfo-0.38-2.i386.rpm
E: klamav-debuginfo script-without-shebang
/usr/src/debug/klamav-0.38/src/klammail/client.c
E: klamav-debuginfo script-without-shebang
/usr/src/debug/klamav-0.38/src/klammail/options.c
E: klamav-debuginfo script-without-shebang
/usr/src/debug/klamav-0.38/src/klammail/options.h
E: klamav-debuginfo script-without-shebang
/usr/src/debug/klamav-0.38/src/klammail/clamdmail.c
E: klamav-debuginfo script-without-shebang
/usr/src/debug/klamav-0.38/src/klammail/treewalk.c

Solution:
in %install, add
chmod 644 src/klammail/*.{c,h}

#5: rpmlint rpm
chitlesh(SPECS)[0]$rpmlint /home/chitlesh/rpmbuild/RPMS/i386/klamav-0.38-2.i386.rpm
W: klamav dangling-relative-symlink /usr/share/doc/HTML/en/klamav02/common ../common

/tmp/klamav-0.38-2.i386.rpm.20210/usr/share/applications/fedora-klamav.desktop:
warning: file contains key "DocPath", this key is currently reserved for use
within KDE, and should in the future KDE releases be prefixed by "X-"

Ignore both.

/tmp/klamav-0.38-2.i386.rpm.20210/usr/share/applications/fedora-klamav.desktop:
warning: boolean key "Terminal" has value "0", boolean values should be "false"
or "true", although "0" and "1" are allowed in this field for backwards
compatibility

Solution (one line):
%{__sed} -i.orig -e '/^Terminal/s|^.*$|Terminal=false|' \
    ${RPM_BUILD_ROOT}%{_datadir}/applnk/Utilities/%{name}.desktop
%{__rm} -f ${RPM_BUILD_ROOT}%{_datadir}/applnk/Utilities/%{name}.desktop.orig
before "desktop-file-install --vendor fedora \" ..


#6: in %doc, drop NEWS, because it's empty

Comment 22 Chitlesh GOORAH 2006-09-25 22:46:16 UTC
drop bzip2-devel as well as BuildRequires, as klamav builds successfully without
it in mock.

Comment 23 Andy Shevchenko 2006-09-27 10:15:29 UTC
Updated file here:
ftp://andriy.asplinux.com.ua/pub/people/andy/extras/klamav-0.38-3.src.rpm

* Wed Sep 27 2006 Andy Shevchenko <andy.ua> 0.38-3
- drop zlib-devel and bzip2-devel
- require clamav-update
- remove condition check from post scriptlets
- satisfy rpmlint claim on debuginfo subpackage
- fix Tefminal value in desktop-file
- do not ship NEWS file due to is empty


Comment 24 Chitlesh GOORAH 2006-09-27 15:05:54 UTC
One last thing :)
- fix Tefminal value in desktop-file

Correct this typo, It should be "Terminal". However I won't block this package
because I'm sure you will update the package to 0.38-4, before committing to
CVS, right ?

This package has been APPROVED.

Comment 25 Andy Shevchenko 2006-09-28 16:28:33 UTC
Yes, you are right.

Thank you for review.


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