Bug 233585 - Review Request: perl-XML-Writer - A simple Perl module for writing XML documents
Review Request: perl-XML-Writer - A simple Perl module for writing XML documents
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-23 07:07 EDT by Alex Lancaster
Modified: 2010-09-05 13:32 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-26 21:56:57 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
panemade: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Alex Lancaster 2007-03-23 07:07:43 EDT
Spec URL: http://allele5.biol.berkeley.edu/~alex/fedora/perl-XML-Writer.spec
SRPM URL: http://allele5.biol.berkeley.edu/~alex/fedora/perl-XML-Writer-0.602-1.src.rpm here>
Description: 
XML::Writer is a simple Perl module for writing XML documents: it
takes care of constructing markup and escaping data correctly, and by
default, it also performs a significant amount of well-formedness
checking on the output, to make certain (for example) that start and
end tags match, that there is exactly one document element, and that
there are not duplicate attribute names.
Comment 1 Parag AN(पराग) 2007-03-23 07:19:40 EDT
Kindly avoid hardcoding version number in Source URL tarball name. Instead use
%{version}
Anyway you can do that at time of importing package in CVS.
Comment 2 Parag AN(पराग) 2007-03-23 07:20:25 EDT
Review:
+ package builds in mock (development i386).
+ rpmlint is silent for SRPM and for RPM.
+ source files match upstream url
c715d6fd90ac775316cc313815ba3b77  XML-Writer-0.602.tar.gz
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written
+ Spec file is written in American English.
+ Spec file is legible.
+ dist tag is present.
+ build root is correct.
+ license is open source-compatible.
+ License text is included in package.
+ %doc is present.
+ BuildRequires are proper.
+ %clean is present.
+ package installed properly.
+ Macro use appears rather consistent.
+ Package contains code, not content.
+ no headers or static libraries.
+ no .pc file present.
+ no -devel subpackage
+ no .la files.
+ no translations are available
+ Does owns the directories it creates.
+ no scriptlets present.
+ no duplicates in %files.
+ file permissions are appropriate.
+ make test
PERL_DL_NONLAZY=1 /usr/bin/perl "-MExtUtils::Command::MM" "-e" "test_harness(0,
'blib/lib', 'blib/arch')" t/*.t
t/01_main.........ok
t/pod-coverage....skipped
        all skipped: Test::Pod::Coverage required for testing pod coverage
t/pod.............skipped
        all skipped: Test::Pod 1.00 required for testing POD
All tests successful, 2 tests skipped.
Files=3, Tests=213,  1 wallclock secs ( 0.30 cusr +  0.03 csys =  0.33 CPU)

+ Provides: perl(XML::Writer) = 0.602 perl(XML::Writer::Namespaces)
perl(XML::Writer::_String)
APPROVED.
Comment 3 Ralf Corsepius 2007-03-23 08:11:08 EDT
Pushing back to review, because of this:

1.)

t/pod-coverage....skipped
        all skipped: Test::Pod::Coverage required for testing pod coverage
t/pod.............skipped
        all skipped: Test::Pod 1.00 required for testing POD

=>

BR: perl(Test::Pod::Coverage)
BR: perl(Test::Pod)


2.
BuildRequires:  perl >= 1:5.6.1
=> Superfluous

3. Missing
BuildRequires: perl(ExtUtils::MakeMaker)

Parag, Alex, these remarks also seem to apply to most other perl-packages Parag
"nodded off" today.
Comment 4 Ville Skyttä 2007-03-23 13:52:55 EDT
On a related note, I don't think testing for coverage of POD documentation is
really that interesting at all from packaging point of view.  If the
documentation coverage test fails, would you block a package from inclusion or
being updated?

So opting to explicity disable the POD coverage tests would IMO be actually even
better than making sure that the associated build dependencies are around and
that the tests are always run (which is acceptable too; the current non-explicit
state of this package is not).  Note: I'm talking about POD coverage tests only
here, not about any other tests.
Comment 6 Parag AN(पराग) 2007-03-26 01:49:54 EDT
(In reply to comment #3)
> Pushing back to review, because of this:
> 
> 1.)
> 
> t/pod-coverage....skipped
>         all skipped: Test::Pod::Coverage required for testing pod coverage
> t/pod.............skipped
>         all skipped: Test::Pod 1.00 required for testing POD
> 
> =>
> 
> BR: perl(Test::Pod::Coverage)
> BR: perl(Test::Pod)
> 
   I thought its not much important as its only check section that said about
skipping testing.
> 
> 2.
> BuildRequires:  perl >= 1:5.6.1
> => Superfluous
> 
  Oops its not needed. Missed one ;)

> 3. Missing
> BuildRequires: perl(ExtUtils::MakeMaker)
    yes
> 
> Parag, Alex, these remarks also seem to apply to most other perl-packages Parag
> "nodded off" today.

Got some spare time to review packages :)
Comment 7 Parag AN(पराग) 2007-03-26 02:20:59 EDT
(In reply to comment #3)
> Pushing back to review, because of this:
> 
> 1.)
> 
> t/pod-coverage....skipped
>         all skipped: Test::Pod::Coverage required for testing pod coverage
> t/pod.............skipped
>         all skipped: Test::Pod 1.00 required for testing POD
> 
> =>
> 
> BR: perl(Test::Pod::Coverage)
> BR: perl(Test::Pod)
> 
> 
> 2.
> BuildRequires:  perl >= 1:5.6.1
> => Superfluous
> 
> 3. Missing
> BuildRequires: perl(ExtUtils::MakeMaker)
> 
Do we have any wiki page that gives above information for perl package
reviewers/packagers?

> Parag, Alex, these remarks also seem to apply to most other perl-packages Parag
> "nodded off" today.

Comment 8 Parag AN(पराग) 2007-03-26 02:26:28 EDT
With new package SRPM
+make test
PERL_DL_NONLAZY=1 /usr/bin/perl "-MExtUtils::Command::MM" "-e" "test_harness(0,
'blib/lib', 'blib/arch')" t/*.t
t/01_main.........ok
t/pod-coverage....ok
t/pod.............ok
All tests successful.
Files=3, Tests=215,  1 wallclock secs ( 0.46 cusr +  0.04 csys =  0.50 CPU)
+ exit 0

All other looks OK to me.
Comment 9 Ralf Corsepius 2007-03-26 02:28:25 EDT
(In reply to comment #4)
> On a related note, I don't think testing for coverage of POD documentation is
> really that interesting at all from packaging point of view. 
Right, but it has always been our policy to enable them, when a perl-dist's test
suite uses them (More precisely: It has been policy to check everything a
test-suite can test for) 

I don't see any reason to make any exceptions.

(In reply to comment #6)
> (In reply to comment #3)
>    I thought its not much important as its only check section that said about
> skipping testing.
Checks aren't "only". We should "test to the max", if possible.

However you are right, Test::Pods aren't very important, but ... c.f. above.

Finally, another remark:
%check ||:

Remove the ||: - This is an anachronism to please "really ancient rpms" and has
been frowned upon for years. Use %check.
Comment 10 Parag AN(पराग) 2007-03-26 02:44:25 EDT
update SPEC and resubmit package removing ||:
Comment 12 Parag AN(पराग) 2007-03-26 04:50:03 EDT
Thanks.
APPROVED Again.
Comment 13 Alex Lancaster 2007-03-26 04:56:22 EDT
New Package CVS Request
=======================
Package Name: perl-XML-Writer
Short Description: A simple Perl module for writing XML documents
Owners: alexl@users.sourceforge.net
Branches: FC-5 FC-6
InitialCC: 

Another orphaned package being resurrected.
Comment 14 Alex Lancaster 2007-03-26 21:56:57 EDT
Builds fine in devel, FC-6, FC-5.  Closing bug.
Comment 15 Xavier Bachelot 2008-04-22 04:16:12 EDT
Hi Alex,

As discussed by mail, I'll take EPEL branches ownership.

Regards,
Xavier

Package Change Request
======================
Package Name: perl-XML-Writer
New Branches: EL-4 EL-5

Updated EPEL Owners: xavierb
Updated EPEL CC: xavierb alexlan
Comment 16 Xavier Bachelot 2008-04-22 04:28:53 EDT
Doh, I didn't add Alex to the owners...
Corrected CVS request below :

Package Change Request
======================
Package Name: perl-XML-Writer
New Branches: EL-4 EL-5

Updated EPEL Owners: xavierb alexlan
Updated EPEL CC: xavierb alexlan
Comment 17 Kevin Fenzi 2008-04-22 13:07:23 EDT
cvs done.
Comment 18 Xavier Bachelot 2008-04-22 15:04:17 EDT
built for EL-4 and EL-5.
Comment 19 Mark Chappell 2010-09-03 04:30:59 EDT
Package Change Request
======================
Package Name: perl-XML-Writer
New Branches: EL-6
Owners: xavierb alexlan tremble iarnell 

Yet another some arches only perl package.

08:26 < tremble> schlobinux_ : Ping - Any objections to perl-XML-Writer getting branched for EL-6, it's yet another some arches only package.
08:27 < schlobinux_> tremble: no objection, indeed.
Comment 20 Kevin Fenzi 2010-09-05 13:32:12 EDT
Git done (by process-git-requests).

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