Bug 1087536

Summary: Review Request: perl-HTML-FormFu-MultiForm - Handle multi-page/stage forms
Product: [Fedora] Fedora Reporter: Petr Pisar <ppisar>
Component: Package ReviewAssignee: Ralf Corsepius <rc040203>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: package-review, rc040203
Target Milestone: ---Flags: rc040203: fedora-review+
petersen: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: perl-HTML-FormFu-MultiForm-1.00-1.fc21 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2014-04-17 07:22:39 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 1085432    

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.