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

Perl 5.18.1 breaks Devel::Cover #13176

Closed
p5pRT opened this issue Aug 18, 2013 · 18 comments
Closed

Perl 5.18.1 breaks Devel::Cover #13176

p5pRT opened this issue Aug 18, 2013 · 18 comments

Comments

@p5pRT
Copy link

p5pRT commented Aug 18, 2013

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

Searchable as RT119351$

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2013

From @jkeenan

At some point between Perl 5.18.0 and 5.18.1, Devel​::Cover began to fail
most of its tests.

Here are the CPAN Testers Perl/Platform version matrices for
Devel​::Cover 1.06 and those two versions of Perl​:

http​://matrix.cpantesters.org/?dist=Devel-Cover%201.06;perl=5.18.0;reports=1

http​://matrix.cpantesters.org/?dist=Devel-Cover%201.06;perl=5.18.1;reports=1

With one unusual exception, D​::C 1.06 has built and tested successfully
on Perl 5.18.0. In addition to the platforms listed there, yesterday I
successfully built, tested and installed D​::C 1.06 on Perl 5.18.0 on the
same Darwin/PPC machine on which I have installed D​::C for many years.
(I had not yet upgraded to Perl 5.18.1 on that machine.)

However, when we turn to the matrix for Perl 5.18.1, we see D​::C failing
across the board. I haven't inspected all the failure reports, but for
those I have I observe​:

* They are the same failures I got yesterday when trying to install D​::C
1.06 on top of a new Perl 5.18.1 on the same Linode (i686) on which I
have installed both Perl and D​::C for many years.

* They are the same failures seen in the one, unusual failure report for
5.18.0 noted above
(http​://www.cpantesters.org/cpan/report/792ad9d4-04b5-11e3-b7c0-ec8abfc46414).

When diagnosing this yesterday, I tried to install a slightly older
version of D​::C (1.02) on Perl 5.18.1 on the Linode. I know that this
version worked with Perl 5.18.0 because I had installed it there earlier
(but I had to remove the 5.18.0 for other reasons). D​::C 1.02 failed on
5.18.1 just as D​::C 1.06 does. So I inferred that the problem lies in
changes to Perl rather than changes to Devel​::Cover.

This has been reported in D-C's bug queue​:
pjcj/Devel--Cover#66

In that report, the poster notes what I also observed​: On top of
5.18.1, none of the cover_db/ databases are being written to. This
would explain why, in the individual failure reports, the "Got"
(left-hand) side of the output chart shows blanks where the "Expected"
(right-hand) side of the chart shows coverage percentages.

Can someone with a big enough box try at a 'git bisect' on this?

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2013

From @jkeenan

Summary of my perl5 (revision 5 version 18 subversion 1) configuration​:
 
  Platform​:
  osname=linux, osvers=2.6.18.8-linode22, archname=i686-linux
  uname='linux li11-226 2.6.18.8-linode22 #1 smp tue nov 10 16​:12​:12 utc 2009 i686 gnulinux '
  config_args='-des'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=undef, usemultiplicity=undef
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=undef, use64bitall=undef, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cc', ccflags ='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
  optimize='-O2',
  cppflags='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
  ccversion='', gccversion='4.3.2', gccosandvers=''
  intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
  ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
  alignbytes=4, prototype=define
  Linker and Libraries​:
  ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
  libpth=/usr/local/lib /lib/../lib /usr/lib/../lib /lib /usr/lib /usr/lib64
  libs=-lnsl -lgdbm -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
  perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
  libc=/lib/libc-2.7.so, so=so, useshrplib=false, libperl=libperl.a
  gnulibc_version='2.7'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
  cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'

Characteristics of this binary (from libperl)​:
  Compile-time options​: HAS_TIMES PERLIO_LAYERS PERL_DONT_CREATE_GVSV
  PERL_HASH_FUNC_ONE_AT_A_TIME_HARD PERL_MALLOC_WRAP
  PERL_PRESERVE_IVUV PERL_SAWAMPERSAND USE_LARGE_FILES
  USE_LOCALE USE_LOCALE_COLLATE USE_LOCALE_CTYPE
  USE_LOCALE_NUMERIC USE_PERLIO USE_PERL_ATOF
  Built under linux
  Compiled at Aug 17 2013 19​:40​:43
  @​INC​:
  /usr/local/lib/perl5/site_perl/5.18.1/i686-linux
  /usr/local/lib/perl5/site_perl/5.18.1
  /usr/local/lib/perl5/5.18.1/i686-linux
  /usr/local/lib/perl5/5.18.1
  .

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2013

From @pjcj

On Sun, Aug 18, 2013 at 06​:09​:56AM -0700, James E Keenan wrote​:

At some point between Perl 5.18.0 and 5.18.1, Devel​::Cover began to fail
most of its tests.

In fact, all tests which had to do with actually collecting any coverage
data.

I'm very sorry that I haven't had much time recently to look at
Devel​::Cover, and especially that I wasn't able to test the RCs for
5.18.1.

I've managed to create a minimal test case​:

$ perl5.18.0 -MB -E 'say B​::main_cv->GV // "undef"'
B​::SPECIAL=SCALAR(0x7b58e0)
$ perl5.18.1 -MB -E 'say B​::main_cv->GV // "undef"'
undef

Jim and I think we have identified the offending commit. It is​:

commit e6c4c33
Author​: Tony Cook <tony@​develop-help.com>
Date​: Thu Jul 25 12​:09​:00 2013 +1000

  CvGV is no longer a simple struct member access
 
  The same slot is also used for the NAME_HEK for lexical subs, so​:
 
  - split B​::CV​::GV out into its own function that uses the CvGV macro
 
  - add B​::CV​::NAME_HEK so the name of a lexical sub can be fetched

Inline Patch
diff --git a/ext/B/B.pm b/ext/B/B.pm
index 8856a32..bd2cf66 100644
--- a/ext/B/B.pm
+++ b/ext/B/B.pm
@@ -15,7 +15,7 @@ require Exporter;
 # walkoptree comes from B.xs
 
 BEGIN {
-    $B::VERSION = '1.42';
+    $B::VERSION = '1.42_01';
     @B::EXPORT_OK = ();
 
     # Our BOOT code needs $VERSION set, and will append to @EXPORT_OK.
@@ -1014,6 +1014,10 @@ For constant subroutines, returns the constant SV returned by the subroutine.
 
 =item const_sv
 
+=item NAME_HEK
+
+Returns the name of a lexical sub, otherwise C<undef>.
+
 =back
 
 =head2 B::HV Methods
diff --git a/ext/B/B.xs b/ext/B/B.xs
index e2ebdad..d933a56 100644
--- a/ext/B/B.xs
+++ b/ext/B/B.xs
@@ -1380,7 +1380,6 @@ IVX(sv)
        B::IO::IoFLAGS = PVIO_flags_ix
        B::AV::MAX = PVAV_max_ix
        B::CV::STASH = PVCV_stash_ix
-       B::CV::GV = PVCV_gv_ix
        B::CV::FILE = PVCV_file_ix
        B::CV::OUTSIDE = PVCV_outside_ix
        B::CV::OUTSIDE_SEQ = PVCV_outside_seq_ix
@@ -1873,6 +1872,27 @@ const_sv(cv)
     PPCODE:
        PUSHs(make_sv_object(aTHX_ (SV *)cv_const_sv(cv)));
 
+void
+GV(cv)
+       B::CV cv
+    PREINIT:
+        GV *gv;
+    CODE:
+       gv = CvGV(cv);
+       ST(0) = gv ? make_sv_object((SV*)gv) : &PL_sv_undef;
+
+#if PERL_VERSION > 17
+
+SV *
+NAME_HEK(cv)
+       B::CV cv
+    CODE:
+       RETVAL = CvNAMED(cv) ? newSVhek(CvNAME_HEK(cv)) : &PL_sv_undef;
+    OUTPUT:
+       RETVAL
+
+#endif
+
 MODULE = B     PACKAGE = B::HV         PREFIX = Hv
 
 STRLEN
diff --git a/ext/B/t/b.t b/ext/B/t/b.t
index a065375..d58d2e0 100644
--- a/ext/B/t/b.t
+++ b/ext/B/t/b.t
@@ -376,4 +376,43 @@ SKIP: {
     is($op->name, "leavesub", "overlay: orig name");
 }
 
+{ # [perl #118525]
+    {
+        sub foo {}
+       my $cv = B::svref_2object(\&foo);
+       ok($cv, "make a B::CV from a non-anon sub reference");
+       isa_ok($cv, "B::CV");
+       my $gv = $cv->GV;
+       ok($gv, "we get a GV from a GV on a normal sub");
+       isa_ok($gv, "B::GV");
+       is($gv->NAME, "foo", "check the GV name");
+      SKIP:
+       { # do we need these version checks?
+           skip "no HEK before 5.18", 1 if $] < 5.018;
+           is($cv->NAME_HEK, undef, "no hek for a global sub");
+       }
+    }
+
+SKIP:
+    {
+        skip "no HEK before 5.18", 4 if $] < 5.018;
+        eval <<'EOS'
+    {
+        use feature 'lexical_subs';
+        no warnings 'experimental::lexical_subs';
+        my sub bar {};
+        my $cv = B::svref_2object(\&bar);
+        ok($cv, "make a B::CV from a lexical sub reference");
+        isa_ok($cv, "B::CV");
+        my $gv = $cv->GV;
+        is($gv, undef, "GV on a lexical sub is NULL");
+        my $hek = $cv->NAME_HEK;
+        is($hek, "bar", "check the NAME_HEK");
+    }
+    1;
+EOS
+         or die "lexical_subs test failed to compile: $@";
+    }
+}
+
 done_testing();

-- 

Paul Johnson - paul@​pjcj.net
http​://www.pjcj.net

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2013

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

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2013

From @jkeenan

On Sun Aug 18 08​:53​:37 2013, paul@​pjcj.net wrote​:

On Sun, Aug 18, 2013 at 06​:09​:56AM -0700, James E Keenan wrote​:

At some point between Perl 5.18.0 and 5.18.1, Devel​::Cover began to
fail
most of its tests.

In fact, all tests which had to do with actually collecting any
coverage
data.

I'm very sorry that I haven't had much time recently to look at
Devel​::Cover, and especially that I wasn't able to test the RCs for
5.18.1.

I've managed to create a minimal test case​:

$ perl5.18.0 -MB -E 'say B​::main_cv->GV // "undef"'
B​::SPECIAL=SCALAR(0x7b58e0)
$ perl5.18.1 -MB -E 'say B​::main_cv->GV // "undef"'
undef

Jim and I think we have identified the offending commit. It is​:

commit e6c4c33
Author​: Tony Cook <tony@​develop-help.com>
Date​: Thu Jul 25 12​:09​:00 2013 +1000

CvGV is no longer a simple struct member access

The same slot is also used for the NAME\_HEK for lexical subs\, so&#8203;:

\- split B&#8203;::CV&#8203;::GV out into its own function that uses the CvGV

macro

\- add B&#8203;::CV&#8203;::NAME\_HEK so the name of a lexical sub can be fetched

If this indeed is the commit associated with the D​::C test failures,
then I have to wonder, "Why was this included in a maintenance release
(the '.1' in 5.18.1)? There's nothing to indicate that it was
correcting a regression in 5.18.0. Looks more like a change in a feature."

Of course, by its very nature, Devel​::Cover has to muck about in the
internals, so it's inherently vulnerable to this sort of thing.

This will affect how we address this problem. If the problematic commit
should not have been included in the maintenance release, but is
nonetheless something desirable for Perl going forward, then the commit
can remain in 5.19 and Devel​::Cover will have to adapt to it during
5.19's development and definitely by 5.20. But that implies that the
problematic commit ought to be removed in 5.18.2.

If, however, the problematic commit was needed to correct a regression,
then Devel​::Cover will have to adapt right away.

(Of course, all the above is true regardless of whether e6c4c33 is the
problematic commit or some other commit is.)

How should we proceed?

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2013

From @pjcj

On Sun, Aug 18, 2013 at 09​:49​:46AM -0700, James E Keenan via RT wrote​:

On Sun Aug 18 08​:53​:37 2013, paul@​pjcj.net wrote​:

On Sun, Aug 18, 2013 at 06​:09​:56AM -0700, James E Keenan wrote​:

$ perl5.18.0 -MB -E 'say B​::main_cv->GV // "undef"'
B​::SPECIAL=SCALAR(0x7b58e0)
$ perl5.18.1 -MB -E 'say B​::main_cv->GV // "undef"'
undef

Jim and I think we have identified the offending commit. It is​:

commit e6c4c33
Author​: Tony Cook <tony@​develop-help.com>
Date​: Thu Jul 25 12​:09​:00 2013 +1000

CvGV is no longer a simple struct member access

The same slot is also used for the NAME\_HEK for lexical subs\, so&#8203;:

\- split B&#8203;::CV&#8203;::GV out into its own function that uses the CvGV

macro

\- add B&#8203;::CV&#8203;::NAME\_HEK so the name of a lexical sub can be fetched

If this indeed is the commit associated with the D​::C test failures,
then I have to wonder, "Why was this included in a maintenance release
(the '.1' in 5.18.1)? There's nothing to indicate that it was
correcting a regression in 5.18.0. Looks more like a change in a feature."

I think the reason is detailed in 323602a​:

+L<B> has been upgraded from version 1.44 to 1.45.
+
+Calling the C<GV> method on C<B​::CV> objects created from a lexical
+sub would return nonsense, possibly crashing perl. C<GV> now returns
+C<undef> for lexical subs. [perl #118525]
+
+Added the C<NAME_HEK> method to return the name of a lexical sub.</NAME></undef></GV></B>

So my guess is that what we have here is probably a buggy bug fix. What
do you think, Tony?

--
Paul Johnson - paul@​pjcj.net
http​://www.pjcj.net

@p5pRT
Copy link
Author

p5pRT commented Aug 18, 2013

From @cpansprout

On Sun Aug 18 08​:53​:37 2013, paul@​pjcj.net wrote​:

On Sun, Aug 18, 2013 at 06​:09​:56AM -0700, James E Keenan wrote​:

At some point between Perl 5.18.0 and 5.18.1, Devel​::Cover began to
fail
most of its tests.

In fact, all tests which had to do with actually collecting any
coverage
data.

I'm very sorry that I haven't had much time recently to look at
Devel​::Cover, and especially that I wasn't able to test the RCs for
5.18.1.

I've managed to create a minimal test case​:

$ perl5.18.0 -MB -E 'say B​::main_cv->GV // "undef"'
B​::SPECIAL=SCALAR(0x7b58e0)
$ perl5.18.1 -MB -E 'say B​::main_cv->GV // "undef"'
undef

Youch! I find B’s interface so arcane it’s very hard to miss things
like that. In general, if something internally returns NULL, should the
perl interface return undef or B​::SPECIAL? If I recall correctly, in
the past I’ve seen both, and different perl versions randomly switching
back and forth between them.

Jim and I think we have identified the offending commit. It is​:

commit e6c4c33
Author​: Tony Cook <tony@​develop-help.com>
Date​: Thu Jul 25 12​:09​:00 2013 +1000

You can blame it all on me. He was following my recommendations.

It is worth changing this back? Any code depending on it will now have
to take undef into account for 5.18.1. Changing it to B​::SPECIAL (for
main_cv and lexical subs) in 5.18.2 could be considered unhelpful churn.
I don’t know the best solution here.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 2013

From @pjcj

On Sun, Aug 18, 2013 at 10​:52​:35AM -0700, Father Chrysostomos via RT wrote​:

On Sun Aug 18 08​:53​:37 2013, paul@​pjcj.net wrote​:

$ perl5.18.0 -MB -E 'say B​::main_cv->GV // "undef"'
B​::SPECIAL=SCALAR(0x7b58e0)
$ perl5.18.1 -MB -E 'say B​::main_cv->GV // "undef"'
undef

Youch! I find B’s interface so arcane it’s very hard to miss things
like that. In general, if something internally returns NULL, should the
perl interface return undef or B​::SPECIAL? If I recall correctly, in
the past I’ve seen both, and different perl versions randomly switching
back and forth between them.

In this particular case, we're consistent with B​::SPECIAL all the way
back to 5.6.1.

Jim and I think we have identified the offending commit. It is​:

commit e6c4c33
Author​: Tony Cook <tony@​develop-help.com>
Date​: Thu Jul 25 12​:09​:00 2013 +1000

You can blame it all on me. He was following my recommendations.

It is worth changing this back? Any code depending on it will now have
to take undef into account for 5.18.1. Changing it to B​::SPECIAL (for
main_cv and lexical subs) in 5.18.2 could be considered unhelpful churn.
I don’t know the best solution here.

As far as Devel​::Cover is concerned, I really don't mind. I have a
patch to manage the undef, so it's no extra work for me either way.

However, countering your argument about churn is the argument that
5.18.1 is still relatively new. Few people will have upgraded, and it
might be prudent to get a 5.18.2 out fairly quickly so that most people
won't even notice the problem.

Then again, does anyone know of any code apart from Devel​::Cover which
works on 5.18.0 but fails on 5.18.1 for this reason?

--
Paul Johnson - paul@​pjcj.net
http​://www.pjcj.net

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 2013

From @tonycoz

On Sun, Aug 18, 2013 at 10​:52​:35AM -0700, Father Chrysostomos via RT wrote​:

On Sun Aug 18 08​:53​:37 2013, paul@​pjcj.net wrote​:

Jim and I think we have identified the offending commit. It is​:

commit e6c4c33
Author​: Tony Cook <tony@​develop-help.com>
Date​: Thu Jul 25 12​:09​:00 2013 +1000

You can blame it all on me. He was following my recommendations.

I think I was confused by a comment in #118525 that a B​::NULL was the
expected result, then couldn't reproduce it, or think of a way to make
CvGV return NULL before lexical subs were introduced.

It is worth changing this back? Any code depending on it will now have
to take undef into account for 5.18.1. Changing it to B​::SPECIAL (for
main_cv and lexical subs) in 5.18.2 could be considered unhelpful churn.
I don’t know the best solution here.

I agree with this, but have no problems changing it back.

Tony

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 2013

From @tonycoz

On Tue, Aug 20, 2013 at 12​:57​:14AM +0200, Paul Johnson wrote​:

As far as Devel​::Cover is concerned, I really don't mind. I have a
patch to manage the undef, so it's no extra work for me either way.

However, countering your argument about churn is the argument that
5.18.1 is still relatively new. Few people will have upgraded, and it
might be prudent to get a 5.18.2 out fairly quickly so that most people
won't even notice the problem.

Then again, does anyone know of any code apart from Devel​::Cover which
works on 5.18.0 but fails on 5.18.1 for this reason?

The same change in behaviour is in blead, and I haven't seen a BBC
report for the corresponding commit.

Tony

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 2013

From @jkeenan

On Mon Aug 19 15​:57​:54 2013, paul@​pjcj.net wrote​:
.

However, countering your argument about churn is the argument that
5.18.1 is still relatively new. Few people will have upgraded, and it
might be prudent to get a 5.18.2 out fairly quickly so that most people
won't even notice the problem.

If we have a consensus that we introduced a bug between 5.18.0 and
5.18.2, then an early 5.18.2 is called for, IMHO.

Then again, does anyone know of any code apart from Devel​::Cover which
works on 5.18.0 but fails on 5.18.1 for this reason?

Well, as you pointed out, "Few people will have upgraded." So that
means we're early in the process of getting bug reports specific to
5.18.1. But the longer we leave the bug in, the more people will start
to (mal-)adapt to it.

But, speaking for myself, I hate being without Devel​::Cover on any
machine I touch -- and I am without it on my Linode now.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2013

From zefram@fysh.org

Paul Johnson wrote​:

However, countering your argument about churn is the argument that
5.18.1 is still relatively new. Few people will have upgraded, and it
might be prudent to get a 5.18.2 out fairly quickly so that most people
won't even notice the problem.

This seems wise. I reckon the change of representation is a bug.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Aug 21, 2013

From @ntyni

On Tue, Aug 20, 2013 at 12​:57​:14AM +0200, Paul Johnson wrote​:

On Sun, Aug 18, 2013 at 10​:52​:35AM -0700, Father Chrysostomos via RT wrote​:

On Sun Aug 18 08​:53​:37 2013, paul@​pjcj.net wrote​:

Jim and I think we have identified the offending commit. It is​:

commit e6c4c33
Author​: Tony Cook <tony@​develop-help.com>
Date​: Thu Jul 25 12​:09​:00 2013 +1000

You can blame it all on me. He was following my recommendations.

It is worth changing this back? Any code depending on it will now have
to take undef into account for 5.18.1. Changing it to B​::SPECIAL (for
main_cv and lexical subs) in 5.18.2 could be considered unhelpful churn.
I don’t know the best solution here.

As far as Devel​::Cover is concerned, I really don't mind. I have a
patch to manage the undef, so it's no extra work for me either way.

However, countering your argument about churn is the argument that
5.18.1 is still relatively new. Few people will have upgraded, and it
might be prudent to get a 5.18.2 out fairly quickly so that most people
won't even notice the problem.

Hi,

as it happens, we're switching Debian unstable/testing to 5.18.1 any day
now (see <http​://bugs.debian.org/712615>) . We'll certainly upgrade to
5.18.2 promptly if it's released soonish, but it would be nice to have
a working Devel​::Cover in the meantime.

Paul, could you please make your patch available somewhere, even if it's
just an interim solution?

Thanks for your work,
--
Niko Tyni ntyni@​debian.org

@p5pRT
Copy link
Author

p5pRT commented Aug 21, 2013

From @pjcj

On Wed, Aug 21, 2013 at 10​:21​:23PM +0300, Niko Tyni wrote​:

as it happens, we're switching Debian unstable/testing to 5.18.1 any day
now (see <http​://bugs.debian.org/712615>) . We'll certainly upgrade to
5.18.2 promptly if it's released soonish, but it would be nice to have
a working Devel​::Cover in the meantime.

Paul, could you please make your patch available somewhere, even if it's
just an interim solution?

I've just released Devel​::Cover 1.07 which should work correctly on
every stable version of perl from 5.6.1 to 5.18.1.

The tests fail on 5.19.2 and 5.19.3 due to changed line numbers, but I
still think this is a perl problem and I will follow up on it
separately.

--
Paul Johnson - paul@​pjcj.net
http​://www.pjcj.net

@p5pRT
Copy link
Author

p5pRT commented Aug 21, 2013

From @jkeenan

On Wed Aug 21 15​:30​:23 2013, paul@​pjcj.net wrote​:

On Wed, Aug 21, 2013 at 10​:21​:23PM +0300, Niko Tyni wrote​:

as it happens, we're switching Debian unstable/testing to 5.18.1 any day
now (see <http​://bugs.debian.org/712615>) . We'll certainly upgrade to
5.18.2 promptly if it's released soonish, but it would be nice to have
a working Devel​::Cover in the meantime.

Paul, could you please make your patch available somewhere, even if it's
just an interim solution?

I've just released Devel​::Cover 1.07 which should work correctly on
every stable version of perl from 5.6.1 to 5.18.1.

And it does indeed work on Perl 5.18.1. I just now installed D-C 1.07
on the same machine where my latest Perl is 5.18.1 and where D-C 1.06
failed this weekend.

The tests fail on 5.19.2 and 5.19.3 due to changed line numbers, but I
still think this is a perl problem and I will follow up on it
separately.

Since the complaint in the OP has been addressed, I'm marking this
ticket resolved. However, I will open a new RT asking for a
reconsideration of the commit in Perl Paul has identified as the likely
source of the problem. Please post comments about that commit -- e.g.,
was it indeed a "buggy bug fix" in that new RT.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Aug 21, 2013

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

@p5pRT p5pRT closed this as completed Aug 21, 2013
@p5pRT
Copy link
Author

p5pRT commented Aug 22, 2013

From @andk

Paul Johnson <paul@​pjcj.net> writes​:

The tests fail on 5.19.2 and 5.19.3 due to changed line numbers, but I
still think this is a perl problem and I will follow up on it
separately.

Same as https://rt-archive.perl.org/perl5/Ticket/Display.html?id=118931

--
andreas

@p5pRT
Copy link
Author

p5pRT commented Aug 22, 2013

From @nwc10

On Tue, Aug 20, 2013 at 09​:08​:08AM +1000, Tony Cook wrote​:

On Sun, Aug 18, 2013 at 10​:52​:35AM -0700, Father Chrysostomos via RT wrote​:

On Sun Aug 18 08​:53​:37 2013, paul@​pjcj.net wrote​:

Jim and I think we have identified the offending commit. It is​:

commit e6c4c33
Author​: Tony Cook <tony@​develop-help.com>
Date​: Thu Jul 25 12​:09​:00 2013 +1000

You can blame it all on me. He was following my recommendations.

I think I was confused by a comment in #118525 that a B​::NULL was the
expected result, then couldn't reproduce it, or think of a way to make
CvGV return NULL before lexical subs were introduced.

It is worth changing this back? Any code depending on it will now have
to take undef into account for 5.18.1. Changing it to B​::SPECIAL (for
main_cv and lexical subs) in 5.18.2 could be considered unhelpful churn.
I don't know the best solution here.

I agree with this, but have no problems changing it back.

Code which wants to work with v5.18.0 and v5.18.1 already has to deal with
two possible results. So from that point of view, I don't think that it adds
any complication switching back to the previous result, because it doesn't
add to the complexity of the userbase.

If B​::SPECIAL is the "right" answer (both because it was the previous result
for a NULL CvGV, and because it's consistent with what other methods do),
then that's also a reason to change it.

To me, it doesn't look like churn. It looks like a beneficial bugfix.

Nicholas Clark

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

No branches or pull requests

1 participant