Bug 547655 - Review Request: perl-Module-Install-RTx - RT extension installer
Review Request: perl-Module-Install-RTx - RT extension installer
Status: ASSIGNED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ralf Corsepius
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 493335
  Show dependency treegraph
 
Reported: 2009-12-15 05:40 EST by Xavier Bachelot
Modified: 2013-11-21 10:26 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rc040203: fedora‑review?


Attachments (Terms of Use)

  None (edit)
Description Xavier Bachelot 2009-12-15 05:40:28 EST
Spec URL: http://www.bachelot.org/fedora/SPECS/perl-Module-Install-RTx.spec
SRPM URL: http://www.bachelot.org/fedora/SRPMS/perl-Module-Install-RTx-0.25-2.fc10.src.rpm
Description: This Module::Install extension implements one function, RTx, that takes the extension name as the only argument.
Comment 1 Ralf Corsepius 2009-12-16 00:10:39 EST
For obvious reasons, I am going to review this ;)

* MUSTFIX: Your patch creates a backup files, which gets installed:

# rpm -qlp perl-Module-Install-RTx-0.25-2.fc12.noarch.rpm
/usr/lib/perl5/vendor_perl/5.10.0/Module
/usr/lib/perl5/vendor_perl/5.10.0/Module/Install
/usr/lib/perl5/vendor_perl/5.10.0/Module/Install/RTx
/usr/lib/perl5/vendor_perl/5.10.0/Module/Install/RTx.pm
/usr/lib/perl5/vendor_perl/5.10.0/Module/Install/RTx.pm.try_RTHOME_first
/usr/lib/perl5/vendor_perl/5.10.0/Module/Install/RTx/Factory.pm
/usr/share/doc/perl-Module-Install-RTx-0.25
/usr/share/doc/perl-Module-Install-RTx-0.25/Changes
/usr/share/doc/perl-Module-Install-RTx-0.25/README
/usr/share/man/man3/Module::Install::RTx.3pm.gz

Note the file named RTx.pm.try_RTHOME_first.


* Can you explain your patch?
The original code first searches @INC, then searches the other (IMO questionable) places. As this module's purpose is to first check where RT is installed on a system, your patch IMO is wrong.

Did you propose your patch to upstream?
Comment 2 Xavier Bachelot 2009-12-16 03:44:08 EST
(In reply to comment #1)
> For obvious reasons, I am going to review this ;)
> 
Thanks :-)

> * MUSTFIX: Your patch creates a backup files, which gets installed:
> 
> # rpm -qlp perl-Module-Install-RTx-0.25-2.fc12.noarch.rpm
> /usr/lib/perl5/vendor_perl/5.10.0/Module
> /usr/lib/perl5/vendor_perl/5.10.0/Module/Install
> /usr/lib/perl5/vendor_perl/5.10.0/Module/Install/RTx
> /usr/lib/perl5/vendor_perl/5.10.0/Module/Install/RTx.pm
> /usr/lib/perl5/vendor_perl/5.10.0/Module/Install/RTx.pm.try_RTHOME_first
> /usr/lib/perl5/vendor_perl/5.10.0/Module/Install/RTx/Factory.pm
> /usr/share/doc/perl-Module-Install-RTx-0.25
> /usr/share/doc/perl-Module-Install-RTx-0.25/Changes
> /usr/share/doc/perl-Module-Install-RTx-0.25/README
> /usr/share/man/man3/Module::Install::RTx.3pm.gz
> 
> Note the file named RTx.pm.try_RTHOME_first.
> 
oops, will fix.

> 
> * Can you explain your patch?
> The original code first searches @INC, then searches the other (IMO
> questionable) places. As this module's purpose is to first check where RT is
> installed on a system, your patch IMO is wrong.
>
The problem is it tries to use LocalPath from the @INC RT.pm, which is set to /usr/local/lib/rt3, thus installing the extension to the wrong place. So I create another RT.pm for the installation purpose and force the use of it thru the RTHOME envvar. However, the RT.pm from @INC is preferred over the one from RTHOME, which is a dubious in my opinion. If one is forcing the use of a RTHOME, one would assume this will be the first tried location, hence the patch.

> Did you propose your patch to upstream?  
Yup, I linked the bug in the spec. No answer yet, but I logged it yesterday, so no surprise.
http://rt.cpan.org/Ticket/Display.html?id=52776
Comment 3 Xavier Bachelot 2009-12-16 03:51:34 EST
> > * Can you explain your patch?
> > The original code first searches @INC, then searches the other (IMO
> > questionable) places. As this module's purpose is to first check where RT is
> > installed on a system, your patch IMO is wrong.
> >
> The problem is it tries to use LocalPath from the @INC RT.pm, which is set to
> /usr/local/lib/rt3, thus installing the extension to the wrong place. So I
> create another RT.pm for the installation purpose and force the use of it thru
> the RTHOME envvar. However, the RT.pm from @INC is preferred over the one from
> RTHOME, which is a dubious in my opinion. If one is forcing the use of a
> RTHOME, one would assume this will be the first tried location, hence the
> patch.
> 
Oops, sorry, this is indeed not understandable as is. I'll blame a missing morning coffee. The RTHOME usage is actually referring to the perl-RTx-Calendar rpackage (https://bugzilla.redhat.com/show_bug.cgi?id=493335), which is making use of Module::Install::RTx.
Comment 4 Xavier Bachelot 2009-12-17 11:07:22 EST
Upstream applied the patch.
Comment 5 Ralf Corsepius 2009-12-18 02:32:25 EST
Could you provide an updated src.rpm?
Comment 7 Xavier Bachelot 2010-02-02 11:57:27 EST
Ralf, you're still ok with reviewing this package ?
Comment 8 Ralf Corsepius 2010-02-02 12:17:26 EST
Urg, this review dropped off my personal radar - Sorry for that :(

I'll try to return to it ASAP (Likely some time later this week).
Comment 9 Xavier Bachelot 2010-02-03 05:23:38 EST
No problem, I understand. I thought a ping would be good idea and indeed it was :-) Thanks you.
Comment 10 Xavier Bachelot 2010-06-15 16:20:09 EDT
Hi Ralf, it seems it dropped off your radar again. I'd be grateful if you can review this package at your convenience. Thanks in advance.

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