Bug 1087536 - Review Request: perl-HTML-FormFu-MultiForm - Handle multi-page/stage forms
Summary: Review Request: perl-HTML-FormFu-MultiForm - Handle multi-page/stage forms
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ralf Corsepius
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1085432
TreeView+ depends on / blocked
 
Reported: 2014-04-14 14:47 UTC by Petr Pisar
Modified: 2014-04-17 07:22 UTC (History)
2 users (show)

Fixed In Version: perl-HTML-FormFu-MultiForm-1.00-1.fc21
Clone Of:
Environment:
Last Closed: 2014-04-17 07:22:39 UTC
Type: ---
Embargoed:
rc040203: fedora-review+
petersen: fedora-cvs+


Attachments (Terms of Use)

Description Petr Pisar 2014-04-14 14:47:35 UTC
Spec URL: http://ppisar.fedorapeople.org/perl-HTML-FormFu-MultiForm/perl-HTML-FormFu-MultiForm.spec
SRPM URL: http://ppisar.fedorapeople.org/perl-HTML-FormFu-MultiForm/perl-HTML-FormFu-MultiForm-1.00-1.fc21.src.rpm
Description:
Multi-page support for HTML::FormFu, a Perl HTML form framework.

Fedora Account System Username: ppisar

Comment 1 Ralf Corsepius 2014-04-14 16:10:14 UTC
Package basically looks fine to me, except that it currently fails to build in mock:
...
Error: No Package found for perl(Test::Aggregate::Nested) >= 0.371
...

I noticed perl-Test-Aggregate-0.371 already is in koji, but apparently it hasn't reached the mirrors, yet. I'll continue this review once it has.

Comment 2 Ralf Corsepius 2014-04-16 05:42:16 UTC
APPROVED

2 remarks on issues, you probably are aware about:

- The package treats Crypt::DES as required dependency, while it actually doesn't use it (You seem to have commented out BR: perl(Crypt::DES) because of this)

I.e. this package only builds by random coincidence, because another package (perl-HTML-FormFu) indirectly pulls in perl-Crypt-DES. If perl-HTML-FormFu didn't do so, building this package would fail.

I'd recommend to patch out Crypt::DES from the source code.


- I am not happy with "PERL_HASH_SEED=0 make test", because it's not clear to me whether this is just an issue with the test-suite or whether this is a defect of this package in general.

Comment 3 Petr Pisar 2014-04-16 12:33:39 UTC
(In reply to Ralf Corsepius from comment #2)
> - The package treats Crypt::DES as required dependency, while it actually
> doesn't use it (You seem to have commented out BR: perl(Crypt::DES) because
> of this)
> 
> I.e. this package only builds by random coincidence, because another package
> (perl-HTML-FormFu) indirectly pulls in perl-Crypt-DES. If perl-HTML-FormFu
> didn't do so, building this package would fail.
> 
> I'd recommend to patch out Crypt::DES from the source code.
>
That's interesting problem. The Crypt::CBC calls from this package do not request Crypt::DES anywhere in the code. None CBC::Crypt->new() passes -cipher argument. The code relays on CBC::Crypt default algorithm which is CBC::DES.

However perl-Crypt-CBC does require Crypt::DES. I would rather see perl-Crypt-CBC run-requiring Crypt::DES to conform to Crypt::CBC documentation.

Unfortunately the perl-Crypt-CBC is in dependency cycle with perl-Crypt-DES and Paul decided to make the dependency on Crypt::DES optional.

So I will add the dependency on Crypt::DES into perl-HTML-FormFu-MultiForm to follow the upstream path.

Comment 4 Petr Pisar 2014-04-16 12:34:14 UTC
New Package SCM Request
=======================
Package Name: perl-HTML-FormFu-MultiForm
Short Description: Handle multi-page/stage forms
Owners: ppisar jplesnik psabata
Branches: 
InitialCC: perl-sig

Comment 5 Jens Petersen 2014-04-17 06:13:26 UTC
Git done (by process-git-requests).

Comment 6 Petr Pisar 2014-04-17 07:22:39 UTC
Thank you for the review and the repository.


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