[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 #2 from Mamoru Tasaka <mtasaka at ioa.s.u-tokyo.ac.jp>  2009-09-27 13:15:19 EDT ---
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.

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