underscore $VERSIONs currently exist in dual-life modules; should not be allowed

[p5pRT]

Migrated from rt.perl.org#131697 (status was 'open')

Searchable as RT131697$

[p5pRT]

From @karenetheridge

from irc​:

17​:29 <%ether> augh https://rt.cpan.org/Ticket/Display.html?id=122314
17​:29 < dipsy> [ Bug #122314 for CPAN-Mini-Visit​: Non-numeric version
warning ]
17​:30 <%ether> we need something in the release management guide saying
that if a dual-life module has an underscore version, then that *must* be
stripped by an eval on the next line when coring the update.
17​:30 <%ether> that is -- if we update a dual-life module in perl, we need
to patch it if it declared its $VERSION incorrectly.
17​:30 <%ether> there should also be a Porting test for this, if there are
any takers
17​:53 <%ether> this is an issue in the 5.22.4 and 5.24.2 RCs as well --
Module​::CoreList has an unevalled underscore version
18​:37 < Grinnz> that's all true, but also
https://metacpan.org/source/BINGOS/Archive-Extract-0.80/lib/Archive/Extract.pm#L818-819
should not be doing a numeric comparison on version strings
18​:37 < dipsy> [ lib/Archive/Extract.pm - metacpan.org ]
18​:40 < Grinnz> line 819 should be "if eval {
Archive​::Tar->VERSION('0.99'); 1 };" and $@​ should be localized since this
is within a module function
18​:46 <%ether> good points!
20​:26 < khw> Grinnz, please file a ticket
21​:22 < khw> ether, please file a ticket

[p5pRT]

From @jkeenan

On Mon, 03 Jul 2017 23​:50​:33 GMT, perl@​froods.org wrote​:

from irc​:

17​:29 <%ether> augh https://rt.cpan.org/Ticket/Display.html?id=122314
17​:29 < dipsy> [ Bug #122314 for CPAN-Mini-Visit​: Non-numeric version
warning ]
17​:30 <%ether> we need something in the release management guide
saying
that if a dual-life module has an underscore version, then that *must*
be
stripped by an eval on the next line when coring the update.
17​:30 <%ether> that is -- if we update a dual-life module in perl, we
need
to patch it if it declared its $VERSION incorrectly.
17​:30 <%ether> there should also be a Porting test for this, if there
are
any takers
17​:53 <%ether> this is an issue in the 5.22.4 and 5.24.2 RCs as well
--
Module​::CoreList has an unevalled underscore version

As to this part of the message ...

18​:37 < Grinnz> that's all true, but also
https://metacpan.org/source/BINGOS/Archive-Extract-
0.80/lib/Archive/Extract.pm#L818-819
should not be doing a numeric comparison on version strings
18​:37 < dipsy> [ lib/Archive/Extract.pm - metacpan.org ]
18​:40 < Grinnz> line 819 should be "if eval {
Archive​::Tar->VERSION('0.99'); 1 };" and $@​ should be localized since
this
is within a module function
18​:46 <%ether> good points!
20​:26 < khw> Grinnz, please file a ticket
21​:22 < khw> ether, please file a ticket

Archive-Tar is maintained upstream on CPAN. Hence, any bug report should be filed via email to​:

bug-archive-tar@​rt.cpan.org

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @jkeenan

On Mon, 03 Jul 2017 23​:50​:33 GMT, perl@​froods.org wrote​:

from irc​:

17​:29 <%ether> augh https://rt.cpan.org/Ticket/Display.html?id=122314
17​:29 < dipsy> [ Bug #122314 for CPAN-Mini-Visit​: Non-numeric version
warning ]
17​:30 <%ether> we need something in the release management guide
saying
that if a dual-life module has an underscore version, then that *must*
be
stripped by an eval on the next line when coring the update.

Could someone provide a grep for the relevant files? Or, better still, a patch for the requested change?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

[p5pRT]

From @petdance

17​:30 <%ether> we need something in the release management guide saying
that if a dual-life module has an underscore version, then that *must* be
stripped by an eval on the next line when coring the update.

Why are we shipping underscore versions of the dual-life modules? If users take underscore versions to mean "developer versions of modules, not released modules", then it seems it's not appropriate to include them in a Perl release.

[p5pRT]

From @tonycoz

On Mon, 03 Jul 2017 17​:36​:40 -0700, jkeenan wrote​:

Archive-Tar is maintained upstream on CPAN. Hence, any bug report
should be filed via email to​:

bug-archive-tar@​rt.cpan.org

The underscore version number for Archive​::Tar in maint was introduced for a security fix specific to maint[1]. It's not an upstream issue.

On Mon, 03 Jul 2017 21​:32​:51 -0700, petdance wrote​:

Why are we shipping underscore versions of the dual-life modules? If
users take underscore versions to mean "developer versions of modules,
not released modules", then it seems it's not appropriate to include
them in a Perl release.

This was for updates in maint releases, we don't pull in updates from CPAN for maint.

Tony

[1] by me, this was the . in @​INC issue

[p5pRT]

From @Grinnz

On Mon, Jul 3, 2017 at 7​:50 PM, Karen Etheridge <perlbug-followup@​perl.org>
wrote​:

18​:40 < Grinnz> line 819 should be "if eval {
Archive​::Tar->VERSION('0.99'); 1 };" and $@​ should be localized since this
is within a module function

I have opened jib/archive-extract#7 for this
change, FWIW.

[p5pRT]

From @karenetheridge

On Mon, 03 Jul 2017 17​:41​:50 -0700, jkeenan wrote​:

Could someone provide a grep for the relevant files? Or, better
still, a patch for the requested change?

The porting test should do something like this (pseudocode)​:

foreach $module (all modules defined in lib, dist, ext, cpan directories) {
  eval "require $module" or die "cannot load $module​: $@​";
  unlike($module->VERSION, qr/_/, "$module VERSION does not contain an underscore");
}

Affected modules should follow the $VERSION declaration with (** on a separate line **)​: $VERSION =~ tr/_//;
(also acceptable is $VERSION = eval $VERSION, or $VERSION =~ s/_//g).

These are the modules/distributions affected (in some directories there are *many* files affected)
(generated with ack -A2 'VERSION\b.*=.*_')​:

v5.22.4-RC1​:
cpan/Archive-Tar/lib/Archive/Tar/Constant.pm
cpan/Archive-Tar/lib/Archive/Tar/File.pm
cpan/Archive-Tar/lib/Archive/Tar.pm
cpan/CPAN/lib/App/Cpan.pm
cpan/Digest/Digest.pm
cpan/Digest-SHA/lib/Digest/SHA.pm
cpan/ExtUtils-Command/lib/ExtUtils/Command.pm
cpan/ExtUtils-MakeMaker/...
cpan/File-Fetch/lib/File/Fetch.pm
cpan/HTTP-Tiny/lib/HTTP/Tiny.pm
cpan/IO-Compress/...
cpan/IPC-Cmd/lib/IPC/Cmd.pm
cpan/Memoize/Memoize.pm
cpan/Sys-Syslog/Syslog.pm
cpan/Test/lib/Test.pm
cpan/Test-Harness/...
cpan/libnet/...
dist/I18N-LangTags/lib/I18N/LangTags/Detect.pm
dist/IO/IO.pm
dist/Locale-Maketext/lib/Locale/Maketext.pm
dist/Module-CoreList/...
dist/Net-Ping/lib/Net/Ping.pm
dist/Storable/Storable.pm
dist/XSLoader/XSLoader_pm.PL
dist/bignum/...
ext/Pod-Html/lib/Pod/Html.pm
ext/XS-APItest/APItest.pm

v5.24.2-RC1​:
cpan/Archive-Tar/...
cpan/CPAN/lib/App/Cpan.pm
cpan/Digest/Digest.pm
cpan/Digest-SHA/lib/Digest/SHA.pm
cpan/ExtUtils-MakeMaker/...
cpan/HTTP-Tiny/lib/HTTP/Tiny.pm
cpan/IO-Compress/...
cpan/IPC-Cmd/lib/IPC/Cmd.pm
cpan/IPC-SysV/...
cpan/JSON-PP/lib/JSON/PP.pm
cpan/Locale-Maketext-Simple/lib/Locale/Maketext/Simple.pm
cpan/Memoize/Memoize.pm
cpan/Sys-Syslog/Syslog.pm
cpan/Socket/Socket.pm
cpan/Test-Harness/...
cpan/libnet/...
dist/IO/IO.pm
dist/Locale-Maketext/lib/Locale/Maketext.pm
dist/Storable/Storable.pm
dist/Test/lib/Test.pm

blead​:
cpan/Digest/Digest.pm
cpan/Locale-Maketext-Simple/lib/Locale/Maketext/Simple.pm
cpan/Memoize/Memoize.pm
cpan/Socket/Socket.pm
lib/IO/Compress/Base/Common.pm
lib/Test/Builder/Tester/Color.pm
lib/Test/Builder/Tester.pm

[p5pRT]

From @Grinnz

On Fri, Jul 7, 2017 at 5​:14 PM, Karen Etheridge via RT <
perlbug-followup@​perl.org> wrote​:

Affected modules should follow the $VERSION declaration with (** on a
separate line **)​: $VERSION =~ tr/_//;

This should be;

$VERSION =~ tr/_//d;

[p5pRT]

From @iabyn

On Mon, Jul 03, 2017 at 04​:50​:36PM -0700, Karen Etheridge wrote​:

17​:30 <%ether> we need something in the release management guide saying
that if a dual-life module has an underscore version, then that *must* be
stripped by an eval on the next line when coring the update.

Why is having $VERSION containing an underscore a bad thing?
(I'm not saying that it isn't a bad thing, just that I don't understand
the issue).

--
Any [programming] language that doesn't occasionally surprise the
novice will pay for it by continually surprising the expert.
  -- Larry Wall

[p5pRT]

From @karenetheridge

On Mon, 10 Jul 2017 01​:14​:55 -0700, davem wrote​:

On Mon, Jul 03, 2017 at 04​:50​:36PM -0700, Karen Etheridge wrote​:

17​:30 <%ether> we need something in the release management guide saying
that if a dual-life module has an underscore version, then that *must* be
stripped by an eval on the next line when coring the update.

Why is having $VERSION containing an underscore a bad thing?
(I'm not saying that it isn't a bad thing, just that I don't understand
the issue).

It's not numeric, so code performing a comparison will generate a warning, e.g.​:

if (Foo​::Bar->VERSION < '6.0') { # VERSION is '6.01_00'
  # do some optional thing
}

Argument "6.01_01" isn't numeric in numeric lt (<) at ...

The reason for the eval or tr/// on a separate line is so static version analyzers
(most chiefly PAUSE, ExtUtils​::MakeMaker, Module​::Metadata) can pull out the version
with the underscore intact for META.* files, indexing etc, but executable code will
see the numeric form with underscores stripped.

[karenetheridge]

I have taken this ticket to review the outstanding offender(s), file tickets and pull requests for the affected modules, and it would probably also be a good idea to write a porting test prohibiting the merging of underscore-versioned dual-life modules into blead.