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.

Sorry the Perl version was left over from other stuff.

I'll change the check.

Gavin.

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

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.

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

[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}/

I have done it Pauls way:

New files:
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-3.src.rpm

md5sums:
http://www.perl.me.uk/downloads/modules/md5sums

(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

oops... i accidently closed this as notabug.. sorry about that... must have been
a fat finger incident.

I never knew you could do that with perldoc either.

Very nice.

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

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. 

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

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 ;)) 

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

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.

Import again.

Looks like this package is now imported and built.  Please close this ticket,
unless there is a reason to leave it open...