[Bug 537587] Review Request: dspam - bayesian filtering daemon, client, library and web ui
bugzilla at redhat.com
bugzilla at redhat.com
Sun Nov 29 18:46:14 UTC 2009
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=537587
Mamoru Tasaka <mtasaka at ioa.s.u-tokyo.ac.jp> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |mtasaka at ioa.s.u-tokyo.ac.jp
--- Comment #34 from Mamoru Tasaka <mtasaka at ioa.s.u-tokyo.ac.jp> 2009-11-29 13:46:10 EDT ---
Some notes
A. Description, etc.
* Naming
- Please explain why you want to name subpackages in libdspam(-foo)
style. On Fedora usually subpackages are named as
"dspam-libs" or "dspam-devel" or "dspam-hash" or so.
! By the way, but for "libdspam" and "libdspam-devel", the naming
of libdspam-foo subpackages are definitely wrong because
none of these package contains system-wide libraries.
- Files under %_libdir/dspam are just plugins and not system-wide
libraries. Also see below (about calling ldconfig)
- "%package -n dspam-web" can simply be "%package web"
* Dependency between subpackages
https://fedoraproject.org/wiki/Packaging/Guidelines#Requiring_Base_Package
- "dspam" binary rpm should have "dspam-libs = %{version}-%{release}"
- "dspam-hash" should have "dspam = %{version}-%{release}"
(and other subpackage should have similar dependency).
! Note that dependency between subpackages should usually be EVR
(Epoch-Version-Release) specific, not just version.
* Other dependency
- "Requires: pkgconfig" (for -devel subpackage) is no longer needed
(for F-11/12/13)
https://fedoraproject.org/wiki/PackagingDrafts/PkgconfigAutoRequires
(This is still a draft, however will be accepted on 2009-12-03).
- For perl module related dependency, please use virtual provides names
instead of using rpm names directly.
https://fedoraproject.org/wiki/Packaging/Perl#Perl_Requires_and_Provides
B. %prep, %build, %install, %check
* Timestamp
- Please consider to use
---------------------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
---------------------------------------------------------------
to keep timestamps on installed files. This method usually
works for Makefiles generated by recent autotools.
C. scriptlets / service
* "source"ing /etc/profile
- Why do you "source" /etc/profile? (note that rpm executes these
scriptlets in subshells and subshell exits when those scriptlets
are done)
* Calling /sbin/ldconfig
- You don't have to call /sbin/ldconfig for rpms not containing
system-wide libraries (i.e. needed only for dspam-libs binary rpm)
* restart service on %postun
- is wrong because "restart" activates service even if the service
was off before (this should be condrestart)
https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets
* Missing Requires
- Add missing "Requires(post): chkconfig" or so:
https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets
* service-default-enabled
https://fedoraproject.org/wiki/Packaging/SysVInitScript#.23_chkconfig:_line
- rpmlint shows:
-------------------------------------------------------------
dspam.i686: W: service-default-enabled /etc/rc.d/init.d/dspam
-------------------------------------------------------------
i.e. currently dspam service is enabled by default once dspam rpm
is enabled, which is usually not desided.
Usually service should be off by default, i.e. change the line
-------------------------------------------------------------
# chkconfig: 345 70 30
-------------------------------------------------------------
to
-------------------------------------------------------------
# chkconfig: - 70 30
-------------------------------------------------------------
D. %files
* %defattr
- Now we usually use %defattr(-.root.root,-)
* Owner/Group/Permission
---------------------------------------------------------------
-r-x--s--x 1 root root 88252 Nov 29 16:10 /usr/bin/dspam
---------------------------------------------------------------
- However your spec file shows:
---------------------------------------------------------------
120 %configure \
137 --with-dspam-owner='%{dspam_user}' \
138 --with-dspam-group='%{dspam_group}' \
---------------------------------------------------------------
and build.log shows:
---------------------------------------------------------------
779 chown: changing ownership of
`/builddir/build/BUILDROOT/dspam-3.9.0-0.9.BETA4.fc13.i386/usr/bin/dspam':
Operation not permitted
784 chgrp: changing group of
`/builddir/build/BUILDROOT/dspam-3.9.0-0.9.BETA4.fc13.i386/usr/bin/dspam':
Operation not permitted
801 chown: changing ownership of
`/builddir/build/BUILDROOT/dspam-3.9.0-0.9.BETA4.fc13.i386/var/lib/dspam':
Operation not permitted
802 chgrp: changing group of
`/builddir/build/BUILDROOT/dspam-3.9.0-0.9.BETA4.fc13.i386/var/lib/dspam':
Operation not permitted
815 chown: changing ownership of
`/builddir/build/BUILDROOT/dspam-3.9.0-0.9.BETA4.fc13.i386/var/log/dspam':
Operation not permitted
816 chgrp: changing group of
`/builddir/build/BUILDROOT/dspam-3.9.0-0.9.BETA4.fc13.i386/var/log/dspam':
Operation not permitted
---------------------------------------------------------------
So the owner/group of these files/directories seem wrong (in
the binary rpms). Set these explicitly by %attr.
- Also build.log shows:
---------------------------------------------------------------
798 chmod "770"
/builddir/build/BUILDROOT/dspam-3.9.0-0.9.BETA4.fc13.i386/var/lib/dspam; \
812 chmod "770"
/builddir/build/BUILDROOT/dspam-3.9.0-0.9.BETA4.fc13.i386/var/log/dspam; \
---------------------------------------------------------------
However, currently these directories have 0755 permission (in the
binary rpm). Please check if permissions are set correctly.
* %files entry unification
- By the way %files entry
---------------------------------------------------------------
%files
%dir foo/
foo/*
---------------------------------------------------------------
(where foo/ is a directory) can be unified as
---------------------------------------------------------------
%files
foo/
---------------------------------------------------------------
This style contains the directory foo/ itself and all files/directories/etc
under foo/.
* Macros
- Use %{_initddir} for %_sysconfdir/rc.d/init.d:
https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_on_the_filesystem
* Directory ownership issue
- The following directories are not owned by any packages:
--------------------------------------------------------------
/usr/share/dspam/
/usr/share/dspam/sql-scripts/
--------------------------------------------------------------
E. Misc
* Permission of files in srpm
--------------------------------------------------------------
dspam.src: W: strange-permission dspam-logrotate 0600
dspam.src: W: strange-permission dspam-cron 0775
dspam.src: W: strange-permission dspam-init.d 0600
--------------------------------------------------------------
- Usually we request that all files in srpm should be 0644.
--
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.
More information about the Fedora-package-review
mailing list