257933 - File::Spec version check does not work against 0.90

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Greg Hendricks]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1

I ran checksetup on a new installation of bugzilla and it complained that it
requires File::Spec > v.82 and gives instructions on how to install using CPAN.
I tried the install and CPAN says it is up to date. Upon further investigation I
found that I have version .90 of File::Spec installed on my system.
I hacked checksetup.pl and set the version for File::Spec to 0 as per the
instructions in the comments to skip checking for the module and it worked. This
is not ideal however if there is a true dependency on >v.82.

Reproducible: Always
Steps to Reproduce:
1. install File::Spec v .90
2. Run checksetup.pl
3.

Actual Results:  
See details

Expected Results:  
checksetup should have been fine with v.90 of File::Spec as v.90 is > .82 but it
was not happy with it. Installing against .87 works fine.

Comment 1

[Davin Hong]

(In reply to comment #0)
> Expected Results:  
> checksetup should have been fine with v.90 of File::Spec as v.90 is > .82 but it
> was not happy with it. Installing against .87 works fine.

Just a quick note: File::Spec reports as v0.9, not v0.90.  Maybe a problem in
vers_cmp, but Perl is greek to me.

Comment 2

[Marshal Newrock]

I had the same problem (using FreeBSD).  It is a problem with vers_cmp, which is
comparing the versions as strings, not numbers.  My solution is to force the
versions to strings of equal lengths with sprintf:

--- checksetup.pl.orig  Fri Feb 11 14:21:04 2005
+++ checksetup.pl       Fri Feb 11 14:21:10 2005
@@ -194,7 +194,8 @@
 sub vers_cmp {
   if (@_ < 2) { die "not enough parameters for vers_cmp" }
   if (@_ > 2) { die "too many parameters for vers_cmp" }
-  my ($a, $b) = @_;
+  my $a = sprintf("%.3f", shift);
+  my $b = sprintf("%.3f", shift);
   my (@A) = ($a =~ /(\.|\d+|[^\.\d]+)/g);
   my (@B) = ($b =~ /(\.|\d+|[^\.\d]+)/g);
   my ($A,$B);

Comment 3

[Marshal Newrock]

To correct myself, the original version of my fix doesn't take into account the
different ways a version can be indicated.  This patch replaces my old one.



--- checksetup.pl.orig  Fri Feb 11 14:21:04 2005
+++ checksetup.pl       Fri Feb 11 15:20:14 2005
@@ -201,6 +201,13 @@
   while (@A and @B) {
     $A = shift @A;
     $B = shift @B;
+    my $A_length = length($A);
+    my $B_length = length($B);
+    if ($A_length != $B_length) {
+      my $longest = ($A_length > $B_length) ? $A_length : $B_length;
+      $A = sprintf("%-*s", $longest, $A);
+      $B = sprintf("%-*s", $longest, $B);
+    }
     if ($A eq "." and $B eq ".") {
       next;
     } elsif ( $A eq "." ) {

Comment 4

[Henrik Nordstrom]

To me this looks like a bug in File::Spec. It returns the version number as a
float which crops away the 0 in .90, while most other modules returns it as a
string.

If vers_cmp is to be changed it must verify the datatype of the returned
version. If a string then verify versions as usual, if float then use > to
compare them fully numerically with . as a decimal point and not separator.
There usually is a big difference between 0.9 and 0.90 in version numbers.

Comment 5

[Henrik Nordstrom]

Attachment: Patch to use perls builtin version checks

After some additional consideration it I changed my mind. The bug is that
checksetup.pl tries to guess how to compare perl module versions where it
really should be using the builtin module version checks.

The attached patch redoes how checksetup.pl verifies the module versions to
instead use the perl built-in method.

It perhaps can be refined a little to clean up the output on error/mismatches.
Maybe a little too verbose now (perl is kind of verbose on this type of error)

Comment 6

[Todd Stansell]

For others who run into this problem, you should use the PathTools perl module,
which is where File::Spec and Cwd got combined.  Since it's version 3.09 right
now, you won't run into this ugly version issue.

Just thought I'd mention it here for anyone else like me searching for a
resolution and not knowing about PathTools.

Todd

Comment 7

[Zach Lipton [:zach]]

Perhaps the best solution here is not to change the version checking code, but
just to require PathTools instead of File::Spec. Max--any thoughts?

Comment 8

[Max Kanat-Alexander]

Actually, see bug 285700 comment 20. We were going to remove the check for
File::Spec entirely, but we didn't for some reason. We should just do that.

Comment 9

[Teemu Mannermaa (:wicked)]

(In reply to comment #8)
> Actually, see bug 285700 comment 20. We were going to remove the check for
> File::Spec entirely, but we didn't for some reason. We should just do that.

No, I'm fairly sure you didn't. Referenced comment just said that the new patch
(that actually was committed) removed something called "direct version checks"
for File::Spec v0.82 that were spread all over the place. Now only checksetup.pl
verifies the File::Spec version just like for any other required module.

So I would really like us to cure version check instead of killing symptoms.
File::Spec is not only one that has gotten problems from funky module versioning
and probably not the last one either.. There's still CGI specific hacks in
have_vers and MIME::Tools check had to be removed in bug 303482.

That builtin version check thingy sounds promising to me..

Comment 10

[Max Kanat-Alexander]

The required version of File::Spec that we need comes with perl 5.6.1. We don't
have to check for it, we already check for perl 5.6.1.

Comment 11

[Teemu Mannermaa (:wicked)]

(In reply to comment #10)
> The required version of File::Spec that we need comes with perl 5.6.1. We don't
> have to check for it, we already check for perl 5.6.1.

Are you sure File::Spec v0.82 (as said on
http://search.cpan.org/~gsar/perl-5.6.1/lib/File/Spec.pm) is really enough?
Currently we require atleast v0.84 and there was bug 285700 that says atleast to
me that 0.82 won't work..

Comment 12

[Frédéric Buclin]

*** Bug 311914 has been marked as a duplicate of this bug. ***

Comment 13

[Kevin Benton]

Comment on attachment 181192 [details] [diff] [review]
Patch to use perls builtin version checks

This patch does not work against Bugzilla 2.20 Release and causes checksetup.pl to display error messages.  It does, however, take care of the problem with requiring File::Spec no longer.

Comment 14

[Max Kanat-Alexander]

perl 5.8.0 ships with File::Spec 0.83, so we do still actually need to check for it.

Comment 15

[Max Kanat-Alexander]

Oh, and for those curious, I've put the evidence in the URL field.

Comment 16

[Marc Schumann [:Wurblzap]]

Henrik, are you interested in uploading a patch against the current CVS tip?

Comment 17

[Frédéric Buclin]

Bugzilla 3.2 and newer require Perl 5.8.1, see bug 361149, and Perl 5.8.1 is shipped with File::Spec 0.86, see http://search.cpan.org/dist/perl-5.8.1/lib/File/Spec.pm. So this check is no longer needed.

Comment 18

[Frédéric Buclin]

Attachment: patch, v1

Comment 19

[Frédéric Buclin]

Attachment: patch, v1.1

I forgot to remove related documentation.

Comment 20

[Max Kanat-Alexander]

Comment on attachment 394310 [details] [diff] [review]
patch, v1.1

Indeed! Nice catch! :-)

Comment 21

[Max Kanat-Alexander]

Might as well backport it. It fixes a bug.

Comment 22

[Frédéric Buclin]

tip:

Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.68; previous revision: 1.67
done
Checking in docs/en/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/en/xml/installation.xml,v  <--  installation.xml
new revision: 1.170; previous revision: 1.169
done
Checking in docs/en/xml/modules.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/en/xml/modules.xml,v  <--  modules.xml
new revision: 1.15; previous revision: 1.14
done


3.4.1:

Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.60.2.4; previous revision: 1.60.2.3
done
Checking in docs/en/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/en/xml/installation.xml,v  <--  installation.xml
new revision: 1.165.2.4; previous revision: 1.165.2.3
done
Checking in docs/en/xml/modules.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/en/xml/modules.xml,v  <--  modules.xml
new revision: 1.13.6.1; previous revision: 1.13
done