Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=914996 Pavel Raiskup <praiskup@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review? --- Comment #6 from Pavel Raiskup <praiskup@xxxxxxxxxx> --- Hi Stephenm, here is my first comment iteration :) ======= 1. bad spec naming? > Upstream have already updated to resolve this so I've updated the SPEC and > SRPM to pick this up. I also realized that there is .pod file provided which > can be used to generate the man page so that is now also included. > > Spec URL: http://fedorapeople.org/~sgordon/gitstats-0.2.spec > SRPM URL: http://fedorapeople.org/~sgordon/gitstats-0.2-20130224git0843039.fc18.src.rpm Why do you now call the spec file gitstats-0.2.spec and not gitstats.spec? Your first specfile was called gitstats.spec.. 2. unnecessary %attr() macros > $ cat *.spec > [...] > %files > %attr(755, root, root) %{_bindir}/%{name} ^^^^^^^^^^^^^^^^^^^^^^ none of these should be needed 3. Missing release number > Release: %{checkout}%{?dist} I think that even the first release number should be specified. Partly because I (as a reviewer) can see, where it will be finally placed and because of this also: $ rpmdev-vercmp gitstats-0.2-20130224git0843039 gitstats-0.2-1.20130224git0843039 gitstats-0.2-20130224git0843039 > gitstats-0.2-1.20130224git0843039 4. (nit: I would create one-newline separator between changelog entries) 5. Gzip inside BuildRequires is redundant .. it must be installed on minimum build system. Pavel -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=L8GL0SGVx7&a=cc_unsubscribe _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review