Bug 237883 - Review Request: perl-SGML-Parser-OpenSP - Perl interface to the OpenSP SGML and XML parser
Review Request: perl-SGML-Parser-OpenSP - Perl interface to the OpenSP SGML a...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Chris Weyl
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-25 16:44 EDT by Ville Skyttä
Modified: 2007-11-30 17:12 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-10 02:53:38 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
cweyl: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ville Skyttä 2007-04-25 16:44:31 EDT
http://cachalot.mine.nu/6/SRPMS/perl-SGML-Parser-OpenSP.spec
http://cachalot.mine.nu/6/SRPMS/perl-SGML-Parser-OpenSP-0.99-3.cmn6.src.rpm

SGML::Parser::OpenSP provides a native Perl interface, written in C++
and XS, to the OpenSP SGML and XML parser.

This package is a requirement for an upcoming version of w3c-markup-validator.
Comment 1 Ralf Corsepius 2007-04-26 01:12:51 EDT
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.

Comment 2 Ville Skyttä 2007-04-26 02:58:37 EDT
(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?
Comment 3 Ralf Corsepius 2007-04-26 11:14:05 EDT
(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 ;)

Comment 4 Ville Skyttä 2007-04-26 17:12:43 EDT
(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.
Comment 5 Chris Weyl 2007-04-26 19:41:38 EDT
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.
Comment 6 Ville Skyttä 2007-04-27 02:25:05 EDT
(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.
Comment 7 Ville Skyttä 2007-04-27 02:29:51 EDT
(In reply to comment #6)
> we know %{__perl} is available or the installation will fail

s/installation/build/
Comment 8 Ralf Corsepius 2007-04-27 09:09:56 EDT
(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.
Comment 9 Chris Weyl 2007-05-06 13:23:09 EDT
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
Comment 10 Ville Skyttä 2007-05-06 14:02:39 EDT
(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@iki.fi
Branches: 
InitialCC: fedora-perl-devel-list@redhat.com
Comment 11 Ville Skyttä 2007-05-10 02:53:38 EDT
Imported and built, thanks.

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