Item1509: version comparison fails given x.y.z format

ImagePlugin, ImageGalleryPlugin, and PublishPlugin (and surely others) produce the same error upon installation via configure:

Argument "1.1.11" isn't numeric in numeric lt (<) at /var/www/community.reefsimple.org/foswiki/tools/extender.pl line 240.

-- WillNorris - 24 Apr 2009

As written in the code, this smells:

            # SMELL: Once all modules have proper version, this should be:
            # if ( not eval { $module->VERSION( $requiredVersion ) } )
            if ( $moduleVersion < $requiredVersion ) {

A proposed fix is:

for( $requiredVersion $moduleVersion ) {
    s/(\d+\.\d+)\.(\d+)/$1$2/g
}

With nice comments, of course. And with something more clever. Maybe a bit like how perl does that. (with sprintf("%03d", $2) ).

-- OlivierRaginel - 24 Apr 2009

Restricting version tags to numeric values is obviously pretty restrictive. Most out there aren't. So they need a custom comparison function. We can only assume that version tags adhere to a consistent schema per plugin. All of them are: a series of subtags separated by "." or "-". Each subtag can be string or digits. Strings are compared lexically, digits numerically. Two version tags are compared by decomposing it into a series of subtags and iteratively compare each to the respective other subtag left-to-right.

-- MichaelDaum - 03 Jun 2009

I'm pretty sure I had something like this before:

foreach my $i ( split(/(\d+)/, $version) {
    if ($i =~ /\d+/) {
        # integer comparison
    } else {
        # lexical comparison
    }
}

Not sure what happened to it (maybe it never got checked in)

-- CrawfordCurrie - 04 Jun 2009

There is such a mechanism in configure, to choose between Upgrade, Install and Reinstall, but not in extender.pl.

And the one in configure also has issues, so...

Normally, what I fixed for this is that x.y version can be compared, and x.y.z too. But you can't mix them, nor will it work if the required version isn't numeric (but it will if the installed version isn't).

And Micha, your solution doesn't work either, as r123 is a string, but shouldn't be compared as string to r1000.

-- OlivierRaginel - 19 Jun 2009

Has anyone actually tested this recently? I see the following code in extender.pl:

        local $SIG{__WARN__} = \&check_non_perl_versions;

        # Providing 0 as version number as version checking is done below
        # and without it, perl < 5.10 won't trigger the warning
        # The eval there is used to automatically transform strings to numbers
        # so that things like '2.36_01' become 2.3601 (numeric)
        my $version = eval $module->VERSION(0);
        $moduleVersion ||= $version;

Maybe I'm missing something, but that seems to reduce to picking the first valid number out of the version string, which should be fine as long as the version number standard (using the SVN build number) is adhered to. I don't know why this is reported against PublishContrib as it uses this standard, and works fine AFAICT.

Recommend this is closed "no action" and tasks raised against any extensions that do not provide a sensible version.

-- CrawfordCurrie - 20 Jun 2009

This does not just affect extensions' versions. Here are some problematic versions that I have encountered.

• ImagePlugin depends on Image::Magick, which has a version like '6.2.4.5' (the version ImagePlugin wants) or '6.3.7' (the version installed on my machine).
• DirectedGraphPlugin depends on Digest::MD5, which has a version like '2.36_01'.
• TinyMCEPlugin depends on WysiwygPlugin, which has an SVN-based version like '$Rev: 4315 $'.

The eval that Olivier added addresses the Digest::MD5 case.

check_non_perl_versions is meant to handle SVN-based versions, and works from configure. It does not work from the command-line. This is apparently because the expected warning is not generated, and the SVN-based version string is interpreted as 0. I can get it to work on the command-line by putting use warnings at the top of extender.pl.

x.y.z format does not work from the command-line. Without use warnings, the version is empty, resulting in messages like this:

Image::Magick version 6.2.4.5 required--this is only version

With use warnings, I get the "Argument isn't numeric" error.

-- MichaelTempest - 29 Jun 2009

[11:16]   <CDot>   I have seen "1.2.4.5-beta1" and "1.2.4.5-beta2" as different versions
[11:16]   <CDot>   I have also seen "1.2.5a"

-- MichaelTempest - 29 Jun 2009

$module->VERSION($minimum) checks that the named perl module has a $VERSION greater than or equal to the specified $minimum version. extender.pl cannot use that because there are perl modules around whose $VERSION is not numeric. So extender.pl uses $module->VERSION(0) to read the module version, and relies on the comparison with zero triggering a warning, and a special $SIG{__WARN__} handler tries to extract the module version from the non-numeric $VERSION in the warning message.

I would rather use ${"${module}::VERSION"}, which also reads the module version, but with less magic, because there are no comparisons. Then, I would rather use an explicit regular expression like this

$moduleVersion =~ s/\$Rev: (\d+) \$/$1/;

to remove SVN's markup from the $VERSION for modules in SVN, without magically discarding information from other non-numeric version numbers.

I would encapsulate the comparison in a function like this:

if ( compare_versions($moduleVersion, '<', $dep->{version}) ) {

so that we can separate comparisons from the processing that uses the comparisons. The comparison function may normalise both versions. The problem that Will reported is that the version in DEPENDENCIES is not numeric.

We would also be able to unit-test the comparison function.

Finally, I have a comparison function that splits each version number into numeric and non-numeric parts, formats the numeric parts with leading zeros and the non-numeric parts with trailing spaces, and then joins them together again. The result is versions in string form that compare correctly.

• extender.pl.txt: Proposed solution (30 June 2009)

-- MichaelTempest - 30 Jun 2009

This looks very fine to me (just a few left-overs from my unsuccessful tries like the eval to change 1.2.3 into ^A^B^C which only compares to 1.2.2 but not to 1.2).

To be honest, I first thought what I wrote was just a little bit better than what was there, and that ideally, all modules should be perl modules, and therefore should use ->VERSION.

Now that I've seen the real world, perl core (and version.pm) do some pretty complex things especially for that.

I think your proposed solution is much more robust, and should also be incorporated into Configure (Crawford did some side-by-side checking split on . iirc).

And as you mentionned, this can be unit-tested, and I think that's the real beauty of it. So that anybody can add its own twisted minded version checking test and see if it breaks your algorithm

So please, go ahead and commit that, once you've written the unit tests to prove the world it does a much better job than what I wrote

-- OlivierRaginel - 30 Jun 2009

I've committed the change to extender.pl on trunk, along with some unit tests. I'll look at configure next. I'm not sure if this change should go into the release branch, so I changed the TargetRelease to minor.

-- MichaelTempest - 01 Jul 2009

Tested the version-comparisons on the command-line with locally-built releases of these extensions, on trunk, at SVN revision 4343, with perl 5.10.0 on ubuntu:

• PublishPlugin
• ImagePlugin
• ImageGalleryPlugin
• DirectedGraphPlugin
• DirectedGraphWebMapPlugin
• ActionTrackerPlugin
• BehaviourContrib
• BreadCrumbsPlugin
• BuildContrib
• CommentPlugin
• EditTablePlugin
• FamFamFamContrib
• FilterPlugin
• FlexWebListPlugin
• GluePlugin
• IfDefinedPlugin
• InterwikiPlugin
• JSCalendarContrib
• MailerContrib
• MathModePlugin
• MeetingBingoPlugin
• PatternSkin - first had to patch its DEPENDENCIES because it depends on TWiki::Plugins::TablePlugin and TWiki::Plugins::TwistyPlugin, but both of those plugins are in the Foswiki namespace now.
• SafeWikiPlugin
• SlideShowPlugin
• SmiliesPlugin
• SpreadsheetPlugin
• TablePlugin
• TestFixturePlugin - first had to fix build.pl to use the correct name for Foswiki::Contrib::Build
• TinyMCEPlugin - with locally-modified DEPENDENCIES

To be checked again:

• TipsContrib - depends on Foswiki::Plugins::SpreadsheetPlugin, but does not specify a version, and extender.pl does not handle this as well as it could.

I found the version-comparison code in lib/Foswiki/Configure/UIs/EXTENSIONS.pm. That code compares "dd Mmm yyyy"-style versions correctly, which mine does not. I want to fix extender.pl to deal with that case, too.

I agree with Olivier that the version-comparison code in extender.pl and EXTENSIONS.pm should be consolidated. That would mean that one of them must be able to "use" the other, or that the version comparison code must be moved to a shared module. Which way round is best?

-- MichaelTempest - 01 Jul 2009

extender.pl now handles "dd Mmm yyyy", "dd-mm-yyyy" and "X.Y-beta". I will try to make EXTENSIONS.pm use extender.pl.

Tested the version-comparisons on the command-line with locally-built releases of these extensions, on trunk, extender.pl at SVN revision 4384, with perl 5.10.0 on ubuntu:

• TipsContrib
• TestFixturePlugin
• TablePlugin
• TinyMCEPlugin
• WysiwygPlugin
• PublishPlugin
• ImagePlugin
• ImageGalleryPlugin
• DirectedGraphPlugin
• DirectedGraphWebMapPlugin
• ActionTrackerPlugin

Tested the version-comparisons on the command-line with locally-built releases of these extensions from trunk, on release branch, with trunk's revision 4384 of extender.pl:

• TinyMCEPlugin
• ImagePlugin
• ImageGalleryPlugin
• PublishPlugin
• SafeWikiPlugin

Installed NatEditPlugin via configure on release branch, with trunk's extender.pl. configure produced error like this:

Bad Request

Bad request (malformed multipart POST)

and then at the end, reported that the "Installer ran without errors". The output from configure is attached:

• nateditplugin_configure_output.html: Output from configure, when I installed NatEditPlugin on the release branch using configure, with extender.pl copied from trunk

This does not seem right to me, but I do not know what configure's output is supposed to look like since I do not normally use it to install plugins.

-- MichaelTempest - 02 Jul 2009

"Bad request" is already captured at Item8084, and existed before my change, so I am not addressing it as part of this bugfix.

I checked in a much-more conservative patch on the release branch. Trunk is certainly more robust in general, but it may have odd cases where it fails and the previous version didn't. The algorithm on the release branch is conceptually closer to the previous algorithm than to the algorithm used on trunk.

-- MichaelTempest - 04 Jul 2009