Bug 166203 - Review Request: perl-Maypole : MVC web application framework
Review Request: perl-Maypole : MVC web application framework
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Chris Grau
David Lawrence
http://maypole.perl.org
:
Depends On: 166182 166185 166186 166187 166189 166190 166193 166195 166196 166200 166202 167755
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-17 16:52 EDT by Tom "spot" Callaway
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-10-21 22:35:11 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Tom "spot" Callaway 2005-08-17 16:52:05 EDT
Spec Name or Url: 
http://www.auroralinux.org/people/spot/review/Maypole/perl-Maypole.spec

SRPM Name or Url:
http://www.auroralinux.org/people/spot/review/Maypole/perl-Maypole-2.10-0.2.pre1.src.rpm

Description: 

Maypole is a Perl framework for MVC-oriented web
applications, similar to Jakarta's Struts. Maypole
is designed to minimize coding requirements for
creating simple web interfaces to databases, while
remaining flexible enough to support enterprise web
applications.
Comment 1 Tom "spot" Callaway 2005-09-07 16:25:11 EDT
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
Comment 2 Chris Grau 2005-10-06 14:06:59 EDT
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.
Comment 3 Tom "spot" Callaway 2005-10-06 15:40:43 EDT
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
Comment 4 Chris Grau 2005-10-06 19:04:10 EDT
(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.
Comment 5 Ralf Corsepius 2005-10-07 09:43:19 EDT
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 ...
Comment 6 Tom "spot" Callaway 2005-10-07 11:33:32 EDT
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
Comment 7 Ralf Corsepius 2005-10-07 12:03:44 EDT
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)
Comment 8 Tom "spot" Callaway 2005-10-07 12:10:27 EDT
Three lines vs seven for my implementation... same end result. I'll defer to
whichever way the reviewer wants it done?
Comment 9 Ralf Corsepius 2005-10-07 12:24:48 EDT
(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
Comment 10 Chris Grau 2005-10-07 15:58:30 EDT
(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?
Comment 11 Jose Pedro Oliveira 2005-10-07 21:14:45 EDT
(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
Comment 12 Ville Skyttä 2005-10-08 05:16:00 EDT
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. 
Comment 13 Chris Grau 2005-10-10 16:15:15 EDT
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.
Comment 14 Chris Grau 2005-10-21 22:35:11 EDT
Housekeeping.  This package is in the repositories now.

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