Bug 525786

Hi,

I've forgotten to mention that this is my first package and I need a sponsor.

Some random commends:

* 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.
    Moreover:
--------------------------------------------------------
# Copyright (c) 2001-2009 John Graham-Cumming
#   This file is part of POPFile
--------------------------------------------------------
    - Why is the copyright holder of your spec file not you?
    - Is this spec file really "a part of POPFile"?

* 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

* Vendor
  - Vendor tag must be removed on Fedora. This tag is automatically imported
    on Fedora build server.

* Exclusiveos
  - I don't think you have to write this explicitly.

* Perl related packaging treatment
  https://fedoraproject.org/wiki/Packaging/Perl
  Some notes:
  - "Requires: perl >= 5.8.1" does almost nothing because Fedora's perl
    rpm has epoch:
------------------------------------------------------
$ rpm -q --qf '%{epoch}:%{name}-%{version}-%{release}\n' perl
4:perl-5.10.0-82.fc12
------------------------------------------------------
  - And anyway I don't think this is needed because even Fedora 10
    (the oldest branch currently supported on Fedora) uses perl 5.10.

  - For perl module dependency, please write virtual Provides names
    instead of rpm names directly:
    https://fedoraproject.org/wiki/Packaging/Perl#Perl_Requires_and_Provides

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

* %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)
------------------------------------------------------
%prep
%setup -q -c
find . -type f | xargs chmod 0644
%{__cp} -p %{SOURCE1} .
%{__cp} -p %{SOURCE2} .

%build

%install
%{__rm} -rf $RPM_BUILD_ROOT

%{__mkdir_p} $RPM_BUILD_ROOT%{_datadir}/%{name}

%{__cp} -a * $RPM_BUILD_ROOT%{_datadir}/%{name}/
%{__rm} -f $RPM_BUILD_ROOT%{_datadir}/%{name}/popfile

%{__mkdir_p} $RPM_BUILD_ROOT%{_localstatedir}/lib/%{name}
%{__install} -p -m 644 stopwords $RPM_BUILD_ROOT%{_localstatedir}/lib/%{name}

%{__mkdir_p} $RPM_BUILD_ROOT%{_localstatedir}/log/%{name}

%{__mkdir_p} $RPM_BUILD_ROOT%{_initrddir}
%{__install} -p -m 755 popfile $RPM_BUILD_ROOT%{_initrddir}/popfile

%{__install} -p -m 755 start_popfile.sh $RPM_BUILD_ROOT%{_datadir}/%{name}/
------------------------------------------------------
  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.

    - 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

    - If you want to close macros with {}, please do so consistently
      (i.e. please use %{name})

* Initscripts treatment

  Please refer to:
  https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets

  Some notes:
  - Some Requires(post) or so are missing
  - Why your spec file stopps daemon every time this package is
    upgraded (on %pre)?
    https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Syntax
    ! Also please check %postun scriptlet example (and the usage of
      condrestart)
  - Please use either "/sbin/service popfile" style or "%{_initrddir}/popfile"
    style consistently
  - Currently using %{_initddir} macro instead of %{_initrddir} is preferred:
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_on_the_filesystem
  - rpmlint warns:
-----------------------------------------------------------------------------
popfile.noarch: W: service-default-enabled /etc/rc.d/init.d/popfile
-----------------------------------------------------------------------------
    Usually service should be enabled by admin manually afterwards
    and should not be enabled by default.
    So popfile initscript should have
-----------------------------------------------------------------------------
# chkconfig: - 80 20
-----------------------------------------------------------------------------
    And "Default-Start", "Default-Stop" lines should be removed
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#.23_chkconfig:_line
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#.23_Default-Start:_line

* %files
  - Now %defattr(-,root,root,-) is preferred on Fedora.
  - By the way
-----------------------------------------------------------------------------
%files
%dir %{_datadir}/%{name}/Classifier
%{_datadir}/%{name}/Classifier/*
-----------------------------------------------------------------------------
    can be unified as
-----------------------------------------------------------------------------
%files
%{_datadir}/%{name}/Classifier/
-----------------------------------------------------------------------------
    The latter %files entry contains both the directory 
    %{_datadir}/%{name}/Classifier itself and all files/directories/etc
    under the directory.

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

Almost okay.

* Documents directory
  - Usually documents (like "license" text) should be installed
    under %{_defaultdocdir}/%{name}-%{version}.

    If this package really wants document files to be unstalled
    under %{_datadir}/%{name}, it can be accepted. Otherwise
    please consider to move them to the directory used on Fedora
    by default.

  ! Note
-------------------------------------------------------------
%files
%doc license
-------------------------------------------------------------
    will do this automatically.

* Empty scriptlets
  - Please remove %pre stage completely. Currently %pre stage
    essentially does nothing, however leaving this calls
    unneeded shell process (however Fedora suggests to leave
    %build stage even if this is empty)

* logrotate file
  - rpmlint says:
-------------------------------------------------------------
popfile.noarch: W: log-files-without-logrotate /var/log/popfile
-------------------------------------------------------------
    Please consider to create logrotate file (not a blocker).

Now:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on my wiki page:
http://fedoraproject.org/wiki/User:Mtasaka#B._Review_request_tickets
(Check "No one is reviewing")

Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

Thanks for another comments.

> Almost okay.

Thank you for reviewing.

> * Documents directory
>   - Usually documents (like "license" text) should be installed
>     under %{_defaultdocdir}/%{name}-%{version}.

OK. I've moved three document files (license and two changelogs) to
the appropriate directory.

>   ! Note
> -------------------------------------------------------------
> %files
> %doc license
> -------------------------------------------------------------
>     will do this automatically.

Thanks for the information.

> * Empty scriptlets
>   - Please remove %pre stage completely. Currently %pre stage
>     essentially does nothing, however leaving this calls
>     unneeded shell process (however Fedora suggests to leave
>     %build stage even if this is empty)

OK. Done.

> * logrotate file
>   - rpmlint says:
> -------------------------------------------------------------
> popfile.noarch: W: log-files-without-logrotate /var/log/popfile
> -------------------------------------------------------------
>     Please consider to create logrotate file (not a blocker).

POPFile has its own log rotation feature.
It checks log directory per hour and removes log file which is older
than three days before.

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-3.fc11.src.rpm

And here's the full changes between release 2 and 3:
http://getpopfile.org/changeset/3620/trunk/linux/fedora/popfile.spec

Naoki

I appreciate your thoughtful message.

> Usually there are two ways to show this.
> A. submit other review requests with enough quality.
> B. Do a "pre-review" of other person's review request
>    (at the time you are not sponsored, you cannot do
>    a formal review)

OK. I'll try to pre-review someone's review request.

> When you have submitted a new review request or have pre-reviewed other 
> person's review request, please write the bug number on this bug report 
> so that I can check your comments or review request.

I see. I'll do so.

Naoki

I've done my first pre-review:
https://bugzilla.redhat.com/show_bug.cgi?id=522920

And I've built SRPM on koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1716333

BTW,

> http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored#Sponsorship_model
> If you are an upstream author of the package you are submitting, or if you
> are active in the community that surrounds it, please say so.

I am a member of upstream's maintenance team (called POPFile Core Team):
http://getpopfile.org/docs/Glossary:POPFileCoreTeam

And as for my recent activities in the upstream project, please see:

(check-ins to the repository (naoki is me))
http://getpopfile.org/log/branches/b0_22_2/engine

(assigned or closed tickets owned by me)
http://getpopfile.org/query?status=assigned&status=closed&owner=amatubu&order=priority

Naoki

Okay.

--------------------------------------------------------
   This package (popfile) is APPROVED by mtasaka
--------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Install the Client Tools (Koji)".

Now I am sponsoring you.

If you want to import this package into Fedora 10/11/12, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Removing NEEDSPONSOR.

(In reply to comment #9)
> --------------------------------------------------------
>    This package (popfile) is APPROVED by mtasaka
> --------------------------------------------------------

Thanks.

> Please follow the procedure written on:
> http://fedoraproject.org/wiki/PackageMaintainers/Join
> from "Install the Client Tools (Koji)".

OK.
I've built my SRPM using koji:

Fedora 10
http://koji.fedoraproject.org/koji/taskinfo?taskID=1717407

Fedora 11
http://koji.fedoraproject.org/koji/taskinfo?taskID=1717412

Fedora 12(same as above)
http://koji.fedoraproject.org/koji/taskinfo?taskID=1716333

Next, I'm going to do CVS admin request.

> Now I am sponsoring you.

Thank you.
I'm glad to join the package maintainer!

> If you want to import this package into Fedora 10/11/12, you also have
> to look at
> http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
> (after once you rebuilt this package on koji Fedora rebuilding system).

OK. I'll read the document.

> If you have questions, please ask me.

I appreciate you.

Naoki

New Package CVS Request
=======================
Package Name: popfile
Short Description: Automatic Email Classification
Owners: amatubu
Branches: F-10 F-11 F-12
InitialCC:

cvs done.

popfile-1.1.1-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/popfile-1.1.1-3.fc10

popfile-1.1.1-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/popfile-1.1.1-3.fc11

Closing.

Note that currently bodhi has not been set up to accept F-12 updates
request. When bodhi begins to do so (a notification will be sent
to fedora-devel-list ), please submit updates request for F-12.

Now bodhi began to accept F-12 updates request, so please submit
it.

popfile-1.1.1-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/popfile-1.1.1-3.fc12

(In reply to comment #16)
> Now bodhi began to accept F-12 updates request, so please submit
> it.  

Thanks. I've submitted update request.

Naoki

popfile-1.1.1-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

popfile-1.1.1-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.