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@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_a...
Oops, sorry for forgetting that. Added.
- Use desktop-file-validate :
http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usag... 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...