Bug 166203

New version of perl-Maypole (also dragged in 2 new dependency packages,
Apache::Session and Apache::Session::Wrapper). Also has fixups pointed out from
other Maypole packages:

SRPM:
http://www.auroralinux.org/people/spot/review/Maypole/perl-Maypole-2.10-1.src.rpm

SPEC:
http://www.auroralinux.org/people/spot/review/Maypole/perl-Maypole.spec

GOOD:

- rpmlint clean
- package name okay
- spec file name okay
- compatible license, matches upstream
- spec file is legible and am. english
- sources match upstream
  5d908a447d4e6f6364403aa56b1ab211  Maypole-2.10.tar.gz
  486cc148ab9b9aab52cc4035994f1c43  Maypole-Plugin-Session-0.2.tar.gz
- package builds in mock (FC-4 x86)
- no locales
- no shared libs
- not relocatable
- owns all created directories
- no duplicate files
- file permissions okay
- %clean okay
- consistent use of macros
- code, not content
- no -doc or -devel, %doc okay

Questions/Comments/Needswork:

Missing requires on perl(Apache2::Request) (see below).

You'll get more test coverage with,
  BR: perl(Test::Pod) >= 1.14
  BR: perl(Test::Pod::Coverage) >= 1.04
  (version numbers probably not be needed)

It's probably too much to hope that t/apache_mvc can be run successfully, since
it uses Apache::Request, which is now Apache2::Request.  An rt.cpan.org bug
report (#13888) seems to indicate that this will be fixed in an upcoming release.

The test suite for Maypole-Plugin-Session isn't run at all.

The Maypole Makefile.PL lists the following prerequisites, which are not listed
as BRs.
  - Class::DBI::Loader  => '0.02',
  - Class::DBI          => 0.96,
  - CGI::Untaint        => 0,
  - UNIVERSAL::moniker  => 0,
  - UNIVERSAL::require  => 0,
  - URI::QueryParam     => 0,
  - HTTP::Headers       => 1.59,
  - Template            => 0,
Since the package builds in mock, I'm assuming that other BRs pull these in
successfully.  Given the list of BRs, it's a safe bet that Class::DBI::Loader,
Class::DBI, and Template will always be pulled in, but I'd like to see them
listed explicitly anyway.

Why is Maypole-Plugin-Session being packaged inside perl-Maypole instead of
separately?  The distributions are separate on CPAN, with different authors. 
Will the versions always stay in sync?

perl-Maypole-2.10-Apache2only.patch:

- forces this to work on only FC-4+, which is fine; Mason did the same, I believe
- get_request() appears to need the requires on Apache::Request (or, in
mod_perl-2.0.x, Apache2::Request) to work; this patch would leave Apache::MVC broken
- no need to require Bundle::Apache2; it's only useful for CPAN shell installs

The patch attached to http://rt.cpan.org/NoAuth/Bug.html?id=13888 appears to
address the issues.

perl-Maypole-2.10-sessionfix.patch:

- is this necessary? (now I see why Maypole-Plugin-Session is being bundled)

On brief examination, Maypole-Plugin-Session's generate_unique_id() routine is
really a fancy pass-thru to Apache::Session's generate_unique_id() routine. 
Maypole::Session's genereate_unique_id() routine appears to simply be a copy of
the one found in Apache::Session.

I hope that was thorough enough.

Thanks for the very thorough review.

-2 fixes the following items:

- Adds more explicit BuildRequires
- Uses patch to fix Apache2 from upstream SVN (patch in rt was incomplete)
  (this also fixes get_request() )
- Runs check on Maypole-Plugin-Session as well

I think the sessionfix patch and the inclusion of Maypole-Plugin-Session are
necessary, because I don't want to deviate from upstream.

t/apache_mvc is still broken in upstream SVN, so I left it broken.

New SRPM:
http://www.auroralinux.org/people/spot/review/Maypole/perl-Maypole-2.10-2.src.rpm

New SPEC:
http://www.auroralinux.org/people/spot/review/Maypole/perl-Maypole.spec

(In reply to comment #3)
> - Runs check on Maypole-Plugin-Session as well

Seems I was a bit hasty.  This now prevents the package from building.  The
tests attempt to use the Maypole module, but it's not actually installed yet
(without some @INC patching to the tests, which I wouldn't really recommend).

> I think the sessionfix patch and the inclusion of Maypole-Plugin-Session are
> necessary, because I don't want to deviate from upstream.

I've looked around the Maypole web site and browsed the SVN trunk, but I don't
see how packaging Maypole-Plugin-Session keeps with upstream.

There is, however, a bug in Maypole::Session:
  http://rt.cpan.org/NoAuth/Bug.html?id=14124

As noted, this is fixed in the SVN trunk.  The sessionfix patch does make this
bug irrelevant, but I'm still not sure why Maypole-Plugin-Session is included. 
I'll need more convincing on that.

Maypole-2.10/lib/Maypole/Session.pm
@@ -35,7 +35,7 @@
 sub generate_unique_id {
     my $length = shift || 32;
     my $id = substr(Digest::MD5::md5_hex(Digest::MD5::md5_hex(time(). {}.
rand(). $$)), 0, $length);
-    return;
+    return $id;
 }


(In reply to comment #4)
> (In reply to comment #3)

> > I think the sessionfix patch and the inclusion of Maypole-Plugin-Session are
> > necessary, because I don't want to deviate from upstream.

> There is, however, a bug in Maypole::Session:
>   http://rt.cpan.org/NoAuth/Bug.html?id=14124
 
> As noted, this is fixed in the SVN trunk. The sessionfix patch does make this
> bug irrelevant, but I'm still not sure why Maypole-Plugin-Session is included. 
> I'll need more convincing on that.
Seconded. I think, the sessionfix should be removed and be replaced by
Maypole/Session.pm from SVN, which effectively is a one-liner diff:

Maypole-2.10/lib/Maypole/Session.pm
@@ -35,7 +35,7 @@
 sub generate_unique_id {
     my $length = shift || 32;
     my $id = substr(Digest::MD5::md5_hex(Digest::MD5::md5_hex(time(). {}.
rand(). $$)), 0, $length);
-    return;
+    return $id;
 }

AFAIU, this also renders the Maypole-Plugin-Session module superfluous, rsp.
enables you to package it separately ...

Ok, works for me. -3 removes the Maypole-Plugin-Session source and build, uses
the upstream SVN fix instead.

The only remaining issue I hit was that RPM is again seeing perl(mod_perl) as a
dependency (scooping it up from Apache/MVC.pm). My obvious choices are to patch
out the mod_perl call as I did before, or to grep out that dependency.

This time, I chose to grep out that dependency:

cat > filter_depends.sh <<EOF
#!/bin/sh
/usr/lib/rpm/find-requires.perl $* | grep -v 'perl(mod_perl)'
EOF
chmod +x filter_depends.sh

%define __find_requires %{_builddir}/Maypole-%{version}/filter_depends.sh
%define _use_internal_dependency_generator 0

New SRPM:
http://www.auroralinux.org/people/spot/review/Maypole/perl-Maypole-2.10-3.src.rpm

New SPEC:
http://www.auroralinux.org/people/spot/review/Maypole/perl-Maypole.spec

This seems like an overly drastic measure to me.

I am using 
%define __perl_requires <filter-script>

in cases like these (cf. perl-Test-Inline.spec in FE's CVS)

Three lines vs seven for my implementation... same end result. I'll defer to
whichever way the reviewer wants it done?

(In reply to comment #8)
> Three lines vs seven for my implementation... same end result.
Nah, this is what makes the difference:
%define _use_internal_dependency_generator 0

(In reply to comment #9)
> (In reply to comment #8)
> > Three lines vs seven for my implementation... same end result.
> Nah, this is what makes the difference:
> %define _use_internal_dependency_generator 0

I don't know the difference between the two approaches.  Ralf, can you explain
it a bit to me before I continue this review?

(In reply to comment #6)
> cat > filter_depends.sh <<EOF
> #!/bin/sh
> /usr/lib/rpm/find-requires.perl $* | grep -v 'perl(mod_perl)'
> EOF

There is a problem in the above script: the $* gets expanded before being
written to the file (empty string in this case).

Possible solutions:
1) remove the special meaning of the '$' char by using a backslash (\$*)
2) quote the here document limit string (eg: single quotes -> 'EOF')
   For additional information see example 17.6 of 
   http://www.faqs.org/docs/abs/HTML/here-docs.html

Another problem with that script is that it     
hardcodes /usr/lib/rpm/find-requires.perl, ignoring the system's rpmbuild   
config.  See the way __perl_provides is handled in perl-Template-Toolkit for a 
way to avoid that (although it's missing the $*, but the args are unused 
nowadays in the default rpmbuild config).  __perl_(provides|requires) do not 
need disabling of the internal dep generator. 

I took a look at perl-Template-Toolkit.spec and I like the way it handles
__perl_provides.  I prefer that approach, but no one has convinced me that
Spot's approach is completely broken.  Since the rest of the package is fine,
I'm going to approve it.  I'll leave it up to Spot to change it post-commit if
he wants to.

Housekeeping.  This package is in the repositories now.