Skip to content

Commit

Permalink
No longer uses version.pm
Browse files Browse the repository at this point in the history
  • Loading branch information
@petdance
petdance committed Oct 4, 2019
1 parent ec343a4 commit b3c43d4
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 5 deletions.
2 changes: 2 additions & 0 deletions Changes
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ than it should be because it would skip an optimization. Now it's fixed.
Fixed test failures that would sometimes happen on Windows machines because
of taint mode. Thanks, Tomasz Konojacki. (GH #235)

Remove the use of the version.pm module.


v3.1.1 Sat Aug 31 22:56:10 CDT 2019
========================================
Expand Down
16 changes: 15 additions & 1 deletion Makefile.PL
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ my %parms = (
NAME => 'ack',
AUTHOR => 'Andy Lester <[email protected]>',
ABSTRACT => 'A grep-like program for searching source code',
VERSION_FROM => 'lib/App/Ack.pm',
VERSION => _version_from( 'lib/App/Ack.pm' ),
LICENSE => 'artistic_2',
MIN_PERL_VERSION => 5.010001,
META_MERGE => {
Expand Down Expand Up @@ -55,6 +55,20 @@ my %parms = (

WriteMakefile( %parms );

# VERSION_FROM in MakeMaker can't handle version objects.
sub _version_from {
my $file = shift;

open( my $fh, '<', $file ) or die "Can't open $file: $!";
my ($line) = grep { /^\s*\$VERSION\s+=/ } <$fh>;
close $fh;

die "Couldn't find a version line in $file" unless $line;
$line =~ /^\s*\$VERSION\s+=\s+(v3\.\d+\.\d+);/ or die "Can't parse VERSION on this line: $line";

return $1;
}

package MY;

# Suppress EU::MM test rule.
Expand Down
3 changes: 1 addition & 2 deletions ack
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@
use strict;
use warnings;

use version;
our $VERSION = version->declare( 'v3.1.1' ); # Check https://beyondgrep.com/ for updates
our $VERSION = v3.1.1; # Check https://beyondgrep.com/ for updates

use 5.010001;

Expand Down
3 changes: 1 addition & 2 deletions lib/App/Ack.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ A container for functions for the ack program.
our $VERSION;
our $COPYRIGHT;
BEGIN {
use version;
$VERSION = version->declare( 'v3.1.1' ); # Check https://beyondgrep.com/ for updates
$VERSION = v3.1.1; # Check https://beyondgrep.com/ for updates
$COPYRIGHT = 'Copyright 2005-2019 Andy Lester.';
}
our $STANDALONE = 0;
Expand Down

17 comments on commit b3c43d4

@Grinnz
Copy link

@Grinnz Grinnz commented on b3c43d4 Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not use bare vstrings as versions. Always quote them. EUMM should handle it fine.

@petdance
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always quote them.

Why?

@Grinnz
Copy link

@Grinnz Grinnz commented on b3c43d4 Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bare v-strings are inappropriate as module versions. They don't print correctly or compare numerically. A string in this form is correctly used since 5.10.1 at least. See http://blogs.perl.org/users/grinnz/2018/04/a-guide-to-versions-in-perl.html. I would instead ask, why use them, necessitating such a workaround?

@petdance
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would instead ask, why use them, necessitating such a workaround?

I wasn't debating you. I asked "why" because you said "Do X" without explanation.

@Grinnz
Copy link

@Grinnz Grinnz commented on b3c43d4 Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. It gets tiring explaining version issues after a while. Hope the blog post is helpful.

@petdance
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read the post a number of times now and I still don't understand why you're saying that the quoted strings is better.

What I'm seeing here is that $^V itself is a v-string, and that the version with the strings and string comparisons doesn't compare correctly like the v-string version does.

#!/usr/bin/perl

use warnings;
use strict;
use 5.010;

printf( "Perl %vd\n", $^V );

my $vold = v3.6.5;
my $vnew = v3.10.1;
printf( '%vd > %vd: ', $vnew, $vold );
say $vnew gt $vold ? 'Good' : 'Bad';

my $sold = 'v3.6.5';
my $snew = 'v3.10.1';
printf( '%s > %s: ', $snew, $sold );
say $snew gt $sold ? 'Good' : 'Bad';

$ perl versions.pl
Perl 5.20.3
3.10.1 > 3.6.5: Good
v3.10.1 > v3.6.5: Bad

What am I missing?

@Grinnz
Copy link

@Grinnz Grinnz commented on b3c43d4 Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$^V is not a v-string (except before 5.10), it is a version.pm object. Version objects are how you correctly compare both numerically and string. Otherwise people expect numeric comparisons to work and they don't, in either case - this is just because numeric comparisons are a mistake unless you have version.pm objects, not vstrings. The difference is that v-strings don't print, EUMM doesn't understand them (as you found), they are surprisingly unintuitive. They are turned into version.pm objects by UNIVERSAL::VERSION otherwise they wouldn't work at all. A string in the form 'vX.Y.Z' is used correctly by everything since 5.10. There is no benefit and several downsides to vstrings.

@Grinnz
Copy link

@Grinnz Grinnz commented on b3c43d4 Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If everyone only used version.pm objects and UNIVERSAL::VERSION and never touched $Package::VERSION directly, vstrings could be acceptable though still weird. But people generally expect direct printing and numerical comparison to work, and even though you can't get the latter to work unless you use version objects, a string means printing will work at least. The more consistently people use the most useful forms, the more useful they will be.

@Grinnz
Copy link

@Grinnz Grinnz commented on b3c43d4 Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also seems the vstring caused PAUSE to fail to index it as a newer version: https://cpanmeta.grinnz.com/packages?module=app%3A%3Aack&match_mode=prefix

@Grinnz
Copy link

@Grinnz Grinnz commented on b3c43d4 Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I'm not sure, that may be because the first our $VERSION line does not contain the assignment. The PAUSE email should indicate...

@petdance
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's where I'm stuck. You say that a "string in the form 'vX.Y.Z' is used correctly by everything" but yet

my $sold = 'v3.6.5';
my $snew = 'v3.10.1';
printf( '%s > %s: ', $snew, $sold );
say $snew gt $sold ? 'Good' : 'Bad';

gives me "Bad". That doesn't seem like doing the right thing.

@Grinnz
Copy link

@Grinnz Grinnz commented on b3c43d4 Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you expect string comparison to be correct for versions?

@petdance
Copy link
Collaborator Author

@petdance petdance commented on b3c43d4 Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I thought that's what the whole point of version strings was, so that we could compare v3.6.5 to v3.10.1 and have it do the right thing.

As perldata says:

A literal of the form v1.20.300.4000 is parsed as a string composed of characters with the specified ordinals. This form, known as v-strings, provides an alternative, more readable way to construct strings, rather than use the somewhat less readable interpolation form "\x{1}\x{14}\x{12c}\x{fa0}" . This is useful for representing Unicode strings, and for comparing version "numbers" using the string comparison operators, cmp, gt, lt etc.

@Grinnz
Copy link

@Grinnz Grinnz commented on b3c43d4 Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the point of vstrings, yes. I don't see how that's useful for module versions, since it's not expected, and it means other things don't work.

@petdance
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to boil this down, you're stating "You don't need the magic of vstrings in a module version, and the indexing tools get confused by them, so don't use them."

@Grinnz
Copy link

@Grinnz Grinnz commented on b3c43d4 Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, with the addition that they also confuse humans.

@petdance
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've released v3.1.3. Thanks for the pointers. See 457f3e3

Please sign in to comment.