Fedora Merge Review: mtools http://cvs.fedora.redhat.com/viewcvs/devel/mtools/ Initial Owner: atkac
Hi there, here's a quick review: good: + source matches upstream + license is OK and correctly included (GPL) + specfile is legible and looks good + dir ownership and permissions look good + has %clean and "rm -rf $RPM_BUILD_ROOT" + pot/preun scriptlets look good + dir ownership and perms OK needswork: - please see the rpmlint output at: http://linux.dell.com/files/fedora/FixBuildRequires/mock-results-core/i386/mtools-3.9.10-3.fc7.src.rpm/result/rpmlint.log which lists a few items that need attention - please use the preferred BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
Assigning to owner who should fix the problems.
problems are fixed in mtools-3.9.10-4.fc7
Assigning back to the reviewer.
Setting flag back to ? to notify ed to re-check things.
Ed, are you going to review mtools, please?
Yes, I'll do it this weekend!
Mass reassigning all merge reviews to their component. For more details, see this FESCO ticket: https://fedorahosted.org/fesco/ticket/1269 If you don't know what merge reviews are about, please see: https://fedoraproject.org/wiki/Merge_Reviews How to handle this bug is left to the discretion of the package maintainer.
Since Ed's review +'s still hold true and rpmlint's pretty happy apart from 3 bogus dates, I consider this issue resolved.
Few additional things you can improve in spec like we have following tags now optional and can be removed safely 1) Group: 2) Buildroot: 3) in %install no need of rm -rf $RPM_BUILD_ROOT 4) %clean section 5) %defattr(-,root,root) Then we have macro for /etc which is %{_sysconfdir} Every patch used should provide information in comment above to that patch line like why patch is needed and if it is upstream or not? Please look http://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used and see if you can avoid using %makeinstall