Spec Name or Url: http://people.redhat.com/~jvdias/perl-RPM2.spec

A few comments after a cursory look:

- I can't find any evidence of license terms under which this package can be
distributed, which would be a blocker and in need of fixing upstream
- Extras packages shouldn't have a buildreq on perl
- latest changelog release tag doesn't match actual release tag
- some of the tests fail but "make check" passes - why?
- missing buildreqs elfutils-libelf-devel and bzip2-devel
- using find/filelist method for %files results in unowned directory
%{perl_vendorarch}/auto/RPM2
- consider adding %{?dist} tag if this package is likely to be needed for future
Fedora releases

OK, I've resolved each of the issues raised:

>  - I can't find any evidence of license terms under which this package can be
>  distributed, which would be a blocker and in need of fixing upstream

The rpm has always had tag:
'License: distributable
'
and there are no license restrictions or copyright in any source file 
or on the package CPAN home-page. As perl-RPM2 is already distributed
in every Fedora Core and  RHEL release except FC-5, these terms are 
evidently acceptable. 

> - Extras packages shouldn't have a buildreq on perl

This package cannot be built without the perl package being
installed; hence, the 'BuildRequires: perl' tag.

> - latest changelog release tag doesn't match actual release tag

huh? The %{version}-%{release} is 0.66-12 - latest changelog:
* Thu Mar 08 2006 Jason Vas Dias <jvdias> - 0.66-12

> - some of the tests fail but "make check" passes - why?

Because the package was not being built as root. 

Now, all tests that require root access are skipped if being run by a non-root
user, so all tests pass as a non-root user; I've also tested that those tests
are run and pass if run by root.

> - missing buildreqs elfutils-libelf-devel and bzip2-devel

Added.

> - using find/filelist method for %files results in unowned directory
> %{perl_vendorarch}/auto/RPM2

I've added:
%dir %{perl_vendorarch}/auto/RPM2

to %files list.

>- consider adding %{?dist} tag if this package is likely to be needed for 
>  future Fedora releases

Added.

Modified .spec file and srpm at:
  http://people.redhat.com/~jvdias/perl-RPM2.spec
  http://people.redhat.com/~jvdias/perl-RPM2-0.66-12.src.rpm

(In reply to comment #3)
> OK, I've resolved each of the issues raised:
> 
> >  - I can't find any evidence of license terms under which this package can be
> >  distributed, which would be a blocker and in need of fixing upstream
> 
> The rpm has always had tag:
> 'License: distributable
> '
> and there are no license restrictions or copyright in any source file 
> or on the package CPAN home-page. As perl-RPM2 is already distributed
> in every Fedora Core and  RHEL release except FC-5, these terms are 
> evidently acceptable.

Put it this way; if this package had not been included in previous Core/RHEL
releases, it would definitely not be allowed into Extras without a clear
license. There is a similar issue affecting Bug 171640
(perl-Log-Dispatch-FileRotate). Given that packages migrating from Core to
Extras must go through the same review process as any other package, I don't
think this issue can waived so lightly.

What I think needs to happen is one of:
(a) Upstream clarifying the license, or
(b) Legal stating that this particular package is OK (and, preferably, why), or
(c) Legal stating that a class of packages into which this one falls (e.g.
packages from CPAN, packages migrating from Core) are OK to be included without
a clear license; this could be beneficial for perl-Log-Dispatch-FileRotate too.

> > - Extras packages shouldn't have a buildreq on perl
> 
> This package cannot be built without the perl package being
> installed; hence, the 'BuildRequires: perl' tag.

Perl is explicitly listed as one of the exclusions from BuildRequires in the
packaging guidelines at: http://fedoraproject.org/wiki/Packaging/Guidelines and
the package review guidelines at
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines state that:

  MUST: A package must not contain any BuildRequires that are listed in
  the exceptions section of Packaging Guidelines.

> > - latest changelog release tag doesn't match actual release tag
> 
> huh? The %{version}-%{release} is 0.66-12 - latest changelog:
> * Thu Mar 08 2006 Jason Vas Dias <jvdias> - 0.66-12

Not in the original SRPM, which had:
* Fri Feb 03 2006 Jason Vas Dias <jvdias> - 0.66-11.1

> > - some of the tests fail but "make check" passes - why?
> 
> Because the package was not being built as root. 
> 
> Now, all tests that require root access are skipped if being run by a non-root
> user, so all tests pass as a non-root user; I've also tested that those tests
> are run and pass if run by root.

Good; I understand why the tests were failing originally, but what I don't
understand is why the presence of failing tests didn't cause the package build
to fail in %check.

> > - missing buildreqs elfutils-libelf-devel and bzip2-devel
> 
> Added.

Good. Build in mock now works.

> > - using find/filelist method for %files results in unowned directory
> > %{perl_vendorarch}/auto/RPM2
> 
> I've added:
> %dir %{perl_vendorarch}/auto/RPM2
> 
> to %files list.

Good.

> >- consider adding %{?dist} tag if this package is likely to be needed for 
> >  future Fedora releases
> 
> Added.

Good.

> Modified .spec file and srpm at:
>   http://people.redhat.com/~jvdias/perl-RPM2.spec
>   http://people.redhat.com/~jvdias/perl-RPM2-0.66-12.src.rpm

Please bump the release number for iterations of packages during the review
process; it helps people to see which version is being referred to.

Still to do: clarify license, remove perl buildreq.
Then I can approve.

(In reply to comment #4)
>   MUST: A package must not contain any BuildRequires that are listed in
>   the exceptions section of Packaging Guidelines.

That MUST has been debated before, and especially in case of perl module
packages buildrequring perl.  Blocking packages based on that is IMHO a bit
silly, and yes, so is the guideline and I don't have anything against fixing it.

(In reply to comment #5)
> (In reply to comment #4)
> >   MUST: A package must not contain any BuildRequires that are listed in
> >   the exceptions section of Packaging Guidelines.
> 
> That MUST has been debated before, and especially in case of perl module
> packages buildrequring perl.  Blocking packages based on that is IMHO a bit
> silly, and yes, so is the guideline and I don't have anything against fixing it.

I agree that it's a silly MUST but the right the to do is to change the rule
rather than ignore it.

I would suggest the following:

Instead of:
 - MUST: A package must not contain any BuildRequires that are listed in the
exceptions section of Packaging Guidelines.
- MUST: All other Build dependencies must be listed in BuildRequires.

I'd have:
 - MUST: All build dependencies must be listed in BuildRequires, except for any
that are listed in the exceptions section of Packaging Guidelines; inclusion of
those as BuildRequires is optional.

Looks like Packaging/ReviewGuidelines has been updated to deal with this:

- MUST: All build dependencies must be listed in BuildRequires, except for any
that are listed in the exceptions section of Packaging Guidelines; inclusion of
those as BuildRequires is optional. Apply common sense.

(In reply to comment #7)
> Looks like Packaging/ReviewGuidelines has been updated to deal with this:
> 
> - MUST: All build dependencies must be listed in BuildRequires, except for any
> that are listed in the exceptions section of Packaging Guidelines; inclusion of
> those as BuildRequires is optional. Apply common sense.

Yes, that's OK, which just leaves the license issue to be resolved.

A couple of moths ago I opened two tickets in rt.cpan in order to ask Chip Turner
to add license information to RPM2 and Rpm::Specfile. No answer until now.  IIRC
in the past Warren was able to contact Chip. Maybe we should ping him.

By the way, isn't RPM2 included in the new versions of RPM? 

I found the following perl related lines in the CHANGES file of RPM 4.4.6

--------
4.4.5 -> 4.4.6:
  ...
  - perl: fix exuberant cut-n-paste.
  ...

4.4.4 -> 4.4.5:
  ...
  - fix: perldeps and prov.pl not to emit perl(main) (#177960)
  ...

4.4.3 -> 4.4.4:
  ...
  - perl: diddle Makefile.PL to build/link within rpm source tree.
  - perl: add build dependency on perl(ExtUtils::MakeMaker) >= 6.17.
  - complete forking perl-RPM2/* to perl/* module name "RPM".
  - perl: install in vendor dir.
  ...

4.4.2 -> 4.4.3:
  ...
  - fix: revert to older/slower perldeps avoid getOutputFrom() error.
  - perl: handle the 'v' in "use v5.6.0" (#140597).
  - add run-time perl(...) provides name space dependency set.
  - resurrect a rpm-perl subpackage from perl-RPM2-0.66.
  ...
----------

Didn't the author formerly work for RedHat, and write this while he worked
there?  Wouldn't RedHat have a record of this license?

(In reply to comment #11)
> Didn't the author formerly work for RedHat, and write this while he worked
> there?  Wouldn't RedHat have a record of this license?

Or, in a similar vein, is the copyright of works produced by employees whilst
working for Red Hat assigned to Red Hat?

Looks like this review is going nowhere, so I'll close it soon unless somebody
has something new to add.

Isn't there anyone @redhat.com that can determine the original license of this
module?

Barring that, have we looked at RPM4 on CPAN?

CC += fedora-perl-devel-list

* I believe Jason is no longer around;  maybe Robin Norwood could take over
  this and try to contact the Red Hat Legal Department

* The core perl-RPM-Specfile package has the same problem:
   - same author (Chip Turner)
   - same initial packager (Chip Turner)
   - it doesn't contain any copyright information
     (http://search.cpan.org/dist/RPM-Specfile/)
   - the specfile states the license is "GPL or Artistic"
     $ rpm -qp --qf="%{license}\n" perl-RPM-Specfile-1.19-2.1.1.src.rpm
     GPL or Artistic

* The upstream RPM package started including the perl RPM[2] module as of 4.4.3.
  It now generates a rpm-perl subpackage (http://wraptastic.org/pub/rpm-4.4.x/)

  $ rpm -qpl rpm-perl-4.4.3-1.i386.rpm
  /usr/lib/perl5/site_perl/5.8.7/i386-linux-thread-multi/RPM2.pm
  /usr/lib/perl5/site_perl/5.8.7/i386-linux-thread-multi/auto/RPM2
  /usr/lib/perl5/site_perl/5.8.7/i386-linux-thread-multi/auto/RPM2/RPM2.so
  /usr/share/man/man3/RPM2.3pm.gz

  $ rpm -qpl rpm-perl-4.4.6-1.i386.rpm
  /usr/lib/perl5/site_perl/5.8.8/i386-linux-thread-multi/RPM.pm
  /usr/lib/perl5/site_perl/5.8.8/i386-linux-thread-multi/auto/RPM
  /usr/lib/perl5/site_perl/5.8.8/i386-linux-thread-multi/auto/RPM/RPM.so
  /usr/share/man/man3/RPM.3pm.gz

  Note: the rpm maintainer has renamed the perl module (RPM2 -> RPM)

I can contact Chip to clarify the license.

Since FC5 and FC6 both still seem to have rpm-4.4.2, I assume we still want to
get this into extras, yes?

I wrote this and RPM::Specfile while at red hat.  The license is the same as
perl itself:


Perl5 is Copyright (C) 1993-2005, by Larry Wall and others.

It is free software; you can redistribute it and/or modify it under the terms of
either:

a) the GNU General Public License as published by the Free Software Foundation;
either version 1, or (at your option) any later version, or

b) the "Artistic License".

from: http://dev.perl.org/licenses/

A lawyer or two from RH contacted me and I've told them the above on a few
occasions.

I'll look into making new CPAN releases of these modules with a clear license as
well.

HTH

And I didn't even have to say his name three times!

Paul, is the above sufficient, or do you need to wait for the new CPAN release?

I'm happy to approve this now, but is Jason still around to maintain it?

No, he isn't.  I'm maintaining most of the perl* packages for FC and RHEL, so I
could take it if no-one else wants it.  I'll send a quick mail to
fedora-perl-devel to see if anyone else wants it first, though.

Turns out that there are/were a couple of existing bugs with this package.  I
haven't verified if they are still bugs with the latest version yet.

bugzilla #73921 - packaging issue
bugzilla #129724 - perl-RPM2 can't install multiple packages in a single
transaction.

What's the status on this review?

(In reply to comment #22)
> What's the status on this review?

The thing that was holding it up originally was the license. This has been fixed
by a new upstream release (0.67) which states that it's licensed under the same
terms as perl itself (i.e. GPL or Artistic).

In Comment #21, Robin was going to check whether Bug #73921 and Bug #129724 were
still present before going ahead with owning the package in Extras,

I have created an updated SRPM for 0.67 that brings the package much more into
line with the way perl modules are normally written for Extras. This addresses
the directory ownership issue of Bug #73921.

I haven't checked the status of Bug #129724 but that looks to me more like an
issue for upstream rather than a packaging issue, and I don't think that should
block the package from being imported.

Updated SRPM:
http://www.city-fan.org/~paul/extras/perl-RPM2/perl-RPM2-0.67-1.src.rpm
Updated Spec: http://www.city-fan.org/~paul/extras/perl-RPM2/perl-RPM2.spec

Robin, what's the state of play for this package from your point of view?

Paul, the new spec file looks great, and I agree that bug #129724 is a bug in
the upstream package.  My impression is that Chip won't be fixing it anytime
soon, so if we're ok with living with it, the package is fine as far as I'm
concerned.

(In reply to comment #24)
> Paul, the new spec file looks great, and I agree that bug #129724 is a bug in
> the upstream package.  My impression is that Chip won't be fixing it anytime
> soon, so if we're ok with living with it, the package is fine as far as I'm
> concerned.

Great stuff. I already approved the package earlier so feel free to import and
built it etc. (I assume you're still happy to own this package?)

ping? :)

Robin, are you going to own this package and import it?

Or does anyone else volunteer?

Sorry, dropped the ball on this one - I'll look into it today.

New Package CVS Request
=======================
Package Name: perl-RPM2
Short Description: Perl bindings for RPM
Owners: rnorwood
Branches: FC-5 FC-6
InitialCC: 

Added fedora-perl-devel-list in CC too.

Was this branched to extras cvs?  I can't seem to find it in there.

Yeah, i don't see it either - did I miss a step here?

Sorry, seems I forgot to make the cvs module.
Should be there now, please try again.

Hrm - checking out the module works for me now (cvs co perl-RPM2), but importing
it fails:

"""
[rnorwood@solitude fedora-extras]$ common/cvs-import.sh
/tmp/perl-RPM2-0.67-1.src.rpm 
Checking out module: 'perl-RPM2'
connect to address 10.8.34.151 port 544: Connection refused
cvs [checkout aborted]: end of file from server (consult above messages if any)
ERROR: "perl-RPM2" module does not exist in cvs.
[rnorwood@solitude fedora-extras]$ echo $CVSROOT
:ext:rnorwood.redhat.com:/cvs/extras
[rnorwood@solitude fedora-extras]$ echo $CVS_RSH
ssh
"""

Any ideas?

(In reply to comment #35)
> connect to address 10.8.34.151 port 544: Connection refused
> cvs [checkout aborted]: end of file from server (consult above messages if any)

That doesn't look right.  Sorry but could you try again
and if you still have problems please ask on #fedora-admin or
cvsadmin-members at fedoraproject org.

Ok, I figured out what was breaking.  The odd error message threw me off...I
added a -t to the cvs command in the cvs_import.sh script:

 -> Starting server: /usr/kerberos/bin/krsh -l rnorwood cvs.fedora.redhat.com
cvs server 


It's using krsh probably because I use krsh for my RH CVS checkout - my
~/.bashrc was setting CVS_RSH to it because of some changed I'd made recently.

Works now, thanks.

Ok, ran builds for FC-5 and FC-6.  We'll see how they go.

ok, looks like they both built successfully.  Finally done with this bug. :-)

(In reply to comment #39)
> ok, looks like they both built successfully.  Finally done with this bug. :-)

Not yet ;)
You haven't built it for development (Fedora 7).

jpo

Oh.  I assumed f7 was going to include the newer version of rpm (your comment
#10), but it looks like we're at 4.4.2.

Anyway, built for f7, I'll send a request to rel-eng folks that it be included.

"""
Package: perl-RPM2
NVR: perl-RPM2-0.67-2.fc7
User: jkeating
Status: complete
Tag Operation: tagged
Into Tag: f7-final

perl-RPM2-0.67-2.fc7 successfully tagged into f7-final by jkeating
"""

Ok, Jose, *now* can I close the bug?  ;-)

Package Change Request
======================
Package Name: perl-RPM2
New Branches: EL-4 EL-5
Updated EPEL Owners: rnorwood

cvs done.

Thanks!

Package Change Request
======================
Package Name: perl-RPM2
New Branches: F-10

Note: This was declared dead due to incompatibility with RPM 4.6 before F-10 branching took place. It had been resurrected in CVS devel and patched for RPM 4.6 now.

cvs done.