Bug 435835

Summary: Review Request: perl-RPC-XML - Set of classes for core data, message and XML handling
Product: [Fedora] Fedora Reporter: Nicholas Boyle <nsboyle>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: cweyl, fedora-package-review, notting, opensource, thomas.moschny
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-08-21 15:24:19 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 201449, 454102    

Description Nicholas Boyle 2008-03-03 22:38:21 EST
Spec URL: http://nboyle.fedorapeople.org/perl-RPC-XML.spec
SRPM URL: http://nboyle.fedorapeople.org/perl-RPC-XML-0.59-2.fc8.src.rpm
Description: The RPC::XML package is an implementation of XML-RPC. The module provides classes for sample client and server implementations, a server designed as an Apache location-handler, and a suite of data-manipulation classes that are  used by them.
Comment 1 Nicholas Boyle 2008-03-03 22:42:05 EST
FYI - License is as follows:

#####################################################################
Copying and distribution are permitted under the terms of the Artistic
License as distributed with Perl versions 5.005 and later. See
http://www.opensource.org/licenses/artistic-license.php
#####################################################################

I'm interpreting the "and later" to mean Artistic 2.0, as opposed to 1.0, the
latter of which is unacceptable as per the guidelines.  If I'm incorrect in my
legalese, let me know, and I'll ping upstream to clarify license, and/or re-license.
Comment 2 Jason Tibbitts 2008-03-03 23:22:33 EST
I think "and later" refers to the perl version.  It would definitely be good to
get clarification from the upstream developer.  It's sufficient to simply get an
email from them which you include in the package.
Comment 3 Nicholas Boyle 2008-03-07 16:05:49 EST
Alright I got license clarification from upstream.

New files:
Spec URL: http://nboyle.fedorapeople.org/perl-RPC-XML.spec
SRPM URL: http://nboyle.fedorapeople.org/perl-RPC-XML-0.59-3.fc8.src.rpm
Comment 4 Chris Weyl 2008-03-14 15:42:32 EDT
A couple thoughts prior to a full-fledged review:

The %perl_vendorarch and %perl_vendorlib macros have been defined by the
system since...  oh, about RHEL3 or so.  They should probably be dropped
unless you're explicitly keeping them in there to better deal with old
systems; in that case they should be conditionalized, however.

Using macros a la %{__rm} is legit, and you use them cleanly and consistently,
but they're still a bit odd in Fedora.  I wouldn't consider it a blocker,
however.  :)

Your buildroot is slightly wonky; Fedora generally uses
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

You have the man pages marked %doc...  AFAIK convention is to not mark these as
such, though I'm not entirely sure convention is correct in this case.

The Apache::RPC::Server/::Status modules pull in mod_perl, which in turn has a
slew of requirements of its own (not the least of which is apache).  I'd break
them out into a subpackage to avoid a rather large potentially unwanted
dependency tree.

Your buildrequires are incomplete:  judging by Makefile.PL there are a couple
more you're going to want to throw in there.  Note also that it's key to start
including core modules such as Test::More if so required.

It's often helpful to kick off a koji scratch build and post the link to the
task results in the bug review (a relatively recent development).  It's also a
good self-sanity check to ensure the package builds as intended.
Comment 5 Nicholas Boyle 2008-03-16 17:04:02 EDT
(In reply to comment #4)
> The %perl_vendorarch and %perl_vendorlib macros have been defined by the
> system since...  oh, about RHEL3 or so.  They should probably be dropped

Done.

> Your buildroot is slightly wonky; Fedora generally uses
> %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

The method I'm using is preferred as per the wiki; see:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-b4fdd45fa76cbf54c885ef0836361319ab962473

> You have the man pages marked %doc...  AFAIK convention is to not mark these as
> such, though I'm not entirely sure convention is correct in this case.

You're right, fixed

> The Apache::RPC::Server/::Status modules pull in mod_perl, which in turn has a
> slew of requirements of its own (not the least of which is apache).  I'd break
> them out into a subpackage to avoid a rather large potentially unwanted
> dependency tree.

Agreed... I wasn't sure of an appropriate name, so for now it's perl-Apache-RPC.
   If there's another way to name it that makes more sense, let me know! :)

> Your buildrequires are incomplete:  judging by Makefile.PL there are a couple
> more you're going to want to throw in there.  Note also that it's key to start
> including core modules such as Test::More if so required.

There is a warning about a missing XML::LibXML, but only to inform the user this
will be required under RPC-XML v0.60.  Other modules required in Makefile.PL,
that aren't in the spec, are part of the core perl package  (ex: Cwd, and
File::Find).

> It's often helpful to kick off a koji scratch build and post the link to the
> task results in the bug review (a relatively recent development).  It's also a
> good self-sanity check to ensure the package builds as intended.

This mock builds on 8 and devel i386 for me, without issue.  Additionally, it
scratch built in Koji for me:

F8: http://koji.fedoraproject.org/koji/taskinfo?taskID=518717
F9: http://koji.fedoraproject.org/koji/taskinfo?taskID=518727

Is there something specific you noticed in building that I'm overlooking?

At any rate, the package/subpackage version is now available here:
Spec URL: http://nboyle.fedorapeople.org/perl-RPC-XML.spec
SRPM URL: http://nboyle.fedorapeople.org/perl-RPC-XML-0.59-4.fc8.src.rpm

I've never really worked with the whole subpackage business, so please look over
that with extra scrutiny ;)

Comment 6 Jason Tibbitts 2008-05-10 23:06:52 EDT
Some additional comments:

There's a test suite included; any reason why it's not being run?  Of course,
when you do run it, you'll find out why Chris was telling you about missing
build dependencies, because most of the tests will fail without Test::More and
XML::Parser.  Note that pretty much any reviewer will consider it a blocker to
fail to run the test suite without a good reason.

Also, is there any particular reason why you didn't just use cpanspec to
generate the package?  While there's nothing specifically wrong with rolling
your own by hand, it's a good but more difficult to review since it doesn't look
anything like essentially all of the other Perl packages in the distro.
Comment 7 Nicholas Boyle 2008-06-05 22:06:30 EDT
Sorry for the long delay in getting these issues fixed:

New version:
Spec URL: http://nboyle.fedorapeople.org/perl-RPC-XML.spec
SRPM URL: http://nboyle.fedorapeople.org/perl-RPC-XML-0.59-5.fc8.src.rpm

(In reply to comment #6) 
> There's a test suite included; any reason why it's not being run?  Of course,
> when you do run it, you'll find out why Chris was telling you about missing
> build dependencies, because most of the tests will fail without Test::More and
> XML::Parser.  Note that pretty much any reviewer will consider it a blocker to
> fail to run the test suite without a good reason.

Chalk this up to my inexperience with packaging Perl modules.  I didn't realize
this module had a unit tests with it... I've since added both missing build
dependencies.

> Also, is there any particular reason why you didn't just use cpanspec to
> generate the package?  While there's nothing specifically wrong with rolling
> your own by hand, it's a good but more difficult to review since it doesn't look
> anything like essentially all of the other Perl packages in the distro.

Again, inexperience (you've got to start somewhere though, right?).  I wasn't
even aware of such a tool.  I had originally adopted this spec from Dries,
although admittedly by this point it's almost a complete rewrite.  My thoughts
are that at this point, it's best to just leave it as-is and keep this tool in
mind for future packagings.  If it still makes more sense to redo it using
cpanspec, I'll gladly do that.  Let me know.
Comment 8 Till Maas 2008-06-16 18:23:11 EDT
two minor issues:

- imho you should add "-p" to the %{__cp} of Source1 to preserve the timestamp
- You should add README.license to %doc of the main package, otherwise there is
no need to cp it ;-)

maybe one major issue:

- I guess you need to add the testsuite to %check in the spec, but I do not
really know anything, because I did not read anything about testsuites in any
Fedora guideline, but only in some reviews that there was no need for %check,
because there was no testsuite.
Comment 9 Chris Weyl 2008-07-03 19:22:31 EDT
(In reply to comment #8)
> - I guess you need to add the testsuite to %check in the spec, but I do not
> really know anything, because I did not read anything about testsuites in any
> Fedora guideline, but only in some reviews that there was no need for %check,
> because there was no testsuite.

Test suites _must_ be run for perl packages; see:

http://fedoraproject.org/wiki/Packaging/Perl#Testing_and_Test_Suites

I like the splitting of the Apache bits off into their own sub-package :)

The %files section should be amended so the correct package owns
%{perl_vendorlib}/Apache and %{perl_vendorlib}/RPC.

See http://fedoraproject.org/wiki/Packaging/Perl#Directory_Ownership

Your license shortname is off, and missing the LGPL :)  See
http://fedoraproject.org/wiki/Licensing#Good_Licenses

There appear to be some samples/docs under the etc/, ex/ and methods/
directories.  It may be worthwhile to include them in %doc.

Do these things, and (from my cursory scan of the spec) I believe this package
will be ready for a review :)
Comment 10 Chris Weyl 2008-07-04 11:48:32 EDT
...and 0.60 was released on 09 Apr 2008.
Comment 11 Thomas Moschny 2008-08-07 04:36:45 EDT
Ping? What's the status of this package?
Comment 12 Jason Tibbitts 2008-08-10 14:34:04 EDT
Last comment from the submitter was over two months ago.  Setting NEEDINFO; I will close this ticket soon if there is no response.
Comment 13 Jason Tibbitts 2008-08-21 15:24:19 EDT
Closing.