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

BBC: Commit 7a831b72 breaks UNIVERAL::isa tests #16222

Closed
p5pRT opened this issue Nov 4, 2017 · 12 comments
Closed

BBC: Commit 7a831b72 breaks UNIVERAL::isa tests #16222

p5pRT opened this issue Nov 4, 2017 · 12 comments

Comments

@p5pRT
Copy link

p5pRT commented Nov 4, 2017

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

Searchable as RT132395$

@p5pRT
Copy link
Author

p5pRT commented Nov 4, 2017

From @jkeenan

As first reported to me by Carlos Guevara​:

#####
It looks like 7a831b7 broke UNIVERSAL​::isa.

No report available, since the install just hangs at​:
t/00-report-prereqs.t .. ok
t/basic.t .............. 1/52
when installing Term​::ProgressBar​::Simple via cpan configured with​:
recommends_policy [0]
suggests_policy [0]
#####

Here is the output from attempting to install UNIVERSAL-isa via 'cpanm'
at 83a320f​:

#####
[snip prerequisites, which installed fine]
Searching UNIVERSAL​::isa (1.20110614) on cpanmetadb ...
--> Working on UNIVERSAL​::isa
Fetching
http​://www.cpan.org/authors/id/E/ET/ETHER/UNIVERSAL-isa-1.20171012.tar.gz
-> OK
Unpacking UNIVERSAL-isa-1.20171012.tar.gz
Entering UNIVERSAL-isa-1.20171012
Checking configure dependencies from META.json
Checking if you have ExtUtils​::MakeMaker 6.58 ... Yes (7.30)
Configuring UNIVERSAL-isa-1.20171012
Running Makefile.PL
Checking if your kit is complete...
Looks good
Generating a Unix-style Makefile
Writing Makefile for UNIVERSAL​::isa
Writing MYMETA.yml and MYMETA.json
-> OK
Checking dependencies from MYMETA.json ...
Checking if you have warnings​::register 0 ... Yes (1.04)
Checking if you have strict 0 ... Yes (1.11)
Checking if you have ExtUtils​::MakeMaker 0 ... Yes (7.30)
Checking if you have Test​::More 0 ... Yes (1.302103)
Checking if you have warnings 0 ... Yes (1.37)
Checking if you have Scalar​::Util 0 ... Yes (1.49)
Checking if you have overload 0 ... Yes (1.28)
Checking if you have File​::Spec 0 ... Yes (3.68)
Building and testing UNIVERSAL-isa-1.20171012
cp lib/UNIVERSAL/isa.pm blib/lib/UNIVERSAL/isa.pm
PERL_DL_NONLAZY=1 "/home/jkeenan/testing/blead/bin/perl"
"-MExtUtils​::Command​::MM" "-MTest​::Harness" "-e" "undef
*Test​::Harness​::Switches; test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
#
# Versions for all modules listed in MYMETA.json (including optional ones)​:

[snip​: all Requires, Recommends look fine]

# === Other Modules ===
#
# Module Have
# ------------- -------
# JSON​::PP 2.94
# Pod​::Coverage missing
# Sub​::Name missing
# YAML missing
# autodie 2.29
#
t/00-report-prereqs.t .. ok
[When you observe an indefinite hang and hit Ctrl-C, you get ...]
Makefile​:861​: recipe for target 'test_dynamic' failed
make​: *** [test_dynamic] Interrupt
#####

t/basic.t hangs going into test 41​:

#####
$ bleadprove t/basic.t
t/basic.t ..
1..52
ok 1 - undef isa nothing
ok 2 - not warning by default
ok 3 - [] is an array ref
[snip]
ok 39 - undef isa nothing
ok 40 - ... warning in verbose mode
[hangs indefinitely going into test 41]
#####

Here's what the next tests scheduled look like​:

#####
  ok( isa( {}, 'HASH' ), 'hash reference isa HASH' );
  like( $warning, qr/Called.+as a function.+reftyp.+basic.t/s,
  '... warning in verbose mode' );
#####

However, experimentation suggests that the indefinite hang will occur
for any of tests 41-52.

Separately, one TODO test in t/warnings.t may need modification​:

#####
$ bleadprove t/warnings.t
t/warnings.t ..
1..12
ok 1 - U​::i should warn by default when redirecting to overridden method
[snip]
ok 11 - ... even when it would return false
not ok 12 - No warnings when called properly, as a method # TODO no
apparent way of distinguishing between being called as a function and a
method
# Failed (TODO) test 'No warnings when called properly, as a method'
# at t/warnings.t line 85.
# got​: 'Called UNIVERSAL​::isa() as a function, not a method at
t/warnings.t line 84.
# '
# expected​: ''
ok
All tests successful.
Files=1, Tests=12, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.03 cusr
0.00 csys = 0.04 CPU)
Result​: PASS
#####

This may be related to certain bug tickets in UNIVERSAL​::isa's queue at
https://rt.cpan.org/Dist/Display.html?Name=UNIVERSAL-isa.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Nov 4, 2017

From @jkeenan

Summary of my perl5 (revision 5 version 27 subversion 6) configuration​:
  Commit id​: 83a320f
  Platform​:
  osname=linux
  osvers=4.4.0-98-generic
  archname=x86_64-linux
  uname='linux zareason 4.4.0-98-generic #121-ubuntu smp tue oct 10 14​:24​:03 utc 2017 x86_64 x86_64 x86_64 gnulinux '
  config_args='-des -Dusedevel -Uversiononly -Dprefix=/home/jkeenan/testing/blead -Dman1dir=none -Dman3dir=none'
  hint=recommended
  useposix=true
  d_sigaction=define
  useithreads=undef
  usemultiplicity=undef
  use64bitint=define
  use64bitall=define
  uselongdouble=undef
  usemymalloc=n
  default_inc_excludes_dot=define
  bincompat5005=undef
  Compiler​:
  cc='cc'
  ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
  optimize='-O2'
  cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
  ccversion=''
  gccversion='5.4.1 20160904'
  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/5/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 /lib64 /usr/lib64
  libs=-lpthread -lnsl -ldb -ldl -lm -lcrypt -lutil -lc
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.23.so
  so=so
  useshrplib=false
  libperl=libperl.a
  gnulibc_version='2.23'
  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-strong'

Characteristics of this binary (from libperl)​:
  Compile-time options​:
  HAS_TIMES
  PERLIO_LAYERS
  PERL_COPY_ON_WRITE
  PERL_DONT_CREATE_GVSV
  PERL_MALLOC_WRAP
  PERL_OP_PARENT
  PERL_PRESERVE_IVUV
  PERL_USE_DEVEL
  USE_64_BIT_ALL
  USE_64_BIT_INT
  USE_LARGE_FILES
  USE_LOCALE
  USE_LOCALE_COLLATE
  USE_LOCALE_CTYPE
  USE_LOCALE_NUMERIC
  USE_LOCALE_TIME
  USE_PERLIO
  USE_PERL_ATOF
  Built under linux
  Compiled at Nov 4 2017 09​:02​:37
  %ENV​:
  PERL2DIR="/home/jkeenan/gitwork/perl2"
  PERLBREW_BASHRC_VERSION="0.78"
  PERLBREW_HOME="/home/jkeenan/.perlbrew"
  PERLBREW_MANPATH="/home/jkeenan/perl5/perlbrew/perls/perl-5.26.0/man"
  PERLBREW_PATH="/home/jkeenan/perl5/perlbrew/bin​:/home/jkeenan/perl5/perlbrew/perls/perl-5.26.0/bin"
  PERLBREW_PERL="perl-5.26.0"
  PERLBREW_ROOT="/home/jkeenan/perl5/perlbrew"
  PERLBREW_VERSION="0.78"
  PERL_WORKDIR="/home/jkeenan/gitwork/perl"
  @​INC​:
  ~/testing/blead/lib
  /home/jkeenan/testing/blead/lib/perl5/site_perl/5.27.6/x86_64-linux
  /home/jkeenan/testing/blead/lib/perl5/site_perl/5.27.6
  /home/jkeenan/testing/blead/lib/perl5/5.27.6/x86_64-linux
  /home/jkeenan/testing/blead/lib/perl5/5.27.6

@p5pRT
Copy link
Author

p5pRT commented Nov 4, 2017

From @iabyn

On Sat, Nov 04, 2017 at 07​:03​:16AM -0700, James E Keenan wrote​:

It looks like 7a831b7 broke UNIVERSAL​::isa.

which is​:

commit 7a831b7
Author​: Nicolas R <atoomic@​cpan.org>
AuthorDate​: Tue Aug 22 13​:26​:15 2017 -0500
Commit​: Nicolas R <atoomic@​cpan.org>
CommitDate​: Fri Nov 3 11​:26​:19 2017 -0500

  Speed up Carp.pm when backtrace arguments are references
 
  Avoid downgrading the string when not required.
 
  Author​: J. Nick Koston <nick@​cpanel.net>
  References​: CPANEL-15140

--
But Pity stayed his hand. "It's a pity I've run out of bullets",
he thought. -- "Bored of the Rings"

@p5pRT
Copy link
Author

p5pRT commented Nov 4, 2017

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

@p5pRT
Copy link
Author

p5pRT commented Nov 4, 2017

From zefram@fysh.org

Dave Mitchell wrote​:

commit 7a831b7
...
Speed up Carp.pm when backtrace arguments are references

That commit should never have been applied, and should be reverted
forthwith. We already established that it's buggy and mostly pointless.
The only part of it that has any value is the UNIVERSAL​::isa() check,
which can be applied independent of the rest.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Nov 4, 2017

From @karenetheridge

Author​: J. Nick Koston <nick@​cpanel.net>
References​: CPANEL-15140

What is this bit referring to? Is this patch intended to fix an issue that cPanel is having?
Can someone report on what that issue is?

@p5pRT
Copy link
Author

p5pRT commented Nov 4, 2017

From zefram@fysh.org

Karen Etheridge via RT wrote​:

Is this patch intended to fix an issue that cPanel is having?

The original version of the patch came with a Changes entry, which
commented

  In testing this decreased the Carp backtrace time by about
  35% in production code.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Nov 4, 2017

From zefram@fysh.org

I wrote​:

That commit should never have been applied, and should be reverted
forthwith.

Now reverted as commit 0ebeacd.

However, the part of the commit that seems potentially useful would
presumably still break UNIVERSAL​::isa in the same way. So this BBC
still needs to be looked into.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Nov 5, 2017

From @bdraco

It looks like lib/UNIVERSAL/isa.pm​:_report_warning is handling the same problem for Test​::Builder and Test​::Stream with an explicit check​:

96 # check calling sub
97 return if (( caller(3) )[3] || '') =~ /​::isa$/;
98 # check calling package - exempt Test​::Builder??
99 return if (( caller(3) )[0] || '') =~ /^Test​::Builder/;
100 return if (( caller(2) )[0] || '') =~ /^Test​::Stream/;

Adding this for Carp allows it to pass​:

101 return if (( caller(2) )[0] || '') =~ /^Carp/;

However there is probably a better way to handle this.

-Nick

On Nov 4, 2017, at 10​:48 AM, Zefram <zefram@​fysh.org> wrote​:

Karen Etheridge via RT wrote​:

Is this patch intended to fix an issue that cPanel is having?

The original version of the patch came with a Changes entry, which
commented

In testing this decreased the Carp backtrace time by about
35% in production code\.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Nov 5, 2017

From @xsawyerx

I think some of it can be applied since they are less pervasive, but
likely to have very minimal gains. The biggest part is the optional
downgrade. Now that we have a patch that makes it work, is a better way
to accomplish this?

Meanwhile, I think considering the breakage, it makes sense to revert.

On 11/05/2017 02​:35 AM, J. Nick Koston wrote​:

It looks like  lib/UNIVERSAL/isa.pm​:_report_warning is handling the
same problem for Test​::Builder and Test​::Stream with an explicit check​:

 96         # check calling sub
 97         returnif(( caller(3) )[3] || '') =~ /​::isa$/;
 98         # check calling package - exempt Test​::Builder??
 99         returnif(( caller(3) )[0] || '') =~ /^Test​::Builder/;
100         returnif(( caller(2) )[0] || '') =~ /^Test​::Stream/;

Adding this for Carp allows it to pass​:

101         returnif(( caller(2) )[0] || '') =~ /^Carp/;

However there is probably a better way to handle this.

-Nick

On Nov 4, 2017, at 10​:48 AM, Zefram <zefram@​fysh.org
<mailto​:zefram@​fysh.org>> wrote​:

Karen Etheridge via RT wrote​:

Is this patch intended to fix an issue that cPanel is having?

The original version of the patch came with a Changes entry, which
commented

    In testing this decreased the Carp backtrace time by about
    35% in production code.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Dec 5, 2017

From @karenetheridge

The blead commit has been reverted.

@p5pRT
Copy link
Author

p5pRT commented Dec 5, 2017

@karenetheridge - 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