Bug 547655

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?

(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

> > * 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.

Upstream applied the patch.

Could you provide an updated src.rpm?

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-3.fc12.src.rpm

Ralf, you're still ok with reviewing this package ?

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).

No problem, I understand. I thought a ping would be good idea and indeed it was :-) Thanks you.

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.

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

This is an automatic action taken by review-stats script.

The ticket reviewer failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we reset the status and the assignee of this ticket.

Do you want to finish the review? 

If yes, please use the latest version of the module and update the spec due to new guidelines.

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.