Bug #29348 for CPANPLUS: Version compare logic doesn't handle alphas?

Thu Sep 13 16:21:48 2007 mschwern [...] cpan.org - Ticket created

Subject:

Version compare logic doesn't handle alphas?

I wanted to install the alpha version of CPAN::YACSmoke so I tried this: $ cpanp CPANPLUS::Shell::Default -- CPAN exploration and module installation (v0.82) *** Please report bugs to <bug-cpanplus@rt.cpan.org>. *** Using CPANPLUS::Backend v0.82. ReadLine support enabled. *** Type 'p' now to show start up log Did you know... You can update CPANPLUS by running: 's selfupdate' CPAN Terminal> s conf verbose 1 Key 'verbose' was set to '1' CPAN Terminal> install /R/RR/RRWO/CPAN-YACSmoke-0.03_07.tar.gz [MSG] Checking if source files are up to date [MSG] Retrieving /Users/schwern/.cpanplus/sourcefiles.2.16.stored Installing CPAN::YACSmoke (0.03_07) [MSG] Module 'CPAN::YACSmoke' already up to date, won't install without force *** Install log written to: /Users/schwern/.cpanplus/install-logs/CPAN-YACSmoke-0.03_07-1189714549.log Module 'CPAN::YACSmoke' installed successfully No errors installing all modules It figured out that I wanted to install CPAN::YACSmoke 0.03_07 and came to the wrong conclusion about it being up to date. My installed version is 0.03. $ perl -wle 'use CPAN::YACSmoke; print $CPAN::YACSmoke::VERSION' 0.03 I suspect the version comparison logic doesn't handle alphas.

Fri Sep 14 05:52:13 2007 kane [...] cpan.org - Correspondence added

On Thu Sep 13 16:21:48 2007, MSCHWERN wrote: Show quoted text

> I wanted to install the alpha version of CPAN::YACSmoke so I tried this:

[...] Show quoted text

> It figured out that I wanted to install CPAN::YACSmoke 0.03_07 and came > to the wrong conclusion about it being up to date. My installed version > is 0.03. > > $ perl -wle 'use CPAN::YACSmoke; print $CPAN::YACSmoke::VERSION' > 0.03 > > I suspect the version comparison logic doesn't handle alphas.

That's correct -- we use Module::Load::Conditional for the version comparison, which does a plain <=. Unfortunately, perl itself doesn't recognize _ as part of numbers: $ perl -le'print "0.17_01" == "0.17_99"' 1 Luckily, version.pm is already a prereq for Module::Load::Conditional, and it's qv() method together with cmp overloading does the right thing: $ perl -Mversion -le'print qv("0.17_01") < qv("0.17_99")' 1 So now M::L::C uses qv() to do it's comparisons :) Thanks for reporting, -- Jos

Fri Sep 14 05:52:28 2007 The RT System itself - Status changed from 'new' to 'open'

Fri Sep 14 05:53:08 2007 kane [...] cpan.org - Status changed from 'open' to 'resolved'

Wed Oct 17 09:43:53 2007 jpeacock [...] cpan.org - Correspondence added

From:

JPEACOCK [...] cpan.org

On Fri Sep 14 05:52:13 2007, KANE wrote: Show quoted text

> $ perl -Mversion -le'print qv("0.17_01") < qv("0.17_99")' > 1 > > So now M::L::C uses qv() to do it's comparisons :)

This is wrong. qv() forces the "quoted version" parsing, which is not the same thing (among other things leading zeros are ignored). However version->new() does the correct thing with dev versions: $ perl -Mversion -le'print version->new("0.17") < version->new("0.17_99")' 1 even if passed in a version object. For more examples of how qv() and version->new() differ, see these two "similar" objects: $ perl -Mversion -MData::Dumper -le'print Dumper(version->new("0.0017"))' $VAR1 = bless( { 'original' => '0.0017', 'version' => [ 0, 1, 700 ] }, 'version' ); $ perl -Mversion -MData::Dumper -le'print Dumper(qv("0.0017"))' $VAR1 = bless( { 'original' => 'v0.0017', 'qv' => 1, 'version' => [ 0, 17, 0 ] }, 'version' ); You should be able to do s/qv/version->new/g in your module and everything will continue to work. I'm trying to come up with a clever way to test this specific scenario... John

Wed Oct 17 09:43:55 2007 The RT System itself - Status changed from 'resolved' to 'open'

Wed Oct 17 10:59:42 2007 kane [...] cpan.org - Correspondence added

On Wed Oct 17 09:43:53 2007, JPEACOCK wrote: Show quoted text

> This is wrong. qv() forces the "quoted version" parsing, which is not > the same thing (among other things leading zeros are ignored). However > version->new() does the correct thing with dev versions:

[...] Show quoted text

> You should be able to do s/qv/version->new/g in your module and > everything will continue to work. I'm trying to come up with a clever > way to test this specific scenario...

Ok, this is the patch I applied. Let me know if this isn't what you meant... $ svk diff === lib/Module/Load/Conditional.pm ========================================================= ========= --- lib/Module/Load/Conditional.pm (revision 2459) +++ lib/Module/Load/Conditional.pm (local) @@ -9,7 +9,7 @@ use Carp (); use File::Spec (); use FileHandle (); -use version qw[qv]; +use version; use constant ON_VMS => $^O eq 'VMS'; @@ -18,7 +18,7 @@ $FIND_VERSION $ERROR $CHECK_INC_HASH]; use Exporter; @ISA = qw[Exporter]; - $VERSION = '0.22'; + $VERSION = '0.23_01'; $VERBOSE = 0; $FIND_VERSION = 1; $CHECK_INC_HASH = 0; @@ -280,8 +280,14 @@ ### use qv(), as it will deal with developer release number ### ie ones containing _ as well. This addresses bug report ### #29348: Version compare logic doesn't handle alphas? + ### + ### Update from JPeacock: apparently qv() and version->new + ### are different things, and we *must* use version->new + ### here, or things like #30056 might start happening $href->{uptodate} = - qv( $args->{version} ) <= qv( $href->{version} ) ? 1 : 0; + version->new( $args->{version} ) <= version->new( $href->{version} ) + ? 1 + : 0; } return $href; @@ -426,9 +432,14 @@ ### use qv(), as it will deal with developer release number ### ie ones containing _ as well. This addresses bug report ### #29348: Version compare logic doesn't handle alphas? + ### + ### Update from JPeacock: apparently qv() and version->new + ### are different things, and we *must* use version->new + ### here, or things like #30056 might start happening if ( !$args->{nocache} && defined $CACHE->{$mod}->{usable} - && (qv($CACHE->{$mod}->{version}||0) >= qv($href->{$mod})) + && (version->new( $CACHE->{$mod}->{version}||0 ) + >= version->new( $href->{$mod} ) ) ) { $error = loc( q[Already tried to use '%1', which was unsuccessful], $mod); last BLOCK;

Wed Oct 17 21:07:16 2007 jpeacock [...] cpan.org - Correspondence added

From:

JPEACOCK [...] cpan.org

On Wed Oct 17 10:59:42 2007, KANE wrote: Show quoted text

> Ok, this is the patch I applied. Let me know if this isn't what you > meant...

Yes, that should do nicely. To be honest, you don't actually need to wrap both sides in version->new(), since the overloaded comparison will automatically upgrade the non-object side. But, by the same token, there is no reason not to either, since the same effort will take place internally otherwise. I'm still trying to craft a reasonable test to ensure that the example case will DTRT... John

Thu Oct 18 07:36:50 2007 jpeacock [...] cpan.org - Correspondence added

From:

JPEACOCK [...] cpan.org

On Wed Oct 17 21:07:16 2007, JPEACOCK wrote: Show quoted text

> I'm still trying to craft a reasonable test to ensure that the example > case will DTRT...

I think the attached diff should be the thing. Ideally, we'd like to have an installed package that we can compare against a newer alpha/beta/dev release, but that isn't practical here. Sorry for the confusion about qv(). I'm going to rewrite the POD to make it clearer why you do or do not want to use qv()... John

--- ./t/01_Module_Load_Conditional.t.orig Thu Oct 18 07:29:04 2007 +++ ./t/01_Module_Load_Conditional.t Thu Oct 18 07:24:49 2007 @@ -172,6 +172,18 @@ ); } +{ + my $use_list = { 'Dev' => 0.17 }; + my $bool = can_load( modules => $use_list ); + + ok( $bool, q[Dev module is newer than requested] ); + + delete $INC{'Dev.pm'}; + $use_list = { 'Dev' => 0.18 }; + $bool = can_load( modules => $use_list, nocache => 1 ); + + ok( !$bool, q[Dev module is older than requested] ); +} ### test 'requires' ### SKIP:{ --- ./t/to_load/Dev.pm.orig Thu Oct 18 07:31:38 2007 +++ ./t/to_load/Dev.pm Thu Oct 18 07:32:04 2007 @@ -0,0 +1,5 @@ +package Dev; + +$VERSION = "0.17_99"; + +1;

Sat Nov 03 13:48:30 2007 kane [...] cpan.org - Status changed from 'open' to 'resolved'