Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

checkconfig.pl: don't re-invent version comparison #2412

Merged
merged 2 commits into from Apr 19, 2019

Conversation

Projects
None yet
3 participants
@nfagerlund
Copy link
Contributor

commented Apr 8, 2019

I tried to get a dev environment running during a period where the latest
version of List::Util was 1.5. 1.5 may be larger than 1.45, but 45 is larger
than 5, so the config check failed.

Could just fix the bug and then wait to independently rediscover the next
version comparison gotcha, but I propose Let's Don't.

checkconfig.pl: don't re-invent version comparison
I tried to get a dev environment running during a period where the latest
version of List::Util was `1.5`. 1.5 may be larger than 1.45, but 45 is larger
than 5, so the config check failed.

Could just fix the bug and then wait to independently rediscover the next
version comparison gotcha, but I propose Let's Don't.
@alierak

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

The current version of List::Util is 1.50, and 50 is greater than 45. See https://metacpan.org/changes/distribution/Scalar-List-Utils . Are you saying this was somehow misinterpreted as 1.5 instead of 1.50 before version comparison?

@nfagerlund

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Ha, no kidding. Well, be that as it may, if I add a debug statement to the old impl:

        if ( $ver_want && $ver_got ) {
            print "is $mod $ver_got version $ver_want or higher?\n";
            my @parts_want = split( /\./, $ver_want );
            my @parts_got  = split( /\./, $ver_got  );
            my $invalid = 0;
            # ...
         }

I get:

dw@dw-dev-server:~/dw$ ./bin/checkconfig.pl
[Checking Timezone...]
[Checking for Perl Modules....]
is Hash::MultiValue 0.16 version 0.10 or higher?
is JSON 2.90 version 2.53 or higher?
is List::Util 1.5 version 1.45 or higher?
is Locale::Country 3.34 version 3.32 or higher?
is MogileFS::Client 1.17 version 1.12 or higher?
is Net::SSL 2.88 version 2.85 or higher?
Paws::S3 is not stable / supported / entirely developed at /usr/local/share/perl/5.22.1/Paws/S3.pm line 2.
is SOAP::Lite 1.19 version 0.710.8 or higher?
is Text::Fuzzy 0.28 version 0.27 or higher?
is Text::Wrap 2013.0523 version 2013.0523 or higher?
is XML::Simple 2.22 version 2.12 or higher?

Problem:
  * Out of date module: List::Util (need 1.45, 1.5 installed)

So the installed versions are making a str->num->str round-trip at some point (and the declared dependencies aren't), and it's only through luck that this hasn't bit someone before. A great illustration of why store-bought version comparison tastes better than homemade.

@alierak

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

I'm sorry, if the module reports its own version incorrectly with the ->VERSION method, what implementation are you suggesting in checkconfig.pl would be able to compare it correctly?

@nfagerlund

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

The one in the PR works great! So does any sensible version comparison library I've met. Have you tried the code in the PR? I went ahead and tested asking for 1.60 with it too, and that worked as expected as well.

edit: apologies, I was confused when I wrote this! @alierak is right that what List::Util is doing is bizarre. But since version->parse() handles it just fine, it seems likely it's a locally expected and planned-for flavor of bizarre.

@nfagerlund

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

https://github.com/Dual-Life/Scalar-List-Utils/blob/1acee612da2548514000946e91382256dbac47df/lib/Scalar/Util.pm#L20

our $VERSION    = "1.50";
$VERSION = eval $VERSION;

...

#!/usr/bin/env perl

my $thing = eval "1.50";
print "version $thing\n";
╚ᐅ /Users/nick/Desktop/junk.pl
version 1.5

Screen Shot 2019-04-08 at Apr 8, 9 01AM

@alierak

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Okay. I guess I just don't understand how it would be okay to cover up the problem that the module reports its version incorrectly by changing how we compare it. If the purpose of the PR is do away with wheels we've reinvented, then I would rather see a change that does the equivalent of "use List::Util 1.45" instead of retrieving and comparing the incorrect version number, no matter the method of comparison.

I think I'm probably missing something here and will stop commenting on this PR now. Thanks.

@nfagerlund

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Ah — I was operating on the premise that 1.50 ≈ 1.5 wasn't "incorrect," since that's how CPAN and the rest of the module toolchain was interpreting it. (I think they use the same version object or a copy/pasted version of it?)

Coming from a non-perl world, I would expect version numbers to go from, e.g., 1.9 to 1.10, but from looking at this module's tags, it looks like it handled that ordering by releasing 1.09 instead... and I'm kind of new here, so I'm basing my understanding of perl's versioning conventions based on what I'm seeing here and what CPAN itself does. But I'd say that if CPAN thinks 1.5 is newer than 1.45, probably anything that uses CPAN modules should also think so?

@alierak

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

You know what, you're right. Perl module version numbers 1.50 and 1.5 are equivalent, and 14 years ago this code just did a simple numeric comparison of the version. Probably what happened later on was that we used some module with the v1.2.3 format, and needed a quick fix (the "parts" code, which turns out to do the wrong thing) before version.pm was widely available or built in.

Except for stylistic tweaks (needs spaces inside parens), your patch should be fine. Sorry about that!

Show resolved Hide resolved bin/checkconfig.pl Outdated
@nfagerlund

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

...ha, I hadn't realized it, but it looks like the last four PRs behind this one have failing travis builds because of exactly this issue.

@alierak alierak merged commit f97ee3d into dreamwidth:develop Apr 19, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@nfagerlund nfagerlund deleted the nfagerlund:apr19_checkconfig branch Apr 19, 2019

@nfagerlund

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

sweet, I'll rebase those next two so they have green travis checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.