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

Bleadperl v5.25.5-43-g607ee43 breaks CDOLAN/Devel-EnforceEncapsulation-0.51.tar.gz #15650

Closed
p5pRT opened this issue Oct 11, 2016 · 16 comments
Closed
Labels
BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) distro-All hasPatch hasTest type-library

Comments

@p5pRT
Copy link

p5pRT commented Oct 11, 2016

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

Searchable as RT129851$

@p5pRT
Copy link
Author

p5pRT commented Oct 11, 2016

From @dcollinsn

While smoking CPAN modules against various versions of Perl, I noticed the
following unexpected failure​:


Bisects to​:

bad - non-zero exit from /tmp/lX36LYqWLD/bin/perl -I /home/cpanbisect/.cpan
-MCPAN​::MyConfig -MCPAN -e $CPAN​::Config->{build_dir}=q{/tmp/ZTufR37XGi};
-e install('Devel​::EnforceEncapsulation'); die unless
CPAN​::Shell->expand(Module => 'Devel​::EnforceEncapsulation')->uptodate;
607ee43 is the first bad commit
commit 607ee43
Author​: James Raspass <jraspass@​gmail.com>
Date​: Wed Jul 15 23​:46​:20 2015 +0100

  Speed up compilation of overload.pm a smidge.

  Measured with the following crude perl script calling perf. Perl
  is in there to get a rough baseline cost of starting perl​:

  print 'PERL', (`perf stat -r100 perl -e 1 2>&1`)[10];
  print 'OLD ', (`perf stat -r100 perl lib/overload.pm 2>&1`)[10];
  print 'NEW ', (`perf stat -r100 perl lib/overload2.pm 2>&1`)[10];

  Produced the following results on my machine​:

  PERL 5,800,051 instructions # 1.05 insns per cycle ( +- 0.06% )
  OLD 14,818,995 instructions # 1.16 insns per cycle ( +- 0.03% )
  NEW 14,696,974 instructions # 1.16 insns per cycle ( +- 0.03% )

  While the numbers did fluctuate between runs, the new code was
  consistently faster.

:040000 040000 e92b68c47128f987038e0a5c2c4b99d361a9561e
a587e32369a751d18c0dcf05b2f5a7fca27830ef M lib
bisect run success
That took 2162 seconds.


Diagnostics attached. Output buffering makes it look like some messages are
coming from the wrong place, here's the "true" output of t/api​:

t/api.t ........... 1/52 overload arg '${}' is invalid at (eval 14) line 1.
overload arg '@​{}' is invalid at (eval 14) line 1.
overload arg '%{}' is invalid at (eval 14) line 1.
overload arg '&{}' is invalid at (eval 14) line 1.
overload arg '*{}' is invalid at (eval 14) line 1.
overload arg 'fallback' is invalid at (eval 14) line 1.

# Failed test 'no warning on unencapsulated class'
# at t/api.t line 153.
# got​: '6'
# expected​: '0'

# Failed test '...but it does raise a warning'
# at t/api.t line 159.
# got​: '7'
# expected​: '1'
# Looks like you failed 2 tests of 52.
t/api.t ........... Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/52 subtests


Perl -V of a failing perl​:

Summary of my perl5 (revision 5 version 25 subversion 6) configuration​:
  Commit id​: 5654fe3
  Platform​:
  osname=linux
  osvers=3.16.0-4-amd64
  archname=x86_64-linux-thread-multi
  uname='linux digitalis 3.16.0-4-amd64 #1 smp debian 3.16.36-1+deb8u1
(2016-09-03) x86_64 gnulinux '
  config_args='-Dusedevel -DDEBUGGING -Dusethreads
-Dprefix=/home/cpan2/install -Uversiononly -des'
  hint=recommended
  useposix=true
  d_sigaction=define
  useithreads=define
  usemultiplicity=define
  use64bitint=define
  use64bitall=define
  uselongdouble=undef
  usemymalloc=n
  bincompat5005=undef
  Compiler​:
  cc='cc'
  ccflags ='-D_REENTRANT -D_GNU_SOURCE -fwrapv -DDEBUGGING
-fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2'
  optimize='-O2 -g'
  cppflags='-D_REENTRANT -D_GNU_SOURCE -fwrapv -DDEBUGGING
-fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
  ccversion=''
  gccversion='4.9.2'
  gccosandvers=''
  intsize=4
  longsize=8
  ptrsize=8
  doublesize=8
  byteorder=12345678
  doublekind=3
  d_longlong=define
  longlongsize=8
  d_longdbl=define
  longdblsize=16
  longdblkind=3
  ivtype='long'
  ivsize=8
  nvtype='double'
  nvsize=8
  Off_t='off_t'
  lseeksize=8
  alignbytes=8
  prototype=define
  Linker and Libraries​:
  ld='cc'
  ldflags =' -fstack-protector-strong -L/usr/local/lib'
  libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/4.9/include-fixed
/usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib
/usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
  libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.19.so
  so=so
  useshrplib=false
  libperl=libperl.a
  gnulibc_version='2.19'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs
  dlext=so
  d_dlsymun=undef
  ccdlflags='-Wl,-E'
  cccdlflags='-fPIC'
  lddlflags='-shared -O2 -g -L/usr/local/lib -fstack-protector-strong'

Characteristics of this binary (from libperl)​:
  Compile-time options​:
  DEBUGGING
  HAS_TIMES
  MULTIPLICITY
  PERLIO_LAYERS
  PERL_COPY_ON_WRITE
  PERL_DONT_CREATE_GVSV
  PERL_HASH_FUNC_ONE_AT_A_TIME_HARD
  PERL_IMPLICIT_CONTEXT
  PERL_MALLOC_WRAP
  PERL_OP_PARENT
  PERL_PRESERVE_IVUV
  PERL_TRACK_MEMPOOL
  PERL_USE_DEVEL
  USE_64_BIT_ALL
  USE_64_BIT_INT
  USE_ITHREADS
  USE_LARGE_FILES
  USE_LOCALE
  USE_LOCALE_COLLATE
  USE_LOCALE_CTYPE
  USE_LOCALE_NUMERIC
  USE_LOCALE_TIME
  USE_PERLIO
  USE_PERL_ATOF
  USE_REENTRANT_API
  Built under linux
  Compiled at Sep 29 2016 16​:06​:12
  %ENV​:
  PERLBREW_BASHRC_VERSION="0.76"
  PERLBREW_HOME="/home/dcollins/.perlbrew"
  PERLBREW_ROOT="/home/dcollins/toolchain/perl5"
  @​INC​:
  /home/cpan2/install/lib/perl5/site_perl/5.25.6/x86_64-linux-thread-multi
  /home/cpan2/install/lib/perl5/site_perl/5.25.6
  /home/cpan2/install/lib/perl5/5.25.6/x86_64-linux-thread-multi
  /home/cpan2/install/lib/perl5/5.25.6
  .


Happy to provide any other information that may be helpful. Respectfully,
Dan Collins

@p5pRT
Copy link
Author

p5pRT commented Oct 11, 2016

From @dcollinsn

I have added James Raspass to the CC list, as he is the original author of the patch. James, this ticket describes a regression introduced by the patch you emailed to the list, which was exposed by a CPAN module called Devel​::EnforceEncapsulation. I'd appreciate any diagnosis you can provide.

--
Respectfully,
Dan Collins

@p5pRT
Copy link
Author

p5pRT commented Oct 11, 2016

From @dcollinsn

Found it. Patch forthcoming, once I write a test.

--
Respectfully,
Dan Collins

@p5pRT
Copy link
Author

p5pRT commented Oct 11, 2016

From [Unknown Contact. See original ticket]

Found it. Patch forthcoming, once I write a test.

--
Respectfully,
Dan Collins

@p5pRT
Copy link
Author

p5pRT commented Oct 11, 2016

From @dcollinsn

The problem was that the patch changed the "ops list" hash from having '1' as the value to having 'undef'. This was checked in two places in order to determine if a warning should be issued - but only one of them was changed. I've attached a patch to correct the bug, and to test for it.

--
Respectfully,
Dan Collins

@p5pRT
Copy link
Author

p5pRT commented Oct 11, 2016

From @dcollinsn

0001-RT-129851-overload.pm-add-a-missing-exists.patch
From 2505c9c45649eed83cc03eb1e008cbfbd1fef9f9 Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Tue, 11 Oct 2016 14:54:16 -0400
Subject: [PATCH] [RT #129851] overload.pm: add a missing 'exists'

607ee4356 changed the hash of permitted ops from having '1' as a value
to having undef as a value. This also changed one of the warning points
from checking for truthiness to existence. However, a second warning
was accidentally left checking for truthiness. This commit fixes that
oversight, and adds a regression test for warnings in this case.
---
 lib/overload.pm           |  2 +-
 t/lib/overload_fallback.t | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/lib/overload.pm b/lib/overload.pm
index 758b67d..df074df 100644
--- a/lib/overload.pm
+++ b/lib/overload.pm
@@ -63,7 +63,7 @@ sub unimport {
   *{$package . "::(("} = \&nil;
   for (@_) {
       warnings::warnif("overload arg '$_' is invalid")
-        unless $ops_seen{$_};
+        unless exists $ops_seen{$_};
       delete $ {$package . "::"}{$_ eq 'fallback' ? '()' : "(" .$_};
   }
 }
diff --git a/t/lib/overload_fallback.t b/t/lib/overload_fallback.t
index 6b50042..802fec3 100644
--- a/t/lib/overload_fallback.t
+++ b/t/lib/overload_fallback.t
@@ -1,6 +1,6 @@
 use warnings;
 use strict;
-use Test::Simple tests => 2;
+use Test::Simple tests => 3;
 
 use overload '""' => sub { 'stringvalue' }, fallback => 1;
 
@@ -16,3 +16,15 @@ use overload '+' => sub { die "unused"; };
 my $x = bless {}, 'main';
 ok (eval {$x eq 'stringvalue'}, 'fallback worked again');
 
+{
+    my $warned = 0;
+    local $SIG{__WARN__} = sub { $warned++; };
+
+    eval q{
+        use overload '${}', 'fallback';
+        no overload '${}', 'fallback';
+    };
+
+    ok($warned == 0, 'no overload should not warn');
+}
+
-- 
2.9.3

@p5pRT
Copy link
Author

p5pRT commented Oct 11, 2016

From [Unknown Contact. See original ticket]

The problem was that the patch changed the "ops list" hash from having '1' as the value to having 'undef'. This was checked in two places in order to determine if a warning should be issued - but only one of them was changed. I've attached a patch to correct the bug, and to test for it.

--
Respectfully,
Dan Collins

@p5pRT
Copy link
Author

p5pRT commented Oct 11, 2016

From @jkeenan

On Tue Oct 11 12​:10​:37 2016, dcollinsn@​gmail.com wrote​:

The problem was that the patch changed the "ops list" hash from having
'1' as the value to having 'undef'. This was checked in two places in
order to determine if a warning should be issued - but only one of
them was changed. I've attached a patch to correct the bug, and to
test for it.

Pushed for testing or for testing the failing module to​:
smoke-me/jkeenan/dcollins/129851

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

@p5pRT
Copy link
Author

p5pRT commented Oct 11, 2016

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

@p5pRT
Copy link
Author

p5pRT commented Dec 6, 2016

From @jkeenan

On Tue, 11 Oct 2016 19​:10​:37 GMT, dcollinsn@​gmail.com wrote​:

The problem was that the patch changed the "ops list" hash from having
'1' as the value to having 'undef'. This was checked in two places in
order to determine if a warning should be issued - but only one of
them was changed. I've attached a patch to correct the bug, and to
test for it.

Dan, I reviewed the status of this RT today. The branch I created for smoke-testing didn't show great results. I tried to apply your patch to blead and got merge conflicts. t/lib/overload.t may have changed. Can you investigate?

Thank you very much.

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

@p5pRT
Copy link
Author

p5pRT commented Dec 26, 2016

From @jkeenan

On Tue, 06 Dec 2016 21​:37​:27 GMT, jkeenan wrote​:

On Tue, 11 Oct 2016 19​:10​:37 GMT, dcollinsn@​gmail.com wrote​:

The problem was that the patch changed the "ops list" hash from
having
'1' as the value to having 'undef'. This was checked in two places in
order to determine if a warning should be issued - but only one of
them was changed. I've attached a patch to correct the bug, and to
test for it.

Dan, I reviewed the status of this RT today. The branch I created for
smoke-testing didn't show great results. I tried to apply your patch
to blead and got merge conflicts. t/lib/overload.t may have changed.
Can you investigate?

Thank you very much.

Dan, when you get a chance, could you review this RT?

Thank you very much.

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

@p5pRT
Copy link
Author

p5pRT commented Dec 26, 2016

From @iabyn

On Sun, Dec 25, 2016 at 06​:05​:32PM -0800, James E Keenan via RT wrote​:

On Tue, 06 Dec 2016 21​:37​:27 GMT, jkeenan wrote​:

On Tue, 11 Oct 2016 19​:10​:37 GMT, dcollinsn@​gmail.com wrote​:

The problem was that the patch changed the "ops list" hash from
having
'1' as the value to having 'undef'. This was checked in two places in
order to determine if a warning should be issued - but only one of
them was changed. I've attached a patch to correct the bug, and to
test for it.

Dan, I reviewed the status of this RT today. The branch I created for
smoke-testing didn't show great results. I tried to apply your patch
to blead and got merge conflicts. t/lib/overload.t may have changed.
Can you investigate?

Thank you very much.

Dan, when you get a chance, could you review this RT?

I've just updated the commit to work with blead, confirmed that it fixes
Devel-EnforceEncapsulation, and mreged into blead as v5.25.8-77-g6b77640.

--
In England there is a special word which means the last sunshine
of the summer. That word is "spring".

@p5pRT
Copy link
Author

p5pRT commented Dec 26, 2016

From @jkeenan

On Mon, 26 Dec 2016 12​:13​:41 GMT, davem wrote​:

On Sun, Dec 25, 2016 at 06​:05​:32PM -0800, James E Keenan via RT wrote​:

On Tue, 06 Dec 2016 21​:37​:27 GMT, jkeenan wrote​:

On Tue, 11 Oct 2016 19​:10​:37 GMT, dcollinsn@​gmail.com wrote​:

The problem was that the patch changed the "ops list" hash from
having
'1' as the value to having 'undef'. This was checked in two places in
order to determine if a warning should be issued - but only one of
them was changed. I've attached a patch to correct the bug, and to
test for it.

Dan, I reviewed the status of this RT today. The branch I created for
smoke-testing didn't show great results. I tried to apply your patch
to blead and got merge conflicts. t/lib/overload.t may have changed.
Can you investigate?

Thank you very much.

Dan, when you get a chance, could you review this RT?

I've just updated the commit to work with blead, confirmed that it fixes
Devel-EnforceEncapsulation, and mreged into blead as v5.25.8-77-g6b77640.

I was able to build a threaded DEBUGGING perl at this commit point​:

#####
This is perl 5, version 25, subversion 9 (v5.25.9 (v5.25.8-78-gd720149)) built for x86_64-linux-thread-multi
#####

... and then use 'cpanm' to successfully test and install Devel-EnforceEncapsulation. Resolving ticket.

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

@p5pRT
Copy link
Author

p5pRT commented Dec 26, 2016

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

@p5pRT p5pRT closed this as completed Dec 26, 2016
@p5pRT p5pRT added BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) Severity Low distro-All hasPatch hasTest type-library labels Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) distro-All hasPatch hasTest type-library
Projects
None yet
Development

No branches or pull requests

1 participant