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@xxxxxxxxxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtasaka@xxxxxxxxxxxxxxxxxxx --- Comment #2 from Mamoru Tasaka <mtasaka@xxxxxxxxxxxxxxxxxxx> 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review