Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Review Request: dstat https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=199780 ------- Additional Comments From toshio@xxxxxxxxxxxxxxx 2006-07-25 15:47 EST ------- MD5sums: 72917aa5eed385464d70ec731bd6d2b1 dstat-0.6.3-2.src.rpm a2df5d7fecc0115f8eef84141a068e86 dstat-0.6.3.tar.bz2 Blocker: * Your patch adds #!/usr/bin/python to the modules files. Since the modules are not intended to be run from the command line, this is improper behaviour. You should make the files non-executable instead. * It's customary to put the defattr line first in the files section. I believe that rpm uses the defattr line even on lines that preceed it in the file listings but I don't know if that's documented behaviour. If that behaviour ever changes, then you could suddenly have files with incorrect ownership and permissions. * Remove the execute permissions from the scripts in the examples directory. * Package needs to own the dstat directory like this: %dir %{_datadir}/dstat Cosmetic: * It looks like you're patching out dos line endings for html files. This is okay if you're going to submit the change upstream. As a general rule (for instance, if upstream were to not accept the change and you had to continue to carry it around in the package) it's better to use sed or dos2unix to fix this. Otherwise your patch becomes filled with whole files where the only difference is the EOL character. * Some people still use the default rpm defined topdir, sourcedir, etc. When this is so, installing the source rpm places the files in the same directory as a multitude of other rpms. So your patch: patch-to-clean-for-fedora.patch should really have a less generic name. Something like dstat-eol.patch or dstat-eol-cleanup.pach. * I'd remove the commented out code as it doesn't add any value to the spec. * You don't need to Require: python; this is being picked up automatically. Good: * Source matches upstream. * Package Name follows the Naming Guidelines. * Spec file follows the %{name}.spec format. * License is GPL, matches the license field, and is included in the rpm. * Package built on x86_64 as a noarch package. * All BuildRequires not listed in the exceptions have been satisfied. * No locale files, so no need to use %find_lang. * No libraries or pkgconfig files. * Package is not relocatable. * No duplicate files. * Package has a proper %clean section. * Package uses proper macros from Packaging Guidelines. * Code, not content. * No large doc files. * Doc files do not affect the runtime behaviour. * Not a GUI application. * Does not own directories that belong to another package. * No scriptlets. * Package built in mock. After you fix the listed issues, I can run a last rpmlint check and make sure the program runs and then APPROVE the package. Since you need a sponsor, I need to know that you understand the guidelines before SPONSORing you. This package was a good indication of understanding on your part. If you do a review of someone else's package that continues to demonstrate your knowledge of the Fedora Guidelines in particular and rpm packaging in general, I'll sponsor you as well. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA contact for the bug, or are watching the QA contact. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review