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=806446 --- Comment #2 from Germán Racca <gracca@xxxxxxxxx> 2012-04-04 08:26:00 EDT --- (In reply to comment #1) > Hi, Hi Michael, sorry for the delay in the answer. > a few comments : > - you should add a note for the patch ( ie, say if it come from upstream or > not, > https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment Although it was Spot who made that change to the spec file, I added a comment for the patch. > - Provides: minutunes%{?_isa} = %{version}-%{release} > > I am not sure about adding the {?_isa} part, as the whole idea of adding the > provides is to help someone typing "yum install minitunes". I think that's > rather unconventional, and maybe not useful. The ISA part is specified here: https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages and if you are not sure about that, then I prefer to leave it unchanged. > - there is a typo, this is minitunes, not minutunes ( in the provides lines ) Fixed. > - BuildRequires: qt-devel taglib-devel phonon-devel gcc-c++ > it is IMHO better to have 1 line for each buildRequires, since this produce > better diff, and so allow to review patches more easily. Fixed. New files here: Spec: http://skytux.fedorapeople.org/packages/musique.spec SRPM: http://skytux.fedorapeople.org/packages/musique-1.1-5.fc16.src.rpm Thanks for the comments, Germán. -- 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