[Bug 525786] Review Request: popfile - Automatic Email Classification

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