[Bug 525786] Review Request: popfile - Automatic Email Classification
bugzilla at redhat.com
bugzilla at redhat.com
Mon Sep 28 11:59:52 UTC 2009
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=525786
--- Comment #3 from Naoki IIMURA <naoki at getpopfile.org> 2009-09-28 07:59:51 EDT ---
Thanks for comments.
> * License statement of the spec file itself
> - It is not forbidden, however ususally spec files in Fedora
> packages don't contain license statements on the spec files
> themselves.
OK. I've removed the license statements.
> * Naming/EVR (Epoch-Version-Release)
> - The release number of the spec file should have integer
> value except for the cases written in
> https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Package_Release
>
> - And usually adding %{?dist} tag is preferred.
> https://fedoraproject.org/wiki/Packaging/DistTag
OK. I've update the release number to "2%{?dist}".
> * Vendor
> - Vendor tag must be removed on Fedora. This tag is automatically imported
> on Fedora build server.
OK. I've removed the Vendor tag.
> * Exclusiveos
> - I don't think you have to write this explicitly.
OK. I've removed the Exclusiveos tag.
> * Perl related packaging treatment
> https://fedoraproject.org/wiki/Packaging/Perl
> - "Requires: perl >= 5.8.1" does almost nothing because Fedora's perl
> rpm has epoch:
> <snip>
> - And anyway I don't think this is needed because even Fedora 10
> (the oldest branch currently supported on Fedora) uses perl 5.10.
OK. I've removed the Requires tag for perl.
> - For perl module dependency, please write virtual Provides names
> instead of rpm names directly:
> https://fedoraproject.org/wiki/Packaging/Perl#Perl_Requires_and_Provides
OK. I've updated the perl module dependencies to use virtual Provides names.
> * rpmlint issue
> ------------------------------------------------------
> popfile.src: W: strange-permission popfile 0775
> popfile.src: W: strange-permission start_popfile.sh 0775
> ------------------------------------------------------
> - All files in srpm should have 0644 permission.
OK. I've applied appropriate permissions.
> * %setup/%build/%install
> - Umm... I think your current spec file is unneededly verbose
> and redundant. While listing %files section verbosely can be
> accepted, is there any reason you don't prefer to write like
> below? (all commentes removed for now)
> ------------------------------------------------------
> <snip>
Thanks. I've updated the %install section.
> Some notes:
> - %setup recognizes zip file. "-c" option on setup creates
> the diretory beforehand when unpacking source. Also "-q" option
> is preferred to suppress messages when unpacking source.
I want to pass "-a" option to unzip for converting line endings of the
text files. So that I splitted the %setup and %{__unzip}.
> - When using "install" or "cp" commands, add "-p" option (or
> "-a" for "cp") to keep timestamps on installed files:
> https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
Thanks for the information.
I've added "-p" option.
> - If you want to close macros with {}, please do so consistently
> (i.e. please use %{name})
OK. I've done.
> * Initscripts treatment
> <snip>
> Some notes:
> - Some Requires(post) or so are missing
I've added Requres(post), Requires(preun) and Requires(postun).
> - Why your spec file stopps daemon every time this package is
> upgraded (on %pre)?
> https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Syntax
I wanted to stop the daemon before upgrading and restart it after upgrading.
Now I've commented out '/sbin/service popfile stop'.
> ! Also please check %postun scriptlet example (and the usage of
> condrestart)
I've written %postun script.
> - Please use either "/sbin/service popfile" style or "%{_initrddir}/popfile"
> style consistently
I've updated the scripts to use "/sbin/service" style.
> - Currently using %{_initddir} macro instead of %{_initrddir} is preferred:
>
https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_on_the_filesystem
I've updated the spec file to use %%{_initddir} macro instead of
%%{_initrddir}.
> - rpmlint warns:
> -----------------------------------------------------------------------------
> popfile.noarch: W: service-default-enabled /etc/rc.d/init.d/popfile
> -----------------------------------------------------------------------------
> <snip>
OK. I've updated chkconfig line and deleted Default-Start and Default-Stop
lines.
> * %files
> - Now %defattr(-,root,root,-) is preferred on Fedora.
OK. I've updated %defattr.
> - By the way
> -----------------------------------------------------------------------------
> <snip>
> -----------------------------------------------------------------------------
> can be unified as
> -----------------------------------------------------------------------------
> %files
> %{_datadir}/%{name}/Classifier/
> -----------------------------------------------------------------------------
> <snip>
Thanks for the information.
I've simplified the %files section.
I've uploaded the new SPEC and SRPM files:
Spec URL:
http://getpopfile.org/browser/trunk/linux/fedora/popfile.spec?format=raw
SRPM URL: http://getpopfile.org/downloads/popfile-1.1.1-2.fc11.src.rpm
Naoki
--
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.
More information about the package-review
mailing list