Bug 166251 - Review Request: perl-Apache-LogRegex
Review Request: perl-Apache-LogRegex
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jef Spaleta
Fedora Package Reviews List
http://search.cpan.org/dist/Apache-Lo...
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-18 07:30 EDT by Gavin Henry
Modified: 2007-11-30 17:11 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-05-08 00:29:02 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Gavin Henry 2005-08-18 07:30:49 EDT
Spec Url: http://www.perl.me.uk/downloads/modules/perl-Apache-LogRegex.spec

SRPM Url: http://www.perl.me.uk/downloads/modules/perl-Apache-LogRegex-1.2-2.src.rpm

Description: Designed as a simple class to parse Apache log files. 
It will construct a regex that will parse the given log 
file format and can then parse lines from the log file 
line by line returning a hash of each line.
Comment 1 Ralf Corsepius 2005-08-18 10:26:43 EDT
Not a formal review, just some comments/questions on the spec:

* BuildRequires:  perl >= 2:5.8.0
Whatfor? 
"BR: perl" makes at least some sense, but ">= 2:5.8.0" seems a bit bizarre to me.

* %check || :
Plain "%check" is sufficient.
Comment 2 Gavin Henry 2005-08-18 12:03:38 EDT
Sorry the Perl version was left over from other stuff.

I'll change the check.

Gavin.
Comment 3 Jef Spaleta 2005-08-19 01:46:58 EDT
Other than what ralf has already pointed out....

GOOD:
rpmlint returns clean
packagename conforms to nameguideline
noarch is appropriate
specfile matches name
spec in legible english
license tag matches license
upstrem md5sum matches Sources in srpm 268f87285fbfb5b7b811e4d779e7835c
owns all created directories
clean section present

BAD:
- not providing the license text for Artistic and GPL licenses in the doc
section. you could probably crib the licenses from another perl package such as
perl-Carp-Clan from core and include them as additional sources in your spec
file.  Providing the license text is a stated requirement as per:
http://www.fedoraproject.org/wiki/PackageReviewGuidelines

- Shouldn't be including perl as a BuildRequires at all as per:
http://www.fedoraproject.org/wiki/PackagingGuidelines#Exceptions

I think thats basically it. It builds on my local rawhide box but I'm having
trouble with mock at the moment on that box so I haven't built it in a clean
mock environment yet.  If you can roll another spec that takes care of the
%check and the tho issues I've raised. I think this is in good shape for
approval. I still want to rebuild in mock before the i give final approval for
completeness.

-jef
Comment 4 Gavin Henry 2005-08-19 05:40:43 EDT
I can't remember how to get an extra file in the doc section. Here's the latest
specfile:

Name:           perl-Apache-LogRegex
Version:        1.2
Release:        3%{?dist}
Summary:        Parse a line from an Apache logfile into a hash

Group:          Development/Libraries
License:        GPL or Artistic
URL:            http://search.cpan.org/dist/Apache-LogRegex/
Source0:       
http://search.cpan.org/CPAN/authors/id/P/PE/PETERHI/Apache-LogRegex-%{version}.tar.gz
Source1:        GNU_GPL.txt
BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

BuildArch:      noarch
Requires:       perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))

%description
Designed as a simple class to parse Apache log files. 
It will construct a regex that will parse the given log 
file format and can then parse lines from the log file 
line by line returning a hash of each line.

%prep
%setup -q -n Apache-LogRegex-%{version}


%build
%{__perl} Makefile.PL INSTALLDIRS=vendor
make %{?_smp_mflags}


%install
rm -rf $RPM_BUILD_ROOT
make pure_install PERL_INSTALL_ROOT=$RPM_BUILD_ROOT
find $RPM_BUILD_ROOT -type f -name .packlist -exec rm -f {} ';'
find $RPM_BUILD_ROOT -type d -depth -exec rmdir {} 2>/dev/null ';'
chmod -R u+w $RPM_BUILD_ROOT/*


%check
make test


%clean
rm -rf $RPM_BUILD_ROOT


%files
%defattr(-,root,root,-)
%doc README GNU_GPL.txt
%{perl_vendorlib}/Apache/
%{_mandir}/man3/*.3*

%changelog
* Fri Aug 19 2005 Gavin Henry <ghenry[AT]suretecsystems.com> - 1.2-3
- Cleaning up specifle.

* Thu Aug 18 2005 Gavin Henry <ghenry[AT]suretecsystems.com> - 1.2-2
- Second build.

* Sun Jul 3 2005 Gavin Henry <ghenry[AT]suretecsystems.com> - 1.2-1
- First build.
Comment 5 Paul Howarth 2005-08-19 06:15:37 EDT
I'd suggest removing the Source1 reference and instead:

Add to %setup:
perldoc -t perlartistic > Artistic.txt
perldoc -t perlgpl      > GPL.txt

Add to %files:
%doc Artistic.txt GPL.txt
Comment 6 Michael Schwendt 2005-08-19 06:16:58 EDT
[independent of Paul's comment]

In %install

  cp %{SOURCE1} .

copies the extra source file into the current build directory,
then "%doc GNU_GPL.txt" in %files section finds it there.

The current build directory you start in at the beginning of the
spec sections (%build, %install, %check) is the directory below
$RPM_BUILD_DIR which is created with the %setup macro. Here it's.
$RPM_BUILD_DIR/Apache-LogRegex-%{version}/
Comment 8 Jef Spaleta 2005-08-19 09:03:29 EDT
(In reply to comment #5)
> I'd suggest removing the Source1 reference and instead:
> 
> Add to %setup:
> perldoc -t perlartistic > Artistic.txt
> perldoc -t perlgpl      > GPL.txt
> 
> Add to %files:
> %doc Artistic.txt GPL.txt

that sir.. is slick.  If the requirement for the full license text sticks around
after the latest round of discussion in the extras-list is over... I'd say this
approach should be documented for future submitters who run into this license
text issue.  Since perldoc is part of the perl package this doesn't add any
requirements at all.

-jef
Comment 9 Jef Spaleta 2005-08-19 09:38:38 EDT
oops... i accidently closed this as notabug.. sorry about that... must have been
a fat finger incident.
Comment 10 Gavin Henry 2005-08-19 09:43:43 EDT
I never knew you could do that with perldoc either.

Very nice.
Comment 11 Jef Spaleta 2005-08-19 10:08:05 EDT
http://www.perl.me.uk/downloads/modules/perl-Apache-LogRegex-1.2-3.src.rpm
builds in mock for fc4 just fine.
rpmlint returns clean on the resulting binary
the relevant md5sums match and the new spec file takes care of the issues raised
so far.

I think this is good to go. I'm going to leave this ticket in review state for
another 8 hours or so and will flip the approved switch at the end of the
business day if there isn't another blocker comment in the meantime.


-jef
Comment 12 Ville Skyttä 2005-08-19 10:33:55 EDT
The perldoc trick is indeed a very useful one.  Thanks!  Two nitpicks, though:  
  
The -T option seems more appropriate than -t even though it doesn't seem to  
matter when the output is directed to a file.  
  
Second, I'd suggest using the "standard" filenames of "COPYING" for the GPL 
and "Artistic" for the Artistic License.  IIRC a statement like "see the file 
COPYING" was included in some revisions of the GPL, in the blurb that it 
suggests adding to source files. 
Comment 13 Paul Howarth 2005-08-19 10:43:48 EDT
(In reply to comment #12)
> The perldoc trick is indeed a very useful one.  Thanks!  Two nitpicks, though:  
>   
> The -T option seems more appropriate than -t even though it doesn't seem to  
> matter when the output is directed to a file.

It makes a difference here: "perldoc -T perlgpl > file" results in a file with
old-fashioned highlighting (visible in vim) such as:

N^HNA^HAM^HME^HE
       perlgpl - the GNU General Public License, version 2

This doesn't happen with -t
 
> Second, I'd suggest using the "standard" filenames of "COPYING" for the GPL 
> and "Artistic" for the Artistic License.  IIRC a statement like "see the file 
> COPYING" was included in some revisions of the GPL, in the blurb that it 
> suggests adding to source files. 

Agreed.
Comment 14 Ville Skyttä 2005-08-19 11:07:17 EDT
Duh, right, that's what you get by going handwaving after hastily (mis)reading 
a man page and not actually testing :þ.  Thanks for the sanity check.  (Could 
still use -tT, though ;)) 
Comment 15 Jef Spaleta 2005-08-19 18:03:25 EDT
Okay I'm going to go ahead and approve this.  You can import this into cvs. Just
make sure you change the license filenames as suggested in comment 12,
"COPYING" for the GPL and "Artistic" for the Artistic License, before you do a
build request.

-jef
Comment 16 Gavin Henry 2005-08-20 17:57:39 EDT
Tried to import it, but my network connection got pulled half way through, and
by the time I put it back in and entered my SSH key pass, the remote connection
timed out.

Could someone delete the half created/uploaded module?

Thanks.
Comment 17 Michael Schwendt 2005-08-20 20:10:08 EDT
Import again.
Comment 18 Christian Iseli 2006-03-28 10:29:01 EST
Looks like this package is now imported and built.  Please close this ticket,
unless there is a reason to leave it open...

Note You need to log in before you can comment on or make changes to this bug.