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

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

Open
p5pRT opened this issue Jul 3, 2017 · 12 comments
Open
Assignees

Comments

@p5pRT
Copy link

p5pRT commented Jul 3, 2017

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

Searchable as RT131697$

@p5pRT
Copy link
Author

p5pRT commented Jul 3, 2017

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
Copy link
Author

p5pRT commented Jul 4, 2017

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
Copy link
Author

p5pRT commented Jul 4, 2017

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

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2017

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
Copy link
Author

p5pRT commented Jul 4, 2017

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
Copy link
Author

p5pRT commented Jul 4, 2017

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
Copy link
Author

p5pRT commented Jul 4, 2017

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
Copy link
Author

p5pRT commented Jul 7, 2017

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
Copy link
Author

p5pRT commented Jul 7, 2017

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
Copy link
Author

p5pRT commented Jul 10, 2017

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
Copy link
Author

p5pRT commented Jul 10, 2017

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 karenetheridge self-assigned this Jan 31, 2020
@karenetheridge
Copy link
Member

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.

@xenu xenu removed the Severity Low label Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants