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

Some version strings in %Module::CoreList::delta contain trailing space and cause warning with version->parse #14666

Closed
p5pRT opened this issue Apr 23, 2015 · 13 comments

Comments

@p5pRT
Copy link

p5pRT commented Apr 23, 2015

Migrated from rt.perl.org#124364 (status was 'resolved')

Searchable as RT124364$

@p5pRT
Copy link
Author

p5pRT commented Apr 23, 2015

From perlancar@cpan.org

Some version strings in %Module​::CoreList​::delta contain trailing space and cause warning with version->parse, e.g. there is one '1.15 ' which when fed to version->parse under warnings​:

% perl -wE'say version->parse("1.15 ")'
Version string '1.15 ' contains invalid data; ignoring​: ' ' at -e line 1.
1.15

Complete list​:

% perl -MModule​::CoreList -E'my $d = \%Module​::CoreList​::delta; for my $rel (keys %$d) { my $rd = $d->{$rel}; for my $t (keys %$rd) { my $h = $rd->{$t}; for my $mod (keys %$h) { my $v = $h->{$mod}; say "$rel -> $t -> $mod​: <$v>" if $v =~ /\s/ } } }'
5.007003 -> changed -> CPAN​::FirstTime​: <1.54 >
5.008001 -> changed -> CPAN​::FirstTime​: <1.60 >
5.00504 -> changed -> ExtUtils​::Liblist​: <1.25 >
5.00504 -> changed -> CPAN​::FirstTime​: <1.36 >
5.00504 -> changed -> ExtUtils​::Install​: <1.28 >
5.00504 -> changed -> ExtUtils​::Manifest​: <1.33 >
5.00504 -> changed -> ExtUtils​::MM_Unix​: <1.12602 >
5.00504 -> changed -> ExtUtils​::Mkbootstrap​: <1.14 >
5.00504 -> changed -> ExtUtils​::testlib​: <1.11 >
5.00504 -> changed -> ExtUtils​::Mksymlists​: <1.17 >
5.00307 -> changed -> ExtUtils​::Mksymlists​: <1.12 >
5.00307 -> changed -> ExtUtils​::MM_Unix​: <1.107 >
5.00307 -> changed -> ExtUtils​::Mkbootstrap​: <1.13 >
5.00307 -> changed -> ExtUtils​::testlib​: <1.11 >
5.00307 -> changed -> ExtUtils​::Liblist​: <1.20 >
5.00307 -> changed -> ExtUtils​::Install​: <1.15 >
5.008 -> changed -> CPAN​::FirstTime​: <1.56 >
5.004 -> changed -> ExtUtils​::Mksymlists​: <1.13 >
5.004 -> changed -> CPAN​::FirstTime​: <1.18 >
5.004 -> changed -> ExtUtils​::Manifest​: <1.33 >
5.004 -> changed -> ExtUtils​::Liblist​: <1.2201 >
5.004 -> changed -> ExtUtils​::MM_Unix​: <1.114 >
5.004 -> changed -> ExtUtils​::Install​: <1.16 >
5.006002 -> changed -> CPAN​::FirstTime​: <1.53 >
5.008000 -> changed -> CPAN​::FirstTime​: <1.56 >
5.00405 -> changed -> ExtUtils​::MM_Unix​: <1.118 >
5.00405 -> changed -> ExtUtils​::Mkbootstrap​: <1.14 >
5.00405 -> changed -> ExtUtils​::Install​: <1.28 >
5.00405 -> changed -> ExtUtils​::Liblist​: <1.25 >
5.00405 -> changed -> ExtUtils​::Mksymlists​: <1.16 >
5.00405 -> changed -> CPAN​::FirstTime​: <1.30 >
5.006000 -> changed -> CPAN​::FirstTime​: <1.38 >
5.006000 -> changed -> ExtUtils​::MM_Unix​: <1.12603 >
5.005 -> changed -> ExtUtils​::MM_Unix​: <1.12601 >
5.005 -> changed -> ExtUtils​::Mkbootstrap​: <1.13 >
5.005 -> changed -> CPAN​::FirstTime​: <1.29 >
5.005 -> changed -> ExtUtils​::Mksymlists​: <1.17 >
5.006 -> changed -> CPAN​::FirstTime​: <1.38 >
5.006 -> changed -> ExtUtils​::MM_Unix​: <1.12603 >

@p5pRT
Copy link
Author

p5pRT commented Apr 23, 2015

From @rjbs

* perlancar <perlbug-followup@​perl.org> [2015-04-23T02​:15​:31]

Some version strings in %Module​::CoreList​::delta contain trailing space and
cause warning with version->parse, e.g. there is one '1.15 ' which when fed
to version->parse under warnings​:

% perl -wE'say version->parse("1.15 ")'
Version string '1.15 ' contains invalid data; ignoring​: ' ' at -e line 1.
1.15

Thanks, I believe I have just fixed all this with b7c1f08.

I see that you probably found this while working on Module​::CoreList​::More,
which contains, among other things, a "faster is_core." Is there a reason we
shouldn't use your implementation in Module​::CoreList?

It seems a shame to leave it in an add-on module that many users would never
see.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Apr 23, 2015

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

@p5pRT
Copy link
Author

p5pRT commented Apr 24, 2015

From @perlancar

I'd love to get it integrated, but we should probably get at least a couple
of other people looking at the code first to check its correctness first.

On Thu, Apr 23, 2015 at 7​:24 PM, Ricardo Signes via RT <
perlbug-followup@​perl.org> wrote​:

* perlancar <perlbug-followup@​perl.org> [2015-04-23T02​:15​:31]

Some version strings in %Module​::CoreList​::delta contain trailing space
and
cause warning with version->parse, e.g. there is one '1.15 ' which when
fed
to version->parse under warnings​:

% perl -wE'say version->parse("1.15 ")'
Version string '1.15 ' contains invalid data; ignoring​: ' ' at -e line
1.
1.15

Thanks, I believe I have just fixed all this with b7c1f08.

I see that you probably found this while working on Module​::CoreList​::More,
which contains, among other things, a "faster is_core." Is there a reason
we
shouldn't use your implementation in Module​::CoreList?

It seems a shame to leave it in an add-on module that many users would
never
see.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Apr 26, 2015

From @jkeenan

On Thu Apr 23 21​:45​:43 2015, perlancar@​gmail.com wrote​:

I'd love to get it integrated, but we should probably get at least a
couple
of other people looking at the code first to check its correctness
first.

On Thu, Apr 23, 2015 at 7​:24 PM, Ricardo Signes via RT <
perlbug-followup@​perl.org> wrote​:

* perlancar <perlbug-followup@​perl.org> [2015-04-23T02​:15​:31]

Some version strings in %Module​::CoreList​::delta contain trailing
space
and
cause warning with version->parse, e.g. there is one '1.15 ' which
when
fed
to version->parse under warnings​:

% perl -wE'say version->parse("1.15 ")'
Version string '1.15 ' contains invalid data; ignoring​: ' ' at -e
line
1.
1.15

Thanks, I believe I have just fixed all this with b7c1f08.

I see that you probably found this while working on
Module​::CoreList​::More,
which contains, among other things, a "faster is_core." Is there a
reason
we
shouldn't use your implementation in Module​::CoreList?

It seems a shame to leave it in an add-on module that many users
would
never
see.

--
rjbs

I've created a project to evaluate Module-CoreList-More at the New York Perl Hackathon, coming up on May 2.

https://github.com/nyperlmongers/nyperlhackathon2015/wiki/Evaluate-Module-CoreList-More

So perhaps we'll get some eyeballs on this there.

Thank you very much.

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

@p5pRT
Copy link
Author

p5pRT commented Apr 27, 2015

From @haarg

On Wed Apr 22 23​:15​:30 2015, perlancar@​cpan.org wrote​:

Some version strings in %Module​::CoreList​::delta contain trailing
space and cause warning with version->parse, e.g. there is one '1.15 '
which when fed to version->parse under warnings​:

% perl -wE'say version->parse("1.15 ")'
Version string '1.15 ' contains invalid data; ignoring​: ' ' at -e line
1.
1.15

Complete list​:

% perl -MModule​::CoreList -E'my $d = \%Module​::CoreList​::delta; for my
$rel (keys %$d) { my $rd = $d->{$rel}; for my $t (keys %$rd) { my $h =
$rd->{$t}; for my $mod (keys %$h) { my $v = $h->{$mod}; say "$rel ->
$t -> $mod​: <$v>" if $v =~ /\s/ } } }'
5.007003 -> changed -> CPAN​::FirstTime​: <1.54 >
5.008001 -> changed -> CPAN​::FirstTime​: <1.60 >
5.00504 -> changed -> ExtUtils​::Liblist​: <1.25 >
5.00504 -> changed -> CPAN​::FirstTime​: <1.36 >
5.00504 -> changed -> ExtUtils​::Install​: <1.28 >
5.00504 -> changed -> ExtUtils​::Manifest​: <1.33 >
5.00504 -> changed -> ExtUtils​::MM_Unix​: <1.12602 >
5.00504 -> changed -> ExtUtils​::Mkbootstrap​: <1.14 >
5.00504 -> changed -> ExtUtils​::testlib​: <1.11 >
5.00504 -> changed -> ExtUtils​::Mksymlists​: <1.17 >
5.00307 -> changed -> ExtUtils​::Mksymlists​: <1.12 >
5.00307 -> changed -> ExtUtils​::MM_Unix​: <1.107 >
5.00307 -> changed -> ExtUtils​::Mkbootstrap​: <1.13 >
5.00307 -> changed -> ExtUtils​::testlib​: <1.11 >
5.00307 -> changed -> ExtUtils​::Liblist​: <1.20 >
5.00307 -> changed -> ExtUtils​::Install​: <1.15 >
5.008 -> changed -> CPAN​::FirstTime​: <1.56 >
5.004 -> changed -> ExtUtils​::Mksymlists​: <1.13 >
5.004 -> changed -> CPAN​::FirstTime​: <1.18 >
5.004 -> changed -> ExtUtils​::Manifest​: <1.33 >
5.004 -> changed -> ExtUtils​::Liblist​: <1.2201 >
5.004 -> changed -> ExtUtils​::MM_Unix​: <1.114 >
5.004 -> changed -> ExtUtils​::Install​: <1.16 >
5.006002 -> changed -> CPAN​::FirstTime​: <1.53 >
5.008000 -> changed -> CPAN​::FirstTime​: <1.56 >
5.00405 -> changed -> ExtUtils​::MM_Unix​: <1.118 >
5.00405 -> changed -> ExtUtils​::Mkbootstrap​: <1.14 >
5.00405 -> changed -> ExtUtils​::Install​: <1.28 >
5.00405 -> changed -> ExtUtils​::Liblist​: <1.25 >
5.00405 -> changed -> ExtUtils​::Mksymlists​: <1.16 >
5.00405 -> changed -> CPAN​::FirstTime​: <1.30 >
5.006000 -> changed -> CPAN​::FirstTime​: <1.38 >
5.006000 -> changed -> ExtUtils​::MM_Unix​: <1.12603 >
5.005 -> changed -> ExtUtils​::MM_Unix​: <1.12601 >
5.005 -> changed -> ExtUtils​::Mkbootstrap​: <1.13 >
5.005 -> changed -> CPAN​::FirstTime​: <1.29 >
5.005 -> changed -> ExtUtils​::Mksymlists​: <1.17 >
5.006 -> changed -> CPAN​::FirstTime​: <1.38 >
5.006 -> changed -> ExtUtils​::MM_Unix​: <1.12603 >

These versions with spaces are an accurate representation of what the versions were defined as. As an example, in perl 5.006, lib/ExtUtils/MM_Unix.pm, line 11​:

$VERSION = substr q$Revision​: 1.12603 $, 10;

This leaves a trailing space.

@p5pRT
Copy link
Author

p5pRT commented May 1, 2015

From @Leont

On Mon, Apr 27, 2015 at 4​:27 PM, Graham Knop via RT <
perlbug-followup@​perl.org> wrote​:

These versions with spaces are an accurate representation of what the
versions were defined as. As an example, in perl 5.006,
lib/ExtUtils/MM_Unix.pm, line 11​:

$VERSION = substr q$Revision​: 1.12603 $, 10;

This leaves a trailing space.

I would consider that an unhelpful artifact. Historically correct as it may
be, leaving it in only causes headaches and doesn't solve any problem I can
imagine.

Leon

@p5pRT
Copy link
Author

p5pRT commented May 2, 2015

From spam-bitcard@yary.ack.org

These versions with spaces are an accurate representation of what the
versions were defined as. As an example, in perl 5.006,
lib/ExtUtils/MM_Unix.pm, line 11​:

$VERSION = substr q$Revision​: 1.12603 $, 10;

This leaves a trailing space.

I would consider that an unhelpful artifact. Historically correct as it may
be, leaving it in only causes headaches and doesn't solve any problem I can
imagine.

The "version.pm" docs say that the above looks like a CVS revision, and "CVS $Revision$ increments differently from Decimal versions (i.e. 1.10 follows 1.9), so it must be handled as if it were a Dotted-Decimal Version" like "v1.12603"

which leads me to believe that a large number of version strings in Module​::CoreList should have a leading "v" on them so they will compare correctly. For example 'Module​::Pluggable' is at version '3.8', if it were to go to '3.10' then this would be a problem, whereas 'v3.10' would compare properly against 'v3.8'.

On the other hand, when I look at the actual version numbers, the authors seem to be sane about this.

That's beyond what I can do in today's Hackathon, but I will have a smaller pull request by the end of the day. It will also handle "CGI​::Fast" which has a letter in its version "1.00a", and letters are just plain wrong, according to version.pm. Full comments will be in the pull request.

@p5pRT
Copy link
Author

p5pRT commented May 2, 2015

From spam-bitcard@yary.ack.org

I've added a pull request to perl-Module-CoreList-More via github

Basically, the trailing space could probably be stripped from the core Module​::CoreList, but I don't think it's needed. As for Module​::CoreList​::Core handling these, I submitted a patch to strip the trailing space (and handle other trailing junk which version.pm complains about). Also this patch resumes using "warnings" which might alert us to issues in the future. The patch also adds a test to look for warnings.

And I think Module-CoreList-More could be folded into Module-CoreList. I would like to factor out common code from is_core/is_still_core, and list_core_modules/list_still_core_modules, but that's not a big deal.

@p5pRT
Copy link
Author

p5pRT commented May 2, 2015

From [Unknown Contact. See original ticket]

I've added a pull request to perl-Module-CoreList-More via github

Basically, the trailing space could probably be stripped from the core Module​::CoreList, but I don't think it's needed. As for Module​::CoreList​::Core handling these, I submitted a patch to strip the trailing space (and handle other trailing junk which version.pm complains about). Also this patch resumes using "warnings" which might alert us to issues in the future. The patch also adds a test to look for warnings.

And I think Module-CoreList-More could be folded into Module-CoreList. I would like to factor out common code from is_core/is_still_core, and list_core_modules/list_still_core_modules, but that's not a big deal.

@p5pRT
Copy link
Author

p5pRT commented Dec 9, 2015

From @jkeenan

On Thu Apr 23 05​:24​:18 2015, perl.p5p@​rjbs.manxome.org wrote​:

* perlancar <perlbug-followup@​perl.org> [2015-04-23T02​:15​:31]

Some version strings in %Module​::CoreList​::delta contain trailing
space and
cause warning with version->parse, e.g. there is one '1.15 ' which
when fed
to version->parse under warnings​:

% perl -wE'say version->parse("1.15 ")'
Version string '1.15 ' contains invalid data; ignoring​: ' ' at -e
line 1.
1.15

Thanks, I believe I have just fixed all this with b7c1f08.

I see that you probably found this while working on
Module​::CoreList​::More,
which contains, among other things, a "faster is_core." Is there a
reason we
shouldn't use your implementation in Module​::CoreList?

It seems a shame to leave it in an add-on module that many users would
never
see.

rjbs​: If we've addressed the problem which the OP reported, then I'd recommend closing this ticket and moving any discussion about a revision to Module-CoreList to a new RT.

Thank you very much.

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

@p5pRT
Copy link
Author

p5pRT commented Dec 14, 2015

From @rjbs

So resolved.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Dec 14, 2015

@rjbs - Status changed from 'open' to 'resolved'

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

1 participant