[Bug 528108] Review Request: Vuurmuur - Firewall manager built on top of iptables
bugzilla at redhat.com
bugzilla at redhat.com
Wed Nov 4 18:05:58 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=528108
Mamoru Tasaka <mtasaka at ioa.s.u-tokyo.ac.jp> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |mtasaka at ioa.s.u-tokyo.ac.jp
--- Comment #2 from Mamoru Tasaka <mtasaka at ioa.s.u-tokyo.ac.jp> 2009-11-04 13:05:55 EDT ---
Some random remarks from one of the sponsor members:
A. Summary, description, etc
* EVR (Epoch-Version-Release)
- For formally released (i.e. non-snapshot) package, the release
number must begin with 1%{?dist}, and then be incremented
every time you modify your spec file (and should be reset
to 1%{?dist} then version is upgrade):
https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Package_Release
* Vendor
- "Vendor" item should be removed on Fedora. Fedora buildsystem (koji)
automatically sets this value.
* License
- The license tag for this package should be "GPLv2+". ref:
https://fedoraproject.org/wiki/Packaging/LicensingGuidelines#.22or_later_version.22_licenses
* BR (BuildRequires)
- This package does not build.
http://koji.fedoraproject.org/koji/taskinfo?taskID=1787887
Please try to rebuild your srpm with mock:
https://fedoraproject.org/wiki/Extras/MockTricks
to check if all needed BuildRequires are included.
( Note: perhaps "BR: libtool automake ncurses-devel" are also needed
for this package )
* Dependency between subpackages
- All subpacakges other than "Vuurnuur" binary rpm should have
"Requires: %{name} = %{version}-%{release}":
https://fedoraproject.org/wiki/Packaging/Guidelines#Requiring_Base_Package
B. %prep, %build
* Working directory
- At the beginning of %build, %install, the working directory is
%{_builddir}/%{name}-%{version} (by default, unless you explicitly change
this value at %setup).
So I suggest to write something like:
--------------------------------------------------------------
%build
cd libvuurmuur-%{version}
LIBVUDIR=$(pwd)
%configure --with-plugindir=%{_libdir}/vuurmuur/plugins
sed ...
....
make %{?_smp_mflags}
cd ../vuurmuur-%{version}
%configure --with-libvuurmuur-includes=${LIBVUDIR}/src ....
.....
---------------------------------------------------------------
Also I suggest not to repeat somewhat long
"%{_builddir}/%{name}-%{version}/libvuurmuur-%{version}" string and
use some variable.
* Parallel make
- Support parallel make unless impossible:
https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make
If parallel make cannot be supported, please write some comments
on the spec file.
C %install
* About the following lines
---------------------------------------------------------------
cd %{_builddir}/%{name}-%{version}/vuurmuur-%{version}
make install DESTDIR=$RPM_BUILD_ROOT
for i in `find %{_builddir}/%{name}-%{version}/vuurmuur-%{version}/skel -name
.keep`
do
rm -rf $i
done
---------------------------------------------------------------
- Would you explain why .keep files under %_builddir (not $RPM_BUILD_ROOT)
have to be removed after the installation is already done by the above
line?
* %{_initddir}
- Use %{_initddir} instead of %{_sysconfdir}/rc.d/init.d:
https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_on_the_filesystem
D. Scriptlets
* SysVinit scrptlets related dependency
- Add proper Requires(post) or to to -daemon subpackage:
https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets
E. %files
* plugin
- From libvuurmuur-0.7/src/backendapi.c:
---------------------------------------------------------------
137 if(snprintf(plugin_location, sizeof(plugin_location),
"%s/plugins/lib%s.so", conf.plugdir, plugin_name) >=
(int)sizeof(plugin_location))
138 {
143 return(-1);
144 }
146 plugin->handle = open_plugin(debuglvl, plugin_location);
147 if(!plugin->handle)
148 {
151 return(-1);
152 }
---------------------------------------------------------------
Like this, usually plugins are expected to have the names of libfoo.so,
not libfoo.so.X.Y as these are plugins, not libraries.
So splitting %{_libdir}/vuurmuur/plugins/libtextdir.so.* and
%{_libdir}/vuurmuur/plugins/libtextdir.so into defferent subpackages is
perhaps nonsense (and plugin files should not be packaged in -devel
subpackage).
Note that usually %{_libdir}/libbar.so have to be installed in XXX-devel
package, however plugin files named libfoo.so can be installed in non-devel
package
* Manfile
- Files under %_mandir are automatically marked as %doc
- Files under %{_mandir}/ru/ should be marked as %lang(ru).
* Static archive
- Static archives (*.a files) must not be packages unless needed:
https://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries_2
* Duplicate files
-----------------------------------------------------------------
$ rpm -qlp *rpm | sort | uniq -d
/usr/share/man/man8/vuurmuur_conf.8.gz
/usr/share/man/ru/man8/vuurmuur_conf.8.gz
/usr/share/vuurmuur/config
/usr/share/vuurmuur/config/config.conf.sample
/usr/share/vuurmuur/config/vuurmuur_conf.conf.sample
/usr/share/vuurmuur/help
/usr/share/vuurmuur/help/vuurmuur-fr.hlp
/usr/share/vuurmuur/help/vuurmuur-ru.UTF-8.hlp
/usr/share/vuurmuur/help/vuurmuur-ru.hlp
/usr/share/vuurmuur/help/vuurmuur.hlp
-----------------------------------------------------------------
- These files are installed in more than 2 packages. Please fix
%files list so that all files are listed only once.
F. And more...
>From rpmlint:
* Vuurmuur-daemon.i686: W: service-default-enabled /etc/rc.d/init.d/vuurmuur
- vuurmuur service is enabled by default after install, which is
usually not desired. The line
-----------------------------------------------------------------
9 # chkconfig: 345 91 9
-----------------------------------------------------------------
should be changed to:
-----------------------------------------------------------------
# chkconfig: - 91 9
-----------------------------------------------------------------
https://fedoraproject.org/wiki/Packaging/SysVInitScript#.23_chkconfig:_line
* Vuurmuur-daemon.i686: E: no-status-entry /etc/rc.d/init.d/vuurmuur
- "status" option should usually be supported.
* %{_localstatedir}/log/vuurmuur
- From %{_sysconfdir}/logrotate.d/vuurmuur, vuurmuur service creates
log files under %{_localstatedir}/log/vuurmuur, however no package
seems to own this directory, which is perhaps wrong. Please make
this directory owned by some package.
When you modify your spec file, please post the URLs new spec file/srpm
on this bug.
--
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