[Bug 176253] Review Request: clement-2.1

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

Summary: Review Request: clement-2.1


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


jeff at ultimateevil.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jeff at ultimateevil.org




------- Additional Comments From jeff at ultimateevil.org  2005-12-26 10:54 EST -------
This isn't a full review, I'm just noting some things to clean up in the spec.

First of all, the %description is huge.  I personally feel it should be one or
two paragraphs.  I don't think that's a blocker.

The Summary tag shouldn't contain the package name.  Just make it "An
application to..."

You should get rid of the %define dist at the top.  If you want a %dist tag in
your personal repository, add it to your .rpmmacros file.

Version: %{version} and %Release: %{release}.%{dist} are not defined, and
probably not handled very well (as in the build system will probably choke). 
Just use real relase numbers and such, like 2.1 and 31%{?dist}.

The Source should be the full URL where the source can be downloaded.

The Vendor tag is deprecated.  Please remove it.

The BuildRoot should be
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) -- see
http://fedoraproject.org/wiki/PackagingGuidelines.

The Requires: clamav is probably not necessary.  The RPM packaging process will
discover this if the package is really linked against clamav.

Is the Requires: httpd absolutely necessary?  I understand that users won't have
access to the quarantine function without it, but will the program run without a
web server?  If so, you shouldn't require httpd, but leave a note in a README
file somewhere.

In the %files section, %defattr should be "(-,root,root,-)" -- note final dash.
 This preserves the mode of directories as well.

What is your macro ${prefix} set to?  Is it /usr?  (As an aside, don't use this
macro like you have done, for one it's not defined in your spec file, and second
it is better to use the macro %{_prefix} which the RPM subsystem already has
defined.  Also, a local administrator can change it to suit non-conformist
freakish desires.)  If it is /usr, your package winds up owning it, which
conflicts with the filesystem package.  A better example would be something like
the following:

%{_bindir}/*
%{_sbindir}/*
%{_datadir}/%{name}/
%{_libdir}/%{name}/

and so on, which will include all files under /usr/bin and /usr/sbin, and the
entire directories of /usr/share/clement and /usr/lib/clement including all
files therein.

It is my adamant opinion that no files in /usr should ever need to be written
directly by the administrator or any other user.  You have a config file listed
in %{prefix}/conf (again, %{prefix} is not defined).  All config files should be
under /etc.  Use the %{_sysconfdir} macro for /etc.  Personally, I prefer
packages to place their config files in %{_sysconfdir}/%{name}.

The attr() on /etc/sysconfig/clement is probably not necessary, if it was
installed with the appropriate permissions (defattr should be right).  Why
should it be writable by the mail user?  It is readable by all users, so root
owning it should be sufficient and more secure.

Also, as above, attr() on /etc/init.d/clement (should be
%{_sysconfdir}/rc.d/init.d/clement) is superfluous if it is installed correctly.

I really think %files as a whole should come after %prep, %build, %install, and
%pre/post/preun/postun.  I'm not sure if that's a hard and fast rule, though.

If you are going to run chkconfig in %post and %preun, then you need to
Requires(post) and Requires(preun): chkconfig.

You should not let chkconfig make the program start automatically in %post. 
Just chkconfig --add it.  It will be set to K**clement in every runlevel.  Let
the administrator turn it on.  Also, get rid of the chkconfig --list line.

I'm not sure what those %{prefix}/support/add*.sh scripts to, but I assume they
help build the default config file.  That's nice, but I don't think you should
do that in %post.  Someone can correct me if I'm wrong, but I think it would be
better to include a script and maybe put it in /usr/share/clement
(%{_datadir}/%{name}) with a note in the default config file saying "please run
/usr/share/clement/*.sh to get a nice baseline config."

In %preun, I would change the path /etc/init.d to %{_sysconfdir}/rc.d/init.d. 
Also, Requires(preun): initscripts, because you require the service command. 
And if you followed what I was saying above about paths and such, that whole (
cd %{prefix} ... line at the end needs to go.

You can use make and rm instead of %{__make} and %{__rm}.  That's up to you.  I
think it's cleaner with the commands instead of the macros.

I'm not sure what that mkcron.sh script does exactly, but you might want anacron
scripts to just go into /etc/cron.daily.

Finally, I don't recall seeing your name or email address on the
fedora-extras-list before.  If you are a new contributor, you need to identify
yourself as such so someone who can sponsor you will help you out.  I'm not a
sponsor.

In spite of my ripping your spec file to shreds (it happens to us all our first
time), I want to say "thank you" for writing this program.  As an administrator
of several email servers and domains, I always appreciate people who help fight
spam.  I personally had plans to spend my time on something else last week, but
I wound up working almost all week on one particular spam problem instead, it
was litterally turning into a DoS attack.  So thanks for fighting the good fight.


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