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

[PATCH] 4e51d69 Fixed bug where is_core assumed linear release sequence #13301

Open
p5pRT opened this issue Sep 22, 2013 · 6 comments
Open

[PATCH] 4e51d69 Fixed bug where is_core assumed linear release sequence #13301

p5pRT opened this issue Sep 22, 2013 · 6 comments
Labels

Comments

@p5pRT
Copy link

p5pRT commented Sep 22, 2013

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

Searchable as RT119945$

@p5pRT
Copy link
Author

p5pRT commented Sep 22, 2013

From @neilb

Somewhat embarrassingly my first core module patch had a bug in it​: if you specify a minimum version for the module, I traverse through releases, seeing if an acceptable version was released upto and including the specified release of Perl. But I was traversing the sequence of releases as if they were a linear sequence.

This fix now traverse the branch on the release tree leading to the specified version of perl.
An example where this resulted in the wrong behaviour, for Text​::Soundex is illustrated in this blog post​:

http​://neilb.org/2013/09/21/adding-is-core.html

Added tests for this example, and a couple more tests for good measure.

Neil

@p5pRT
Copy link
Author

p5pRT commented Sep 22, 2013

From @neilb

0001-Fixed-bug-where-is_core-assumed-linear-release-seque.patch
From 4e51d697cef10ca7b1dfa1c76767bb69d2053dc2 Mon Sep 17 00:00:00 2001
From: Neil Bowers <neil@bowers.com>
Date: Sun, 22 Sep 2013 23:56:24 +0100
Subject: [PATCH] Fixed bug where is_core assumed linear release sequence
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------1.7.6"

This is a multi-part message in MIME format.
--------------1.7.6
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


If you specified a version of the module, is_core has to track through
releases, as the %delta data structure only records where a module
version number changes in core, not every module version number in every release.
I was naively trawling the releases in numerical order, but %delta includes
information that let's you construct the release tree.

This fix only traverses the branch of the overall release tree that leads
to the specified Perl release. Further explanation and example in blog post:

    http://neilb.org/2013/09/21/adding-is-core.html
---
 dist/Module-CoreList/Changes                |    2 ++
 dist/Module-CoreList/lib/Module/CoreList.pm |   15 ++++++++++++---
 dist/Module-CoreList/t/is_core.t            |   17 ++++++++++++++++-
 3 files changed, 30 insertions(+), 4 deletions(-)


--------------1.7.6
Content-Type: text/x-patch; name="0001-Fixed-bug-where-is_core-assumed-linear-release-seque.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="0001-Fixed-bug-where-is_core-assumed-linear-release-seque.patch"

diff --git a/dist/Module-CoreList/Changes b/dist/Module-CoreList/Changes
index ff87f1b..66742c7 100644
--- a/dist/Module-CoreList/Changes
+++ b/dist/Module-CoreList/Changes
@@ -1,6 +1,8 @@
 3.00
   - Prepared for v5.19.5
   - exported %delta
+  - fixed bug in is_core(): it was naively assuming a linear sequence of releases,
+    rather than the tree with multiple branches.
 
 2.99 Fri Sep 20 2013
   - Updated for v5.19.4
diff --git a/dist/Module-CoreList/lib/Module/CoreList.pm b/dist/Module-CoreList/lib/Module/CoreList.pm
index e316afc..1ec4350 100644
--- a/dist/Module-CoreList/lib/Module/CoreList.pm
+++ b/dist/Module-CoreList/lib/Module/CoreList.pm
@@ -8842,14 +8842,23 @@ sub is_core
     return 0 if defined($final_release) && $perl_version > $final_release;
 
     # If a minimum version of the module was specified:
-    # Step through all perl release numbers ($prn)
-    # in order, so we can find what version of the module
+    # Step through all perl releases ($prn)
+    # so we can find what version of the module
     # was included in the specified version of perl.
     # On the way if we pass the required module version, we can
     # short-circuit and return true
     if (defined($module_version)) {
+        # The Perl releases aren't a linear sequence, but a tree. We need to build the path
+        # of releases from 5 to the specified release, and follow the module's version(s)
+        # along that path.
+        my @releases = ($perl_version);
+        my $rel = $perl_version;
+        while (defined($rel)) {
+            $rel = $delta{$rel}->{delta_from};
+            unshift(@releases, $rel) if defined($rel);
+        }
         RELEASE:
-        foreach my $prn (sort keys %delta) {
+        foreach my $prn (@releases) {
             next RELEASE if $prn <= $first_release;
             last RELEASE if $prn > $perl_version;
             next unless defined(my $next_module_version
diff --git a/dist/Module-CoreList/t/is_core.t b/dist/Module-CoreList/t/is_core.t
index a145315..712221f 100644
--- a/dist/Module-CoreList/t/is_core.t
+++ b/dist/Module-CoreList/t/is_core.t
@@ -1,7 +1,7 @@
 #!perl -w
 use strict;
 use Module::CoreList;
-use Test::More tests => 23;
+use Test::More tests => 33;
 
 BEGIN { require_ok('Module::CoreList'); }
 
@@ -50,3 +50,18 @@ ok(Module::CoreList->is_core('encoding', '2.01', '5.008007'), "encoding 2.01 was
 ok(!Module::CoreList::is_core('Module::CoreList', undef, '5.007003'), "Module::CoreList wasn't core in perl 5.7.3");
 ok(!Module::CoreList->is_core('Module::CoreList', undef, '5.007003'), "Module::CoreList wasn't core in perl 5.7.3 (class method)");
 
+# Test for situations where different branches on the perl
+# release tree had different versions of a module, and a naive
+# checking of perl release number will trip you up
+ok(Module::CoreList->is_core('Text::Soundex', '1.01', '5.008007'), "Text::Soundex 1.01 was first included in 5.007003");
+ok(Module::CoreList->is_core('Text::Soundex', '3.03', '5.008009'), "Text::Soundex 3.03 was included in 5.008009");
+ok(!Module::CoreList->is_core('Text::Soundex', '3.03', '5.009003'), "5.009003 still had Text::Soundex 1.01");
+ok(Module::CoreList->is_core('Text::Soundex', '1.01', '5.009003'), "5.009003 still had Text::Soundex 1.01");
+ok(!Module::CoreList->is_core('Text::Soundex', '3.03', '5.009005'), "5.009005 still had Text::Soundex 3.02");
+ok(Module::CoreList->is_core('Text::Soundex', '3.02', '5.009005'), "5.009005 had Text::Soundex 3.02");
+ok(Module::CoreList->is_core('Text::Soundex', '3.03', '5.01'), "5.01 had Text::Soundex 3.03");
+
+# 5.002 was the first perl release where core modules had a version number
+ok(Module::CoreList->is_core('DB_File', '1.01', '5.002'), "DB_File 1.01 was included in 5.002");
+ok(!Module::CoreList->is_core('DB_File', '1.03', '5.002'), "DB_File 1.03 wasn't included in 5.002");
+ok(Module::CoreList->is_core('DB_File', '1.03', '5.00307'), "DB_File 1.03 was included in 5.00307");

--------------1.7.6--


@p5pRT
Copy link
Author

p5pRT commented Sep 23, 2013

From @bingos

On Sun, Sep 22, 2013 at 04​:05​:14PM -0700, Neil Bowers wrote​:

# New Ticket Created by Neil Bowers
# Please include the string​: [perl #119945]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=119945 >

Somewhat embarrassingly my first core module patch had a bug in it​: if you specify a minimum version for the module, I traverse through releases, seeing if an acceptable version was released upto and including the specified release of Perl. But I was traversing the sequence of releases as if they were a linear sequence.

This fix now traverse the branch on the release tree leading to the specified version of perl.
An example where this resulted in the wrong behaviour, for Text​::Soundex is illustrated in this blog post​:

http​://neilb.org/2013/09/21/adding-is-core.html

Added tests for this example, and a couple more tests for good measure.

Neil

Thanks, applied as 717ace6

http​://perl5.git.perl.org/perl.git/commitdiff/717ace6eb4102762ffd65f965ff32b26947eacc9

--
Chris Williams
aka BinGOs
PGP ID 0x4658671F
http​://www.gumbynet.org.uk

@p5pRT
Copy link
Author

p5pRT commented Sep 23, 2013

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

@p5pRT
Copy link
Author

p5pRT commented Sep 23, 2013

From @jkeenan

On Sun Sep 22 16​:05​:13 2013, neil.bowers@​cogendo.com wrote​:

Somewhat embarrassingly my first core module patch had a bug in it​: if
you specify a minimum version for the module, I traverse through
releases, seeing if an acceptable version was released upto and
including the specified release of Perl. But I was traversing the
sequence of releases as if they were a linear sequence.

This fix now traverse the branch on the release tree leading to the
specified version of perl.
An example where this resulted in the wrong behaviour, for
Text​::Soundex is illustrated in this blog post​:

1. The patch appears sound. All tests PASS. I'll defer to maintainer
bingos on actually applying it.

http​://neilb.org/2013/09/21/adding-is-core.html

2. A very good blog post!

Added tests for this example, and a couple more tests for good
measure.

Neil

3. Could I ask that you re-break line 8830 in
dist/Module-CoreList/lib/Module/CoreList.pm?

##########
8829 my $module = shift;
8830 $module = shift if eval { $module->isa(__PACKAGE__) } && @​_ >
0 && defined($_[0]) && $_[0] =~ /^\w/;
8831 my ($module_version, $perl_version);
##########

Its length, combined with the absence of (admittedly optional) parens,
makes it difficult to grasp immediately, IMO.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Sep 24, 2013

From @neilb

Hi Jim,

1. The patch appears sound. All tests PASS. I'll defer to maintainer
bingos on actually applying it.

He did. Phew :-)

2. A very good blog post!

Thank you.

3. Could I ask that you re-break line 8830 in
dist/Module-CoreList/lib/Module/CoreList.pm?

8830 $module = shift if eval { $module->isa(__PACKAGE__) } && @​_ >
0 && defined($_[0]) && $_[0] =~ /^\w/;

Its length, combined with the absence of (admittedly optional) parens,
makes it difficult to grasp immediately, IMO.

Good point.

I'm slowly working on something that might result in another patch for Module​::CoreList, in which case I'll make this change at the same time (given the patch has already been applied).

Cheers,
Neil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants