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=595011 Rafael Aquini <aquini@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |aquini@xxxxxxxxx --- Comment #2 from Rafael Aquini <aquini@xxxxxxxxx> 2010-05-22 18:19:36 EDT --- David, In spite of this is being an informal review, I ask you to consider some of the following observations, please. * As I could not find your Fedora Account, I don't know if this is your first RPM package submission or not. If this is you first submission, take a closer look at: http://fedoraproject.org/wiki/PackageMaintainers/Join * As you stated that there is no longer upstream maintenance for sshdfilter you should be prepared to maintain it as you were its own upstream. Take a look at: http://fedoraproject.org/wiki/PackageMaintainers/WhyUpstream#Some_Examples_Of_Exceptions * I think there is no need to sshdfilter.spec explicitly list any 'Requires:' at all. Even though, if they are really necessary, listing twice rsyslog is surely unnecessary. For further reference on 'Requires:', take a look at: http://fedoraproject.org/wiki/Packaging:Guidelines#Requires * You should write a %changelog section, with useful stuff to keep track of modifications made to the package. * Under %install section of Spec: - There is no need for 'mkdir $RPM_BUILD_ROOT' - You should be using the '-p' option to preserve file timestamps - You shouldn't be forcing that .gz extention to man files. Let rpmbuild do its job. - For the sake of legibility, consider rewrite spec's %install to sth like: %install rm -rf %{buildroot} install -p -m 0644 -D etc/%{name}rc %{buildroot}/%{_sysconfdir}/%{name}rc install -p -m 0755 -D etc/init.d/%{name} %{buildroot}/%{_sysconfdir}/rc.d/init.d/%{name} install -p -m 0755 -D source/%{name}.pl %{buildroot}/%{_sbindir}/%{name} install -p -m 0644 -D man/%{name}.1 %{buildroot}/%{_mandir}/man1/%{name}.1 install -p -m 0644 -D man/%{name}rc.5 %{buildroot}/%{_mandir}/man5/%{name}rc.5 mkdir -p %{buildroot}/%{_var}/run/ mkfifo -m 0644 %{buildroot}/%{_var}/run/%{name}.fifo * Under %files, consider rewrite to sth like: %files %defattr(-,root,root,-) %doc INSTALL todo %{_sysconfdir}/%{name}rc %{_sysconfdir}/rc.d/init.d/%{name} %{_sbindir}/%{name} %{_mandir}/man1/%{name}.1* %{_mandir}/man5/%{name}rc.5* %{_var}/run/%{name}.fifo * Once RPM packages keep track of their own files, there is no need to do all those 'rm -f' in %postun * Please, consider a cleaner approach to modify config files owned by other packages. Best regards -- 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review