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.3-232-g10f9b9b breaks LEONT/Const-Fast-0.014.tar.gz #15522

Closed
p5pRT opened this issue Aug 16, 2016 · 11 comments
Closed

Bleadperl v5.25.3-232-g10f9b9b breaks LEONT/Const-Fast-0.014.tar.gz #15522

p5pRT opened this issue Aug 16, 2016 · 11 comments
Labels
BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) Closable? We might be able to close this ticket, but we need to check with the reporter

Comments

@p5pRT
Copy link

p5pRT commented Aug 16, 2016

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

Searchable as RT128966$

@p5pRT
Copy link
Author

p5pRT commented Aug 16, 2016

From @andk

bisect


commit 10f9b9b
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Mon Aug 8 18​:53​:20 2016 +0200

  move Internals​::hv_clear_placeholders() to Hash​::Util​::_clear_placeholders()

diagnostics


http​://www.cpantesters.org/cpan/report/40ee7106-62a0-11e6-b236-c19558b9f28c

perl -V


Summary of my perl5 (revision 5 version 25 subversion 4) configuration​:
  Commit id​: 2e66fe9
  Platform​:
  osname=linux
  osvers=4.3.0-1-amd64
  archname=x86_64-linux
  uname='linux k83 4.3.0-1-amd64 #1 smp debian 4.3.3-7 (2016-01-19) x86_64 gnulinux '
  config_args='-Dprefix=/home/sand/src/perl/repoperls/installed-perls/perl/v5.25.3-245-g2e66fe9/79cc -Dmyhostname=k83 -Dinstallusrbinperl=n -Uversiononly -Dusedevel -des -Ui_db -Dlibswanted=cl pthread socket inet nsl gdbm dbm malloc dl ld sun m crypt sec util c cposix posix ucb BSD gdbm_compat -Uuseithreads -Uuselongdouble -DDEBUGGING=-g'
  hint=recommended
  useposix=true
  d_sigaction=define
  useithreads=undef
  usemultiplicity=undef
  use64bitint=define
  use64bitall=define
  uselongdouble=undef
  usemymalloc=n
  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 -D_FORTIFY_SOURCE=2'
  optimize='-O2 -g'
  cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
  ccversion=''
  gccversion='5.3.1 20160121'
  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
  libs=-lpthread -lnsl -lgdbm -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.21.so
  so=so
  useshrplib=false
  libperl=libperl.a
  gnulibc_version='2.21'
  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​:
  HAS_TIMES
  PERLIO_LAYERS
  PERL_COPY_ON_WRITE
  PERL_DONT_CREATE_GVSV
  PERL_HASH_FUNC_ONE_AT_A_TIME_HARD
  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 Aug 15 2016 04​:11​:41
  %ENV​:
  PERL5LIB=""
  PERL5OPT=""
  PERL5_CPANPLUS_IS_RUNNING="29355"
  PERL5_CPAN_IS_RUNNING="29355"
  PERL_CANARY_STABILITY_NOPROMPT="1"
  PERL_MM_USE_DEFAULT="1"
  @​INC​:
  /home/sand/src/perl/repoperls/installed-perls/perl/v5.25.3-245-g2e66fe9/79cc/lib/site_perl/5.25.4/x86_64-linux
  /home/sand/src/perl/repoperls/installed-perls/perl/v5.25.3-245-g2e66fe9/79cc/lib/site_perl/5.25.4
  /home/sand/src/perl/repoperls/installed-perls/perl/v5.25.3-245-g2e66fe9/79cc/lib/5.25.4/x86_64-linux
  /home/sand/src/perl/repoperls/installed-perls/perl/v5.25.3-245-g2e66fe9/79cc/lib/5.25.4
  .
--
andreas

@p5pRT
Copy link
Author

p5pRT commented Aug 23, 2016

From @eserte

Also affected​: BMORROW/Readonly-Tiny-3.tar.gz
See https://rt.cpan.org/Ticket/Display.html?id=117170

@p5pRT
Copy link
Author

p5pRT commented Aug 23, 2016

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

@p5pRT
Copy link
Author

p5pRT commented Aug 24, 2016

From @Leont

On Tue, Aug 16, 2016 at 9​:44 PM, Andreas J. Koenig via RT <
perlbug-followup@​perl.org> wrote​:

bisect
------
commit 10f9b9b
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Mon Aug 8 18​:53​:20 2016 +0200

move Internals&#8203;::hv\_clear\_placeholders\(\) to

Hash​::Util​::_clear_placeholders()

diagnostics
-----------
http​://www.cpantesters.org/cpan/report/40ee7106-62a0-11e6-
b236-c19558b9f28c

I think the removal from Internals​:: was a bit premature. This function is
useful outside of Hash​::Util in one particular circumstance​: when making a
hash readonly (or actually restricted). Moving this but leaving SvREADONLY
is suboptimal, what you really want is make_const function that takes care
of this if it's a hash.

Or actually, what would be even better is having a sensible split between
readonly and restricted hashes, but that's a different discussion.

Leon

@p5pRT
Copy link
Author

p5pRT commented Aug 26, 2016

From @andk

also affected​: TOBYINK/Sub-Trigger-Lock-0.002.tar.gz

--
andreas

@p5pRT
Copy link
Author

p5pRT commented Sep 19, 2016

From @demerphq

On 24 August 2016 at 16​:41, Leon Timmermans <fawaka@​gmail.com> wrote​:

On Tue, Aug 16, 2016 at 9​:44 PM, Andreas J. Koenig via RT
<perlbug-followup@​perl.org> wrote​:

bisect
------
commit 10f9b9b
Author​: Yves Orton <demerphq@​gmail.com>
Date​: Mon Aug 8 18​:53​:20 2016 +0200

move Internals&#8203;::hv\_clear\_placeholders\(\) to

Hash​::Util​::_clear_placeholders()

diagnostics
-----------

http​://www.cpantesters.org/cpan/report/40ee7106-62a0-11e6-b236-c19558b9f28c

I think the removal from Internals​:: was a bit premature.

TL;DR​: It was readded some time ago. But if you are going to use
undocumented internals features and unroll official functionality to
reimplement or extend that functionality then IMO you get to keep the
pieces when the internals changes.

This function is
useful outside of Hash​::Util in one particular circumstance​: when making a
hash readonly (or actually restricted). Moving this but leaving SvREADONLY
is suboptimal, what you really want is make_const function that takes care
of this if it's a hash.

I have a few issues with this.

First off, Internals​::hv_clear_placehold() is only needed to get rid
of placeholders stored in an *ALREADY* restricted hash.

So, if your code is meant to mark a data structure readonly then it is
not needed.

Second, as far as I can tell the code in Const​::Fast overlaps with and
extends Hash​::Util, and as the official place to expose
readonly/restricted hashes IMO you should have either used Hash​::Util,
or patched Hash​::Util to offer a way to recursively handle other types
or to perhaps expose hv_clear_placeholders() via Hash​::Util so you
could use it as a documented function.

However what you have done in this module is use undocumented
internals routines to unroll the official functionality. In theory as
undocumented internals functions we should be able to remove or change
Internals​::hv_clear_placehold() and Internals​::SvREADONLY() in any way
we choose, like I did, and not break any well written code. But you
have decided to violate the encapsulation and as far as I am concerned
when you do that you get to keep both pieces. (Like I do in
Data​::Dump​::Streamer or Sereal when something internals changes.)

Third, I think that functionality like Const​::Fast should be coded in
XS, and be part of the core. If its meant to be fast then it should be
XS, and since it didles internals flags that have uncertain and
ambiguous meanings (such as the restricted/readonly ambiguity) it
should probably be part of core and maintained as part of core.
Essentially you are using undocumented functions to fill a
functionalitry vacuum that core has not filled. I think it would be
better for all if this functionality lived in core.

Having said all that​: I re-added hv_clear_placehold() back to
Internals some months back. I personally think we should mark the
Internals functions deprecated and make them throw deprecation
warnings and require people to use well defined and well documented
functions to achieve the same results.

Or actually, what would be even better is having a sensible split between
readonly and restricted hashes, but that's a different discussion.

I think we should ditch restricted hashes, and just have what you
would call readonly hashes. But as you say that is a different
discussion.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Dec 26, 2016

From @jkeenan

On Fri, 26 Aug 2016 06​:15​:37 GMT, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

also affected​: TOBYINK/Sub-Trigger-Lock-0.002.tar.gz

Appears to have been fixed at least as of​:

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

I was able to use 'cpanm' to install the distro at this point in blead.

Ticket not yet closable until the Const-Fast discussion is resolved.

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

@p5pRT
Copy link
Author

p5pRT commented Mar 20, 2017

From @iabyn

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

On Fri, 26 Aug 2016 06​:15​:37 GMT, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

also affected​: TOBYINK/Sub-Trigger-Lock-0.002.tar.gz

Appears to have been fixed at least as of​:

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

I was able to use 'cpanm' to install the distro at this point in blead.

Ticket not yet closable until the Const-Fast discussion is resolved.

But unless I've misunderstood this thread, this ticket is no longer a
5.26.0 blocker.

--
I before E. Except when it isn't.

@p5pRT
Copy link
Author

p5pRT commented Mar 20, 2017

From @jkeenan

On Mon, 20 Mar 2017 12​:59​:21 GMT, davem wrote​:

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

On Fri, 26 Aug 2016 06​:15​:37 GMT,
andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

also affected​: TOBYINK/Sub-Trigger-Lock-0.002.tar.gz

Appears to have been fixed at least as of​:

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

I was able to use 'cpanm' to install the distro at this point in
blead.

Ticket not yet closable until the Const-Fast discussion is resolved.

But unless I've misunderstood this thread, this ticket is no longer a
5.26.0 blocker.

Agreed; removing from 5.26.0 blockers list.

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

@p5pRT p5pRT added BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) Severity Low labels Oct 19, 2019
@toddr
Copy link
Member

toddr commented Feb 17, 2020

From @jkeenan

Ticket not yet closable until the Const-Fast discussion is resolved.

@Leont is this closeable or is further discussion/decisioning needed?

@toddr toddr added the Closable? We might be able to close this ticket, but we need to check with the reporter label Feb 17, 2020
@Leont
Copy link
Contributor

Leont commented Feb 17, 2020

I think it's closable, though at some point we may need to have a policy around Internals::

@Leont Leont closed this as completed Feb 17, 2020
@toddr toddr reopened this Feb 17, 2020
@toddr toddr closed this as completed Feb 17, 2020
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) Closable? We might be able to close this ticket, but we need to check with the reporter
Projects
None yet
Development

No branches or pull requests

3 participants