Package looks good, except for one detail:

# POD Coverage is interesting for upstream, not us.
%{__perl} -pi -e 's|t/99podcov.t||' MANIFEST ; rm t/99podcov.t

It has always been Fedora's policy to "test to the max" and therefore always
been Fedora policy to enable Pod-tests. Also pod-tests are some sort of package
integrity check, which is interesting to us.

I am not wanting to provide a precedence and therefor am not approving this
package because of this.

(In reply to comment #1)
> Also pod-tests
> are some sort of package integrity check, which is interesting to us.

The interesting Pod tests (those done by Test::Pod) are enabled.  I fail to see
why we would be interested in the Pod coverage tests - if they fail, are we
going to not ship a package because some of its Perl functions aren't adequately
documented?

> I am not wanting to provide a precedence and therefor am not approving this
> package because of this.

Quite a few packages disable various tests already, networking related,
Module::Signature related, ones that start daemons etc.  What about the other
way - if I enable the Pod coverage tests, will you review/approve this package?

(In reply to comment #2)
> (In reply to comment #1)
> > Also pod-tests
> > are some sort of package integrity check, which is interesting to us.
> 
> The interesting Pod tests (those done by Test::Pod) are enabled.  I fail to see
> why we would be interested in the Pod coverage tests - if they fail, are we
> going to not ship a package because some of its Perl functions aren't
> adequately documented?
Well, it's an indication that a package is poorly implemented ...

Whether lack of docs shall result into not shipping a package needs to be judged
on a case by case basis. If the core functionalties aren't documented, I don't
see a problem in qualifying a package as "not ready for shipping".

> > I am not wanting to provide a precedence and therefor am not approving this
> > package because of this.
> 
> Quite a few packages disable various tests already,
We also have a number of packages which are failing their tests :(

> networking related,
> Module::Signature related, ones that start daemons etc.
I know, but these actually are limitations of the build-system.

I prefer seeing these packages packaged in a way such tests can be enabled by
rpmbuild options (--with ...) for builts outside of the build-system (It's at
least what I try for my perl-packages.

>  What about the other
> way - if I enable the Pod coverage tests, will you review/approve this
> package?
I probably will ;)

(In reply to comment #3)
> >  What about the other
> > way - if I enable the Pod coverage tests, will you review/approve this
> > package?
> I probably will ;)

If that's what it takes to get this package in, so be it.  Le me know when
you've decided for real, perhaps in the form of conditional "to be done before
the first build" approval; until then I'm leaving the package as is in case
others are willing to take a look.

There's a samples/ directory.  Why not include it in %doc?  For the purposes 
of this review, I added it.

Also...  perl is referred to as %{__perl}, while, e.g., rm isn't %{__rm}.
Doesn't it fit better with the consistentcy guideline to use all one or the
other?  (with the customary exception for the "%{__perl} Makefile.PL ..."
incantation, of course)

Missing BRs on perl(Test::Pod::Coverage) and perl(Test::More) (due to 
perl/perl-devel splittage).  I tend to agree with you -- documentation in and
of itself isn't a _reliable_ quality measurement at all -- but it has been 
customary for perl reviewers to enforce the enabling of this test (unless it 
fails, interestingly enough).  I'd be all for overturning this custom -- 
good/bad/whatever documentation has no bearing on how code actually 
functions -- but I'd prefer to see a discussion on fedora-perl-devel first.

+ source files match upstream:
 cb08669ed566ef4070671cf57aa749e3  SGML-Parser-OpenSP-0.99.tar.gz
 cb08669ed566ef4070671cf57aa749e3  ../SGML-Parser-OpenSP-0.99.tar.gz
+ package meets naming and versioning guidelines.
+ specfile is properly named, is cleanly written and uses macros consistently.
+ dist tag is present.
+ build root is correct.
+ license field matches the actual license.
+ license is open source-compatible.  License text not included upstream.
+ latest version is being packaged.
X BuildRequires are proper.
+ compiler flags are appropriate.
+ %clean is present.
+ package installs properly
+ debuginfo package looks complete.
X rpmlint is silent.
+ final provides and requires are sane:
 ** perl-SGML-Parser-OpenSP-0.99-3.fc6.x86_64.rpm
 == rpmlint
 W: perl-SGML-Parser-OpenSP wrong-file-end-of-line-encoding
 /usr/share/doc/perl-SGML-Parser-OpenSP-0.99/samples/xml.dcl
 W: perl-SGML-Parser-OpenSP wrong-file-end-of-line-encoding
 /usr/share/doc/perl-SGML-Parser-OpenSP-0.99/samples/test.soc
 == provides
 OpenSP.so()(64bit)  
 perl(SGML::Parser::OpenSP) = 0.99
 perl(SGML::Parser::OpenSP::Tools)  
 perl-SGML-Parser-OpenSP = 0.99-0.3.fc6
 == requires
 libc.so.6()(64bit)  
 libc.so.6(GLIBC_2.2.5)(64bit)  
 libgcc_s.so.1()(64bit)  
 libgcc_s.so.1(GCC_3.0)(64bit)  
 libm.so.6()(64bit)  
 libosp.so.5()(64bit)  
 libstdc++.so.6()(64bit)  
 libstdc++.so.6(CXXABI_1.3)(64bit)  
 libstdc++.so.6(GLIBCXX_3.4)(64bit)  
 perl >= 0:5.008
 perl(:MODULE_COMPAT_5.8.8)  
 perl(Carp)  
 perl(Class::Accessor)  
 perl(File::Temp)  
 perl(SGML::Parser::OpenSP::Tools)  
 perl(XSLoader)  
 perl(base)  
 perl(strict)  
 perl(warnings)  
 ** perl-SGML-Parser-OpenSP-debuginfo-0.99-3.fc6.x86_64.rpm
 == rpmlint
 == provides
 OpenSP.so.debug()(64bit)  
 perl-SGML-Parser-OpenSP-debuginfo = 0.99-0.3.fc6
 == requires
+ %check is present and all tests pass:
 All tests successful.
 Files=20, Tests=246,  4 wallclock secs ( 2.31 cusr +  0.76 csys =  3.07 CPU)
+ no shared libraries are added to the regular linker search paths.
+ owns the directories it creates.
+ doesn't own any directories it shouldn't.
+ no duplicates in %files.
+ file permissions are appropriate.
+ no scriptlets present.
+ code, not content.
+ documentation is small, so no -docs subpackage is necessary.
+ %docs are not necessary for the proper functioning of the package.
+ no headers.
+ no pkgconfig files.
+ no libtool .la droppings.
+ not a GUI app.

(In reply to comment #5)
> There's a samples/ directory.  Why not include it in %doc?

I can't imagine what those files would be useful for if included.  They're just
data used by the test suite.  Did I miss something?

> Also...  perl is referred to as %{__perl}, while, e.g., rm isn't %{__rm}.

That's on purpose; it's what the perl (and python and php-pear) spec templates,
cpanspec, and 620+ perl-* packages in Fedora do.  rpmbuild itself uses %{__perl}
for setting the install paths and macroizing it serves a functional purpose of
making it possible to rebuild these packages with a perl other than
/usr/bin/perl.  And we know %{__perl} is available or the installation will fail
anyway, so why not use it for the in-place edits as well (+ for consistency)? 
It's quite hard to come up with a use case where plain "rm" in $PATH wouldn't
work (ie. wouldn't grok "-f" or "-rf").  So I think this is a valid example of
consistently using the non-macroized "base" case in which certain commands that
are really useful to have/use macroized are done so.

> Missing BRs on perl(Test::Pod::Coverage) and perl(Test::More) (due to 
> perl/perl-devel splittage).

As discussed, perl(Test::Pod::Coverage) is intentionally missing. 
perl(Test::More) is pulled in by perl-Test-Pod and perl-Test-Exception (through
perl(Test::Builder) being in the same post-perl-split perl-Test-Simple package
as perl(Test::More)).  Even if this is not quite as explicit as it could be, I
don't see this becoming a problem.  Will of course fix if it does.

> but I'd prefer to see a discussion on fedora-perl-devel first.

Please do discuss (or let me know if you'd approve this right now if the pod
coverage tests would be run).  BTW, in addition to Pod coverage tests IMO being
of very doubtful usefulness in packaging, perl-Test-Pod-Coverage and
perl-Pod-Coverage are not found packaged as often as perl-Test-Pod; I think
that's not a coincidence.  For example, EPEL doesn't have them, so rebuilding
this package (assuming the Test::Pod::Coverage BR in place) for it would have to
jump through extra hoops of packaging the coverage things first for IMO no gain.

(In reply to comment #6)
> we know %{__perl} is available or the installation will fail

s/installation/build/

(In reply to comment #6)

> > Missing BRs on perl(Test::Pod::Coverage) and perl(Test::More) (due to 
> > perl/perl-devel splittage).
> 
> As discussed, perl(Test::Pod::Coverage) is intentionally missing. 
As discussed, I will not approve this package exactly because of this, because I
am not wanting produce a precendence for to undermine established practice for
many years.

> perl(Test::More) is pulled in by perl-Test-Pod
A random cooincidence.

> > but I'd prefer to see a discussion on fedora-perl-devel first.
> 
> Please do discuss (or let me know if you'd approve this right now if the pod
> coverage tests would be run).  BTW, in addition to Pod coverage tests IMO being
> of very doubtful usefulness in packaging, perl-Test-Pod-Coverage and
> perl-Pod-Coverage are not found packaged as often as perl-Test-Pod;

Whether you like it or not, they nevertheless are widely used packages.

> I think
> that's not a coincidence.  For example, EPEL doesn't have them,
Sorry, but this argument is irrelevant because this (1.) is Fedora and is not
EPEL, and (2.) isn't an argument at all, because RHEL lacks a lot of packages
which are present in Fedora.

You should include perl(Test::More) as an explicit BR -- I'm not seeing it being
pulled in by perl-Test-Pod (on my system at least).  Taking care of the rpmlint
warnings is trivial in %prep -- sed -i 's/\r//' samples/* -- so this should be
taken care of as well.

If Test::Pod::Coverage is available on the distros you're building for, its use
is strongly encouraged as a matter of form, but I'm not insisting on it for
reasons covered several times here and in other places :)

APPROVED

(In reply to comment #9)
> You should include perl(Test::More) as an explicit BR -- I'm not seeing it 
> being pulled in by perl-Test-Pod (on my system at least).

It should; see comment 6 for description of the dep chain.  But I don't see it
causing any harm and it's directly required, so will add.

> Taking care of the rpmlint
> warnings is trivial in %prep -- sed -i 's/\r//' samples/* -- so this should be
> taken care of as well.

The samples are not included, see comment 6 for the rationale.  If you can think
of where they could be useful when installed, let me know.

Thanks for the review!

New Package CVS Request
=======================
Package Name: perl-SGML-Parser-OpenSP
Short Description: Perl interface to the OpenSP SGML and XML parser
Owners: ville.skytta
Branches: 
InitialCC: fedora-perl-devel-list

Imported and built, thanks.