[Bug 678634] Review Request: Saaghar - A Cross-Platform Persian Poetry Software

[bugzilla at redhat.com]

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=678634

--- Comment #2 from Hedayat Vatankhah <hedayatv at gmail.com> 2011-03-13 19:09:27 EDT ---
(In reply to comment #1)
> Hello Hedayat,
Hi! Thanks a lot for reviewing this package :)

> 
> Few things to note :
> 
> - Consider using lower case for the spec and package name :
> 
> http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Case_Sensitivity
When I was selecting the name I checked various placing in the code and the
project and I conclude that upstream prefers Saaghar to saaghar, so I decided
to go with the former (e.g. the archive file name and the name of Debian
packages generated by upstream). 


> 
> - BuildRoot and %clean are not required any more :
> 
> http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
OK, removed.


> 
> - Add some description for the patches - are they Fedora specific? Can the
> patches be integrated into upstream? See :
> 
> http://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment
> 
Oops, sorry for forgetting that. Added.


> - Use desktop-file-validate :
> http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage Added (was a little misinterpretation :P).

> 
> - Looks like a typo in the release field :
> 
> Release:        1.1051%{?dist}
Nope. A little description added to the first patch. This is in fact a minor
version used by upstream for the version with Saaghar-upstream_update.patch
patch applied (the version is directly extracted from a version macro in source
files).

> 
> - You can omit the %dir in the files section :
> 
> %dir %{_datadir}/saaghar
I don't want to add the whole directory to the main package. There is a DB
inside %{_datadir}/saaghar which is included in Saaghar-data sub-package. But I
wanted the main package to own the directory.

> 
> 
> HTH

Thanks again. 

Updated SRPM and SPEC files are here:
SPEC: http://hedayat.fedorapeople.org/reviews/Saaghar/0.7.2-2/Saaghar.spec
SRPM:
http://hedayat.fedorapeople.org/reviews/Saaghar/0.7.2-2/Saaghar-0.7.2-2.1051.fc14.src.rpm

-- 
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.