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

Blead Breaks CPAN: Role::Tiny, Variable::Magic, Moo #16191

Closed
p5pRT opened this issue Oct 9, 2017 · 92 comments
Closed

Blead Breaks CPAN: Role::Tiny, Variable::Magic, Moo #16191

p5pRT opened this issue Oct 9, 2017 · 92 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Oct 9, 2017

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

Searchable as RT132252$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 9, 2017

From @simcop2387

Somewhere between e04fc1a and e92359c, Role​::Tiny and Variable​::Magic
were broken. I haven't bisected it myself yet but based on talk in #p5p
from haarg I *suspect* that it's the stash changes in
1369fd5

Reported by HAARG also, Moo has failures of it's own with this change, one
of us will attach a log shortly.

--- BEGIN perl -V ---
Summary of my perl5 (revision 5 version 27 subversion 5) configuration​:
  Derived from​: e92359c
  Platform​:
  osname=linux
  osvers=4.9.36-x86_64-linode85
  archname=x86_64-linux
  uname='linux simcop2387.info 4.9.36-x86_64-linode85 #1 smp thu jul 6
15​:31​:23 utc 2017 x86_64 gnulinux '
  config_args='-de
-Dprefix=/home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776
-Dccflags=-fpie -fPIC -march=native -fstack-protector-all -pie
-D_FORTIFY_SOURCE=2 -Dldflags=-Wl,-z,now -Wl,-zrelro -Dusedevel
-Aeval​:scriptdir=/home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/bin'
  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 ='-fpie -fPIC -march=native -fstack-protector-all -pie
-D_FORTIFY_SOURCE=2 -fwrapv -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64'
  optimize='-O2'
  cppflags='-fpie -fPIC -march=native -fstack-protector-all -pie
-D_FORTIFY_SOURCE=2 -fwrapv -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include'
  ccversion=''
  gccversion='7.2.0'
  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 ='-Wl,-z,now -Wl,-zrelro -fstack-protector-strong
-L/usr/local/lib'
  libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/7/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 -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc
-lgdbm_compat
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.24.so
  so=so
  useshrplib=false
  libperl=libperl.a
  gnulibc_version='2.24'
  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
  Locally applied patches​:
  uncommitted-changes
  Devel​::PatchPerl 1.38
  Built under linux
  Compiled at Oct 9 2017 07​:03​:06
  %ENV​:
  PERLBREW_BASHRC_VERSION="0.78"
  PERLBREW_HOME="/home/ryan/.perlbrew"

PERLBREW_MANPATH="/home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/man"

PERLBREW_PATH="/home/ryan/perl5/perlbrew/bin​:/home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/bin"
  PERLBREW_PERL="perlbot-blead-2017-10-09_12776"
  PERLBREW_ROOT="/home/ryan/perl5/perlbrew"
  PERLBREW_VERSION="0.78"
  @​INC​:

/home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/lib/site_perl/5.27.5/x86_64-linux

/home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/lib/site_perl/5.27.5

/home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/lib/5.27.5/x86_64-linux

/home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/lib/5.27.5
----- END perl -V -----

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 9, 2017

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 9, 2017

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 10, 2017

From @cpansprout

On Mon, 09 Oct 2017 08​:06​:46 -0700, simcop2387@​simcop2387.info wrote​:

Somewhere between e04fc1a and e92359c, Role​::Tiny and
Variable​::Magic
were broken. I haven't bisected it myself yet but based on talk in
#p5p
from haarg I *suspect* that it's the stash changes in
1369fd5

Very likely. I was expecting some fallout.

Reported by HAARG also, Moo has failures of it's own with this change,
one
of us will attach a log shortly.

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 10, 2017

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 10, 2017

From @haarg

Some additional failures due to the same change.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 10, 2017

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 10, 2017

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 10, 2017

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 10, 2017

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 10, 2017

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 12, 2017

From @simcop2387

Also breaks autovivification

On Tue, Oct 10, 2017 at 2​:16 AM, Graham Knop via RT <
perlbug-followup@​perl.org> wrote​:

Some additional failures due to the same change.

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132252

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 12, 2017

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 15, 2017

From @andk

On Mon, 09 Oct 2017 17​:07​:34 -0700, "Father Chrysostomos via RT" <perlbug-followup@​perl.org> said​:

  > Very likely. I was expecting some fallout.

What are the plans how to proceed? Shall this go into 5.27.5? I mean, is
everybody aware this is a huge fallout?

--
andreas

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 15, 2017

From @cpansprout

On Sat, 14 Oct 2017 23​:48​:08 -0700, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

On Mon, 09 Oct 2017 17​:07​:34 -0700, "Father Chrysostomos via RT"
<perlbug-followup@​perl.org> said​:

Very likely. I was expecting some fallout.

What are the plans how to proceed? Shall this go into 5.27.5? I mean,
is
everybody aware this is a huge fallout?

I am hoping, when I get time on the weekends (like today), to start patching the CPAN modules one by one. We still have many months to go before the stable release.

But if the pumpking wants to back out the change for now, I have no objection (except that it may make it harder to get the patches accepted--I really do want this change in before 5.28).

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 15, 2017

From @jkeenan

On Sun, 15 Oct 2017 15​:17​:03 GMT, sprout wrote​:

On Sat, 14 Oct 2017 23​:48​:08 -0700,
andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

On Mon, 09 Oct 2017 17​:07​:34 -0700, "Father Chrysostomos via
RT"
<perlbug-followup@​perl.org> said​:

Very likely. I was expecting some fallout.

What are the plans how to proceed? Shall this go into 5.27.5? I mean,
is
everybody aware this is a huge fallout?

I am hoping, when I get time on the weekends (like today), to start
patching the CPAN modules one by one. We still have many months to go
before the stable release.

But if the pumpking wants to back out the change for now, I have no
objection (except that it may make it harder to get the patches
accepted--I really do want this change in before 5.28).

Father C,

Can you describe the purpose of the branch you merged on Oct 08?

Given the fact that the libraries which we already know have been broken are rather high up on the CPAN river, there is a greater than normal likelihood that libraries farther down the river and darkpan code will also break. If so, then we need to assess the costs and benefits of the merge before proceeding.

Thank you very much.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 16, 2017

From @xsawyerx

On 10/15/2017 10​:07 PM, James E Keenan via RT wrote​:

On Sun, 15 Oct 2017 15​:17​:03 GMT, sprout wrote​:

On Sat, 14 Oct 2017 23​:48​:08 -0700,
andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

On Mon, 09 Oct 2017 17​:07​:34 -0700, "Father Chrysostomos via
RT"
<perlbug-followup@​perl.org> said​:
Very likely. I was expecting some fallout.
What are the plans how to proceed? Shall this go into 5.27.5? I mean,
is
everybody aware this is a huge fallout?
I am hoping, when I get time on the weekends (like today), to start
patching the CPAN modules one by one. We still have many months to go
before the stable release.

But if the pumpking wants to back out the change for now, I have no
objection (except that it may make it harder to get the patches
accepted--I really do want this change in before 5.28).
Father C,

Can you describe the purpose of the branch you merged on Oct 08?

Given the fact that the libraries which we already know have been broken are rather high up on the CPAN river, there is a greater than normal likelihood that libraries farther down the river and darkpan code will also break. If so, then we need to assess the costs and benefits of the merge before proceeding.

An additional temporary problem is that this makes it difficult for CPAN
smokers to expose problems with downstream modules. If Moo fails, we
can't test anything using Moo.

I think it's better to temporarily revert this just until we can resolve it.

Father C., if you can share your patch, others could help with the
patching. (I volunteer.)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 16, 2017

From @cpansprout

On Sun, 15 Oct 2017 13​:07​:56 -0700, jkeenan wrote​:

Father C,

Can you describe the purpose of the branch you merged on Oct 08?

It saves a large amount of memory.

Before this change, subroutines in packages (the most common use of package symbols) resided in typeglobs. So ‘package Foo; sub foo{}’ would be stored as *foo{CODE} in the *foo typeglob in the Foo package. I.e., $Foo​::{foo} was a typeglob with its CODE slot pointing to something.

The new behaviour is that, instead of a typeglob, we have a scalar with a reference to the subroutine. So now $Foo​::{foo} is a coderef.

Since an RV (a reference) takes 24 bytes of memory whereas a GV (typeglob) takes 24 (sv head) + 48 (xpvgv body) + 80 (gp) = 152 bytes, plus a little malloc overhead for the gp struct, which I think is 4 to 6 bytes on most platforms, so the total is about 157 bytes.

That’s 133 bytes extra per subroutine. If you load five modules each with 100 subroutines, you’ve just used 65 K more ram than you needed to.

(These are all 64-bit numbers. The 32-bit numbers differ, but the GV will always be bigger than the RV.)

Granted, method calls will undo the optimization (caching the method in the typeglob, trading memory for speed), but this only happens if a particular method actually gets called. Hence, small scripts that load large libraries will see the biggest reduction in memory usage. Large object-oriented systems that call methods for everything see the smallest reduction in memory usage.

Given the fact that the libraries which we already know have been
broken are rather high up on the CPAN river, there is a greater than
normal likelihood that libraries farther down the river and darkpan
code will also break. If so, then we need to assess the costs and
benefits of the merge before proceeding.

If we are to revert the behaviour, it is only this hunk from 6881372 that needs to be reverted​:

@​@​ -8583,7 +8590,7 @​@​ Perl_newATTRSUB_x(pTHX_ I32 floor, OP *o, OP *proto, OP *attrs,
  sub is stored in. */
  const I32 flags =
  ec ? GV_NOADD_NOINIT
- : PL_curstash != CopSTASH(PL_curcop)
+ : IN_PERL_RUNTIME && PL_curstash != CopSTASH(PL_curcop)
  || memchr(name, '​:', namlen) || memchr(name, '\'', namlen)
  ? gv_fetch_flags
  : GV_ADDMULTI | GV_NOINIT | GV_NOTQUAL;

and whatever tests end up failing need to be marked TODO.

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 17, 2017

From @jkeenan

On Mon, 16 Oct 2017 16​:16​:18 GMT, xsawyerx@​gmail.com wrote​:

On 10/15/2017 10​:07 PM, James E Keenan via RT wrote​:

On Sun, 15 Oct 2017 15​:17​:03 GMT, sprout wrote​:

On Sat, 14 Oct 2017 23​:48​:08 -0700,
andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

On Mon, 09 Oct 2017 17​:07​:34 -0700, "Father Chrysostomos via
RT"
<perlbug-followup@​perl.org> said​:
Very likely. I was expecting some fallout.
What are the plans how to proceed? Shall this go into 5.27.5? I
mean,
is
everybody aware this is a huge fallout?
I am hoping, when I get time on the weekends (like today), to start
patching the CPAN modules one by one. We still have many months to
go
before the stable release.

But if the pumpking wants to back out the change for now, I have no
objection (except that it may make it harder to get the patches
accepted--I really do want this change in before 5.28).
Father C,

Can you describe the purpose of the branch you merged on Oct 08?

Given the fact that the libraries which we already know have been
broken are rather high up on the CPAN river, there is a greater than
normal likelihood that libraries farther down the river and darkpan
code will also break. If so, then we need to assess the costs and
benefits of the merge before proceeding.

An additional temporary problem is that this makes it difficult for
CPAN
smokers to expose problems with downstream modules. If Moo fails, we
can't test anything using Moo.

I think it's better to temporarily revert this just until we can
resolve it.

I also think that would be the best course of action. Father C could fork HEAD to a branch just before reverting. Ongoing work could then take place in that branch and, at a suitable point, we could install it somewhere for smoking the failing libraries and the upper parts of the CPAN river.

Father C., if you can share your patch, others could help with the
patching. (I volunteer.)

Since this code had no FAILs within the CPAN test suite -- only downstream on CPAN. I think we need to look at the code on CPAN which broke to write tests which we can then include in the Perl 5 test suite to head off such failures in the future.

Thank you very much.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 17, 2017

From @shlomif

On Tue, 17 Oct 2017 06​:55​:11 -0700
"James E Keenan via RT" <perlbug-followup@​perl.org> wrote​:

On Mon, 16 Oct 2017 16​:16​:18 GMT, xsawyerx@​gmail.com wrote​:

On 10/15/2017 10​:07 PM, James E Keenan via RT wrote​:

On Sun, 15 Oct 2017 15​:17​:03 GMT, sprout wrote​:

On Sat, 14 Oct 2017 23​:48​:08 -0700,
andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

On Mon, 09 Oct 2017 17​:07​:34 -0700, "Father Chrysostomos via
RT"
<perlbug-followup@​perl.org> said​:
Very likely. I was expecting some fallout.
What are the plans how to proceed? Shall this go into 5.27.5? I
mean,
is
everybody aware this is a huge fallout?
I am hoping, when I get time on the weekends (like today), to start
patching the CPAN modules one by one. We still have many months to
go
before the stable release.

But if the pumpking wants to back out the change for now, I have no
objection (except that it may make it harder to get the patches
accepted--I really do want this change in before 5.28).
Father C,

Can you describe the purpose of the branch you merged on Oct 08?

Given the fact that the libraries which we already know have been
broken are rather high up on the CPAN river, there is a greater than
normal likelihood that libraries farther down the river and darkpan
code will also break. If so, then we need to assess the costs and
benefits of the merge before proceeding.

An additional temporary problem is that this makes it difficult for
CPAN
smokers to expose problems with downstream modules. If Moo fails, we
can't test anything using Moo.

I think it's better to temporarily revert this just until we can
resolve it.

I also think that would be the best course of action. Father C could fork
HEAD to a branch just before reverting. Ongoing work could then take place
in that branch and, at a suitable point, we could install it somewhere for
smoking the failing libraries and the upper parts of the CPAN river.

Sounds like a good course of action.

Father C., if you can share your patch, others could help with the
patching. (I volunteer.)

Since this code had no FAILs within the CPAN test suite -- only downstream on
CPAN. I think we need to look at the code on CPAN which broke to write tests
which we can then include in the Perl 5 test suite to head off such failures
in the future.

I also think this will be a good idea.

Thank you very much.

--


Shlomi Fish http​://www.shlomifish.org/
http​://youtu.be/xZLwtc9x4yA - Anime in Real Life!! (Parody)

Every successful open source project will eventually spawn a sub‐project.

Please reply to list if it's a mailing list post - http​://shlom.in/reply .

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 17, 2017

From @haarg

On Sun, Oct 15, 2017 at 5​:17 PM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sat, 14 Oct 2017 23​:48​:08 -0700, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

On Mon, 09 Oct 2017 17​:07​:34 -0700, "Father Chrysostomos via RT"
<perlbug-followup@​perl.org> said​:

Very likely. I was expecting some fallout.

What are the plans how to proceed? Shall this go into 5.27.5? I mean,
is
everybody aware this is a huge fallout?

I am hoping, when I get time on the weekends (like today), to start patching the CPAN modules one by one. We still have many months to go before the stable release.

But if the pumpking wants to back out the change for now, I have no objection (except that it may make it harder to get the patches accepted--I really do want this change in before 5.28).

I have no particular opinion about if this patch should be backed out,
but I do have patches prepared for Role​::Tiny and Moo to deal with the
issue.

--

Father Chrysostomos

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132252

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 18, 2017

From @cpansprout

On Tue, 17 Oct 2017 15​:16​:46 -0700, haarg wrote​:

On Sun, Oct 15, 2017 at 5​:17 PM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sat, 14 Oct 2017 23​:48​:08 -0700,
andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

On Mon, 09 Oct 2017 17​:07​:34 -0700, "Father Chrysostomos via
RT"
<perlbug-followup@​perl.org> said​:

Very likely. I was expecting some fallout.

What are the plans how to proceed? Shall this go into 5.27.5? I
mean,
is
everybody aware this is a huge fallout?

I am hoping, when I get time on the weekends (like today), to start
patching the CPAN modules one by one. We still have many months to
go before the stable release.

But if the pumpking wants to back out the change for now, I have no
objection (except that it may make it harder to get the patches
accepted--I really do want this change in before 5.28).

I have no particular opinion about if this patch should be backed out,
but I do have patches prepared for Role​::Tiny and Moo to deal with the
issue.

I have just submitted a patch for Variable​::Magic​: https://rt.cpan.org/Ticket/Display.html?id=123314

I am very short on time till the weekend. Would someone else have a chance to revert the hunk I cited?

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 18, 2017

From @xsawyerx

On 10/17/2017 03​:55 PM, James E Keenan via RT wrote​:

On Mon, 16 Oct 2017 16​:16​:18 GMT, xsawyerx@​gmail.com wrote​:

[...]
Father C., if you can share your patch, others could help with the
patching. (I volunteer.)
Since this code had no FAILs within the CPAN test suite -- only downstream on CPAN. I think we need to look at the code on CPAN which broke to write tests which we can then include in the Perl 5 test suite to head off such failures in the future.

I like this idea.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 18, 2017

From @jkeenan

On 10/18/2017 05​:43 AM, Sawyer X wrote​:

On 10/17/2017 03​:55 PM, James E Keenan via RT wrote​:

On Mon, 16 Oct 2017 16​:16​:18 GMT, xsawyerx@​gmail.com wrote​:

[...]
Father C., if you can share your patch, others could help with the
patching. (I volunteer.)
Since this code had no FAILs within the CPAN test suite -- only downstream on CPAN. I think we need to look at the code on CPAN which broke to write tests which we can then include in the Perl 5 test suite to head off such failures in the future.

I like this idea.

Sawyer,

I think it will have to be your call to either (a) do the above, which
entails reverting merge commit 1369fd5
from blead in its entirety (after copying HEAD to a branch); or (b)
fixing only a small portion on that commit, as indicated in Father C's
most recent posts​:

https://www.nntp.perl.org/group/perl.perl5.porters/2017/10/msg246738.html
https://www.nntp.perl.org/group/perl.perl5.porters/2017/10/msg246757.html

(a) has the disadvantage that it puts us into the murkiness described in
file​:///usr/share/doc/git-doc/howto/revert-a-faulty-merge.html. (b) has
the disadvantage that we have to trust the committer that all *other*
parts of 1369fd5 are correct and that we only need to repair one line.
I think it is unfortunate that we have been placed in this situation.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 21, 2017

From @jkeenan

On Wed, 18 Oct 2017 03​:04​:52 GMT, sprout wrote​:

On Tue, 17 Oct 2017 15​:16​:46 -0700, haarg wrote​:

On Sun, Oct 15, 2017 at 5​:17 PM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Sat, 14 Oct 2017 23​:48​:08 -0700,
andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

On Mon, 09 Oct 2017 17​:07​:34 -0700, "Father Chrysostomos via
RT"
<perlbug-followup@​perl.org> said​:

Very likely. I was expecting some fallout.

What are the plans how to proceed? Shall this go into 5.27.5? I
mean,
is
everybody aware this is a huge fallout?

I am hoping, when I get time on the weekends (like today), to start
patching the CPAN modules one by one. We still have many months to
go before the stable release.

But if the pumpking wants to back out the change for now, I have no
objection (except that it may make it harder to get the patches
accepted--I really do want this change in before 5.28).

I have no particular opinion about if this patch should be backed
out,
but I do have patches prepared for Role​::Tiny and Moo to deal with
the
issue.

I have just submitted a patch for Variable​::Magic​:
https://rt.cpan.org/Ticket/Display.html?id=123314

I am very short on time till the weekend. Would someone else have a
chance to revert the hunk I cited?

Father C made these commits​:

#####
commit 6fbf0c3
Author​: Father Chrysostomos <sprout@​cpan.org>

  Deparse.t tweak

commit 834e1dc
Author​: Father Chrysostomos <sprout@​cpan.org>

  perldelta for 1406232

commit 6eed25e
Author​: Father Chrysostomos <sprout@​cpan.org>

  Temporarily revert CV-in-stash optimisation
#####

I installed blead for testing, then installed cpanm. I then ran the following​:

#####
./bin/cpanm --verbose indirect autovivification Type​::Tiny Variable​::Magic Role​::Tiny Moo Log​::Trace Test​::API
#####

All these modules installed with no test failures. But I think the ticket should stay open for a while to see if any other distros had the BBC.

Thank you very much.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 23, 2017

From @cpansprout

On Mon, 16 Oct 2017 09​:16​:18 -0700, xsawyerx@​gmail.com wrote​:

Father C., if you can share your patch, others could help with the
patching. (I volunteer.)

Thank you. For starters, I have just submitted a patch to fix Log​::Trace’s bug #49409​: https://rt.cpan.org/Ticket/Display.html?id=49409

Log​::Trace fails to handle the new optimization for the same reason that it has always failed with constants (see the ticket). The bug wrt constants was reported eight years ago, and Log​::Trace last saw a new release twelve years ago. This is an unfortunate situation for a module depended on by so many things.

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 26, 2017

From @xsawyerx

On 10/18/2017 02​:13 PM, James E Keenan wrote​:

On 10/18/2017 05​:43 AM, Sawyer X wrote​:

On 10/17/2017 03​:55 PM, James E Keenan via RT wrote​:

On Mon, 16 Oct 2017 16​:16​:18 GMT, xsawyerx@​gmail.com wrote​:

[...]
Father C., if you can share your patch, others could help with the
patching. (I volunteer.)
Since this code had no FAILs within the CPAN test suite -- only
downstream on CPAN.  I think we need to look at the code on CPAN
which broke to write tests which we can then include in the Perl 5
test suite to head off such failures in the future.

I like this idea.

Sawyer,

I think it will have to be your call to either (a) do the above, which
entails reverting merge commit
1369fd5 from blead in its entirety
(after copying HEAD to a branch); or (b) fixing only a small portion
on that commit, as indicated in Father C's most recent posts​:

https://www.nntp.perl.org/group/perl.perl5.porters/2017/10/msg246738.html
https://www.nntp.perl.org/group/perl.perl5.porters/2017/10/msg246757.html

(a) has the disadvantage that it puts us into the murkiness described
in file​:///usr/share/doc/git-doc/howto/revert-a-faulty-merge.html. 
(b) has the disadvantage that we have to trust the committer that all
*other* parts of 1369fd5 are correct and that we only need to repair
one line. I think it is unfortunate that we have been placed in this
situation.

I had asked to temporarily revert it.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 29, 2017

From @cpansprout

On Thu, 26 Oct 2017 08​:06​:54 -0700, xsawyerx@​gmail.com wrote​:

On 10/18/2017 02​:13 PM, James E Keenan wrote​:

On 10/18/2017 05​:43 AM, Sawyer X wrote​:

On 10/17/2017 03​:55 PM, James E Keenan via RT wrote​:

On Mon, 16 Oct 2017 16​:16​:18 GMT, xsawyerx@​gmail.com wrote​:

[...]
Father C., if you can share your patch, others could help with the
patching. (I volunteer.)

All the modules listed in this ticket now have patches​:

Haarg says in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132252#txn-1500347 that he has prepared patches for Moo and Role​::Tiny.

Log​::Trace​: https://rt.cpan.org/Ticket/Display.html?id=49409

Variable​::Magic​: https://rt.cpan.org/Ticket/Display.html?id=123314

Test​::API​: dagolden/Test-API#6

Type​::Tiny​: https://rt.cpan.org/Ticket/Display.html?id=123408

indirect​: https://rt.cpan.org/Ticket/Display.html?id=123374

autovivification​: https://rt.cpan.org/Ticket/Display.html?id=123411

Since this code had no FAILs within the CPAN test suite -- only
downstream on CPAN.  I think we need to look at the code on CPAN
which broke to write tests which we can then include in the Perl 5
test suite to head off such failures in the future.

I like this idea.

I this case it doesn’t really apply. In fact, most of the modules are already buggy, because they don’t take into account that, since 5.6 or so, stash elements have contained things other than typeglobs. In some instances I wrote new tests for the modules that got them to fail with existing perl versions. The fix to work with the optimisation in these cases was the same as the fix to work with constants in earlier perl versions.

Sawyer,

I think it will have to be your call to either (a) do the above, which
entails reverting merge commit
1369fd5 from blead in its entirety
(after copying HEAD to a branch); or (b) fixing only a small portion
on that commit, as indicated in Father C's most recent posts​:

https://www.nntp.perl.org/group/perl.perl5.porters/2017/10/msg246738.html
https://www.nntp.perl.org/group/perl.perl5.porters/2017/10/msg246757.html

(a) has the disadvantage that it puts us into the murkiness described
in file​:///usr/share/doc/git-doc/howto/revert-a-faulty-merge.html. 
(b) has the disadvantage that we have to trust the committer that all
*other* parts of 1369fd5 are correct and that we only need to repair
one line. I think it is unfortunate that we have been placed in this
situation.

I had asked to temporarily revert it.

I reverted to the old behaviour in commit 6eed25e and have now pushed a new sprout/cv-in-stash branch, based on current blead (6e8135a), with the revert reverted.

At this point, how do we proceed?

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 1, 2017

From @cpansprout

On Sun, 29 Oct 2017 11​:40​:14 -0700, sprout wrote​:

On Thu, 26 Oct 2017 08​:06​:54 -0700, xsawyerx@​gmail.com wrote​:

On 10/18/2017 02​:13 PM, James E Keenan wrote​:

On 10/18/2017 05​:43 AM, Sawyer X wrote​:

On 10/17/2017 03​:55 PM, James E Keenan via RT wrote​:

Since this code had no FAILs within the CPAN test suite -- only
downstream on CPAN.  I think we need to look at the code on CPAN
which broke to write tests which we can then include in the Perl
5
test suite to head off such failures in the future.

I like this idea.

I this case it doesn’t really apply.

Except for Cpanel​::JSON​::XS, which was failing with my patch because the core function get_cvn_flags was not coping with subrefs when called the way that module calls it. This I have fixed and tested in a385812.

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 1, 2017

From @dur-randir

2017-11-01 4​:00 GMT+03​:00 Father Chrysostomos via RT <perlbug-followup@​perl.org>​:

This I have fixed and tested in a385812.

This commit introduced a new build warning, as follows​:

perl.c​:2708​:9​: warning​: incompatible pointer types returning 'SV *' (aka 'struct sv *') from a function with result type 'CV *' (aka 'struct cv *')
  [-Wincompatible-pointer-types]
  return SvRV((SV *)gv);
  ^~~~~~~~~~~~~~
./sv.h​:1213​:2​: note​: expanded from macro 'SvRV'
  (*({ SV *const _svrv = MUTABLE_SV(sv); \
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

Best regards,
Sergey Aleynikov

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 29, 2018

From @haarg

This seems like lot of breakage for such a tiny benefit. Can this change be reverted?

Additionally, since the stash entries get de-optimized to globs when called as methods, this change will actually lead to increased memory usage on servers that fork. Many methods won't get called until post-fork, so the globs will need to be created for every fork instead of being able to benefit from copy-on-write.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 29, 2018

From @dur-randir

On Mon, 29 Jan 2018 05​:51​:38 -0800, haarg wrote​:

This seems like lot of breakage for such a tiny benefit.

All those modules are already broken on the current perl version (and some versions back in time), they just don't test for it - as globs are already not created for subs in the 'main' package.

Additionally, since the stash entries get de-optimized to globs when
called as methods, this change will actually lead to increased memory
usage on servers that fork. Many methods won't get called until post-
fork, so the globs will need to be created for every fork instead of
being able to benefit from copy-on-write.

You can't rely on copy-on-write for almost all non-optree data (and sometimes even on optree too), as any run-time SV upgrade/free will touch the whole page. Furthermore, there're constant subs (sub foo () {42}), which are almost never called as methods - and by reverting this we will loose memory savings on them.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 30, 2018

From @haarg

On Mon, Jan 29, 2018 at 6​:01 PM, Sergey Aleynikov via RT
<perlbug-followup@​perl.org> wrote​:

On Mon, 29 Jan 2018 05​:51​:38 -0800, haarg wrote​:

This seems like lot of breakage for such a tiny benefit.

All those modules are already broken on the current perl version (and some versions back in time), they just don't test for it - as globs are already not created for subs in the 'main' package.

They are "broken" in a way that nobody has noticed for a long time.
Most of the modules that are failing will usually not be used against
the main package. Breaking the tests of this many modules for an edge
case like this is a poor trade off.

Additionally, since the stash entries get de-optimized to globs when
called as methods, this change will actually lead to increased memory
usage on servers that fork. Many methods won't get called until post-
fork, so the globs will need to be created for every fork instead of
being able to benefit from copy-on-write.

You can't rely on copy-on-write for almost all non-optree data (and sometimes even on optree too), as any run-time SV upgrade/free will touch the whole page. Furthermore, there're constant subs (sub foo () {42}), which are almost never called as methods - and by reverting this we will loose memory savings on them.

The actual memory savings is minuscule. I'm not sure what you are
referring to with constant subs. Constants are more commonly created
via constant.pm, which uses a different optimization of storing scalar
refs in the symbol table. That optimization makes more sense since
they aren't commonly called as methods.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 30, 2018

From @eserte

Another one with "Not a GLOB reference at"​:

DAGOLDEN/Object-LocalVars-0.21.tar.gz

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 31, 2018

From @eserte

Here's a list of distributions which have an old ExtUtils​::Installer bundled. "perl Makefile.PL" fails for these with a "Not a GLOB reference" error.

  * AUTRIJUS/LWP-Authen-Wsse-0.05.tar.gz
  * BOBTFISH/Catalyst-Plugin-HTML-Widget-1.1.tar.gz
  * HCCHIEN/Yahoo-CAS-0.2.tar.gz
  * HCCHIEN/Yahoo-Lifestyle-0.2.tar.gz
  * INGY/Class-Spiffy-0.15.tar.gz
  * INGY/Perldoc-0.20.tar.gz
  * LICHTKIND/Perl6-Bible-0.37.tar.gz
  * LUKEC/WWW-Selenium-Utils-0.09.tar.gz
  * MARKSTOS/CGI-Uploader-2.18.tar.gz
  * MIYAGAWA/Kwiki-Emoticon-0.03.tar.gz

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 31, 2018

From @iabyn

On Tue, Jan 30, 2018 at 10​:39​:30PM -0800, slaven@​rezic.de via RT wrote​:

Here's a list of distributions which have an old ExtUtils​::Installer bundled. "perl Makefile.PL" fails for these with a "Not a GLOB reference" error.

 \* AUTRIJUS/LWP\-Authen\-Wsse\-0\.05\.tar\.gz
 \* BOBTFISH/Catalyst\-Plugin\-HTML\-Widget\-1\.1\.tar\.gz
 \* HCCHIEN/Yahoo\-CAS\-0\.2\.tar\.gz
 \* HCCHIEN/Yahoo\-Lifestyle\-0\.2\.tar\.gz
 \* INGY/Class\-Spiffy\-0\.15\.tar\.gz
 \* INGY/Perldoc\-0\.20\.tar\.gz
 \* LICHTKIND/Perl6\-Bible\-0\.37\.tar\.gz
 \* LUKEC/WWW\-Selenium\-Utils\-0\.09\.tar\.gz
 \* MARKSTOS/CGI\-Uploader\-2\.18\.tar\.gz
 \* MIYAGAWA/Kwiki\-Emoticon\-0\.03\.tar\.gz

I feel that we have too much breakage too late in the release cycle,
and that we should consider reverting this optimisation for now.

--
The Enterprise's efficient long-range scanners detect a temporal vortex
distortion in good time, allowing it to be safely avoided via a minor
course correction.
  -- Things That Never Happen in "Star Trek" #21

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 1, 2018

From @eserte

For the sake of completeness, here more distributions where "Not a GLOB reference at ..." appears in the failing test logs​:

  * HKOBA/YATT-Lite-0.101.tar.gz
  * TOBIWAN/Fukurama-Class-0.032.tar.gz
  * RCAPUTO/POE-Stage-0.060.tar.gz
  * PHAYLON/POE-Component-Basement-0.01.tar.gz

The following distributions cannot be built anymore because of an old bundled inc/Module/Install.pm​:

  * CFRANKS/HTML-Menu-Select-1.01.tar.gz
  * CDENT/Kwiki/Kwiki-PageStats-0.06.tar.gz
  * INGY/Kwiki-Wikiwyg-0.13.tar.gz
  * XERN/Lingua-LinkParser-FindPath-0.01.tar.gz
  * TPG/Net-Radius-PacketOrdered-1.53.tar.gz
  * AUTRIJUS/Parse-SVNDiff-0.05.tar.gz
  * HACHI/POE-Component-Client-LDAP-0.04.tar.gz
  * TPG/Test-Singleton-1.02.tar.gz
  * TPG/What-1.00.tar.gz
  * BTROTT/WWW-Blog-Metadata-Icon-0.02.tar.gz

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 1, 2018

From @cpansprout

On Mon, 29 Jan 2018 21​:31​:31 -0800, haarg wrote​:

On Mon, Jan 29, 2018 at 6​:01 PM, Sergey Aleynikov via RT
<perlbug-followup@​perl.org> wrote​:

On Mon, 29 Jan 2018 05​:51​:38 -0800, haarg wrote​:

This seems like lot of breakage for such a tiny benefit.

All those modules are already broken on the current perl version (and
some versions back in time), they just don't test for it - as globs
are already not created for subs in the 'main' package.

They are "broken" in a way that nobody has noticed for a long time.
Most of the modules that are failing will usually not be used against
the main package. Breaking the tests of this many modules for an edge
case like this is a poor trade off.

I will point out that most of the module I patched already fail with constants for the same reason. I.e., we have uncovered existing bugs in many modules.

I do agree, though, that this amount of breakage warrants a revert. Would you be adverse to reënabling the optimisation in blead after 5.28?

Additionally, since the stash entries get de-optimized to globs when
called as methods, this change will actually lead to increased
memory
usage on servers that fork. Many methods won't get called until
post-
fork, so the globs will need to be created for every fork instead of
being able to benefit from copy-on-write.

You can't rely on copy-on-write for almost all non-optree data (and
sometimes even on optree too), as any run-time SV upgrade/free will
touch the whole page. Furthermore, there're constant subs (sub foo ()
{42}), which are almost never called as methods - and by reverting
this we will loose memory savings on them.

The actual memory savings is minuscule. I'm not sure what you are
referring to with constant subs. Constants are more commonly created
via constant.pm, which uses a different optimization of storing scalar
refs in the symbol table. That optimization makes more sense since
they aren't commonly called as methods.

(I hope you will respond to Mr. Aleynikov’s point about copy-on-write. Such programs also suffer from modules that do autoloading.)

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 2, 2018

From @eserte

Dana Sat, 09 Dec 2017 02​:00​:58 -0800, andreas.koenig.7os6VVqR@​franz.ak.mind.de reče​:

Also affected​: MSCHWERN/Test-Sims-20130412.tar.gz

Discovered by Slaven.

http​://www.cpantesters.org/cpan/report/2d6c121a-dcb5-11e7-b48d-b8678c5b7d3e

Fix for Test-Sims​: schwern/Test-Sims#3

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 2, 2018

From @jkeenan

On 01/31/2018 01​:39 AM, slaven@​rezic.de via RT wrote​:

Here's a list of distributions which have an old ExtUtils​::Installer bundled. "perl Makefile.PL" fails for these with a "Not a GLOB reference" error.

Spot-checking these, I find 'use inc​::Module​::Install;' -- but I don't
find ExtUtils​::Installer. (Only reference I can find to that is here​:
http​://codeverge.com/perl.qa/re-module-requirements-was-module-build-and-installin/100246)

  \* AUTRIJUS/LWP\-Authen\-Wsse\-0\.05\.tar\.gz
  \* BOBTFISH/Catalyst\-Plugin\-HTML\-Widget\-1\.1\.tar\.gz
  \* HCCHIEN/Yahoo\-CAS\-0\.2\.tar\.gz
  \* HCCHIEN/Yahoo\-Lifestyle\-0\.2\.tar\.gz
  \* INGY/Class\-Spiffy\-0\.15\.tar\.gz
  \* INGY/Perldoc\-0\.20\.tar\.gz
  \* LICHTKIND/Perl6\-Bible\-0\.37\.tar\.gz
  \* LUKEC/WWW\-Selenium\-Utils\-0\.09\.tar\.gz
  \* MARKSTOS/CGI\-Uploader\-2\.18\.tar\.gz
  \* MIYAGAWA/Kwiki\-Emoticon\-0\.03\.tar\.gz
@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 2, 2018

From @jkeenan

On Thu, 01 Feb 2018 20​:39​:43 GMT, slaven@​rezic.de wrote​:

[snip]

The following distributions cannot be built anymore because of an old
bundled inc/Module/Install.pm​:

* CFRANKS/HTML-Menu-Select-1.01.tar.gz
* CDENT/Kwiki/Kwiki-PageStats-0.06.tar.gz
* INGY/Kwiki-Wikiwyg-0.13.tar.gz
* XERN/Lingua-LinkParser-FindPath-0.01.tar.gz
* TPG/Net-Radius-PacketOrdered-1.53.tar.gz
* AUTRIJUS/Parse-SVNDiff-0.05.tar.gz
* HACHI/POE-Component-Client-LDAP-0.04.tar.gz
* TPG/Test-Singleton-1.02.tar.gz
* TPG/What-1.00.tar.gz
* BTROTT/WWW-Blog-Metadata-Icon-0.02.tar.gz

Spot-checking these, I find that none has had a new release since 2006.

Am I correct in thinking that, to keep these distros alive, the best advice to the authors would be, "Move off Module​::Install"?

Thank you very much.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 2, 2018

From @demerphq

On 29 Jan 2018 18​:01, "Sergey Aleynikov via RT" <perlbug-followup@​perl.org>
wrote​:

On Mon, 29 Jan 2018 05​:51​:38 -0800, haarg wrote​:

This seems like lot of breakage for such a tiny benefit.

All those modules are already broken on the current perl version (and some
versions back in time), they just don't test for it - as globs are already
not created for subs in the 'main' package.

Additionally, since the stash entries get de-optimized to globs when
called as methods, this change will actually lead to increased memory
usage on servers that fork. Many methods won't get called until post-
fork, so the globs will need to be created for every fork instead of
being able to benefit from copy-on-write.

You can't rely on copy-on-write for almost all non-optree data (and
sometimes even on optree too), as any run-time SV upgrade/free will touch
the whole page.

It will touch a whole page of Sv *heads*, which are slab allocated anyway.

So I don't follow your point.

Furthermore

, there're constant subs (sub foo () {42}), which are almost never called
as methods - and by reverting this we will loose memory savings on them

That is a piffle compared to the methods that will get forked. If we keep
this optimization $work will disable it for sure in our builds. Making our
code even less fork friendly is imo not a step forward.

Yves

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 2, 2018

From @eserte

Dana Fri, 02 Feb 2018 09​:26​:57 -0800, jkeenan@​pobox.com reče​:

On 01/31/2018 01​:39 AM, slaven@​rezic.de via RT wrote​:

Here's a list of distributions which have an old ExtUtils​::Installer
bundled. "perl Makefile.PL" fails for these with a "Not a GLOB
reference" error.

Spot-checking these, I find 'use inc​::Module​::Install;' -- but I don't
find ExtUtils​::Installer. (Only reference I can find to that is here​:
http​://codeverge.com/perl.qa/re-module-requirements-was-module-build-
and-installin/100246)

Sorry, I really meant Module​::Install, not ExtUtils​::Installer.

* AUTRIJUS/LWP-Authen-Wsse-0.05.tar.gz
* BOBTFISH/Catalyst-Plugin-HTML-Widget-1.1.tar.gz
* HCCHIEN/Yahoo-CAS-0.2.tar.gz
* HCCHIEN/Yahoo-Lifestyle-0.2.tar.gz
* INGY/Class-Spiffy-0.15.tar.gz
* INGY/Perldoc-0.20.tar.gz
* LICHTKIND/Perl6-Bible-0.37.tar.gz
* LUKEC/WWW-Selenium-Utils-0.09.tar.gz
* MARKSTOS/CGI-Uploader-2.18.tar.gz
* MIYAGAWA/Kwiki-Emoticon-0.03.tar.gz

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 4, 2018

From @xsawyerx

I'm late here (had a serious move over the week) but I've already
spoken about this with Graham last week and we should revert this for
this release. I tried sending it earlier but I'm also having
networking issues. :)

The problem is not the conversation about which way is more optimized,
how, and why. It is simply how much breakage we have incurred with
this.

It should be reverted and we can reapproach it next release.

On 31 January 2018 at 20​:29, Dave Mitchell <davem@​iabyn.com> wrote​:

On Tue, Jan 30, 2018 at 10​:39​:30PM -0800, slaven@​rezic.de via RT wrote​:

Here's a list of distributions which have an old ExtUtils​::Installer bundled. "perl Makefile.PL" fails for these with a "Not a GLOB reference" error.

 \* AUTRIJUS/LWP\-Authen\-Wsse\-0\.05\.tar\.gz
 \* BOBTFISH/Catalyst\-Plugin\-HTML\-Widget\-1\.1\.tar\.gz
 \* HCCHIEN/Yahoo\-CAS\-0\.2\.tar\.gz
 \* HCCHIEN/Yahoo\-Lifestyle\-0\.2\.tar\.gz
 \* INGY/Class\-Spiffy\-0\.15\.tar\.gz
 \* INGY/Perldoc\-0\.20\.tar\.gz
 \* LICHTKIND/Perl6\-Bible\-0\.37\.tar\.gz
 \* LUKEC/WWW\-Selenium\-Utils\-0\.09\.tar\.gz
 \* MARKSTOS/CGI\-Uploader\-2\.18\.tar\.gz
 \* MIYAGAWA/Kwiki\-Emoticon\-0\.03\.tar\.gz

I feel that we have too much breakage too late in the release cycle,
and that we should consider reverting this optimisation for now.

--
The Enterprise's efficient long-range scanners detect a temporal vortex
distortion in good time, allowing it to be safely avoided via a minor
course correction.
-- Things That Never Happen in "Star Trek" #21

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 4, 2018

From @cpansprout

On Sun, 04 Feb 2018 01​:51​:53 -0800, xsawyerx@​gmail.com wrote​:

I'm late here (had a serious move over the week) but I've already
spoken about this with Graham last week and we should revert this for
this release. I tried sending it earlier but I'm also having
networking issues. :)

The problem is not the conversation about which way is more optimized,
how, and why. It is simply how much breakage we have incurred with
this.

It should be reverted and we can reapproach it next release.

I have disabled it in 1e2cfe1.

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Feb 4, 2018

From @xsawyerx

On 02/04/2018 10​:22 PM, Father Chrysostomos via RT wrote​:

On Sun, 04 Feb 2018 01​:51​:53 -0800, xsawyerx@​gmail.com wrote​:

I'm late here (had a serious move over the week) but I've already
spoken about this with Graham last week and we should revert this for
this release. I tried sending it earlier but I'm also having
networking issues. :)

The problem is not the conversation about which way is more optimized,
how, and why. It is simply how much breakage we have incurred with
this.

It should be reverted and we can reapproach it next release.
I have disabled it in 1e2cfe1.

Thank you.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 19, 2018

From @iabyn

On Sun, Feb 04, 2018 at 12​:22​:31PM -0800, Father Chrysostomos via RT wrote​:

On Sun, 04 Feb 2018 01​:51​:53 -0800, xsawyerx@​gmail.com wrote​:

I'm late here (had a serious move over the week) but I've already
spoken about this with Graham last week and we should revert this for
this release. I tried sending it earlier but I'm also having
networking issues. :)

The problem is not the conversation about which way is more optimized,
how, and why. It is simply how much breakage we have incurred with
this.

It should be reverted and we can reapproach it next release.

I have disabled it in 1e2cfe1.

I'll remove this ticket from the 5.28 blockers list;

--
A problem shared is a problem doubled.

@toddr
Copy link
Member

@toddr toddr commented Feb 17, 2020

Given this was reverted, I assume the case can now be closed. If we want to put the optimization back, maybe we're lacking an issue to track that?

@toddr toddr added Closable? and removed BBC labels Feb 17, 2020
@jkeenan
Copy link
Contributor

@jkeenan jkeenan commented Feb 17, 2020

Given this was reverted, I assume the case can now be closed. If we want to put the optimization back, maybe we're lacking an issue to track that?

This "optimization" caused a considerable amount of CPAN breakage and a lot of discussion which has to be reviewed whenever anyone looks at this ticket. So if there is anyone interested in pursuing a revival of that work, they should do so in a fork or a branch.

Closing.

Thank you very much.
Jim Keenan

@jkeenan jkeenan closed this Feb 17, 2020
@jkeenan jkeenan removed the Closable? label Feb 17, 2020
@xsawyerx
Copy link
Member

@xsawyerx xsawyerx commented Feb 18, 2020

This is a good time if anyone believes this optimization is valuable enough to open the discussion. (This is what it's really about.)

@iabyn
Copy link
Contributor

@iabyn iabyn commented Feb 18, 2020

@dur-randir
Copy link
Member

@dur-randir dur-randir commented Feb 18, 2020

Given this was reverted, I assume the case can now be closed. If we want to put the optimization back, maybe we're lacking an issue to track that?

This "optimization" caused a considerable amount of CPAN breakage and a lot of discussion which has to be reviewed whenever anyone looks at this ticket. So if there is anyone interested in pursuing a revival of that work, they should do so in a fork or a branch.

Closing.

Thank you very much.
Jim Keenan

Do you mean that broken modules should not be fixed just because of there's a lot of them?

@hvds
Copy link
Contributor

@hvds hvds commented Feb 18, 2020

I appreciate the enormous amount of effort @cpansprout went to in fixing broken modules. I feel we'd benefit from having this patch if it isn't actively pessimal, since it would expose those existing bugs and help avoid the same mistake being made by future CPAN and darkPAN authors - but if we wanted to roll it out again, it seems likely there would still be a lot more such fixing to do, and that's unlikely to happen unless it has a champion willing to do much of the work.

But I agree with Jim that any such work would be best started in a branch, at least to the point that enough of the upstream dependencies have fixes to allow us to get a reasonably complete picture of the size of the remaining downstream problem.

Hugo

@haarg
Copy link
Contributor

@haarg haarg commented Feb 18, 2020

While I'm a bit dubious of this change in general, the high impact CPAN dists have been fixed, so it hopefully wouldn't impede general smoke testing of CPAN.

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

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.