Skip Menu |

This queue is for tickets about the Struct-Dumb CPAN distribution.

Report information
The Basics
Id: 132378
Status: open
Priority: 0/
Queue: Struct-Dumb
People
Owner: Nobody in particular
Requestors: corion [...] cpan.org
Cc:
AdminCc:
Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)

History Show all quoted text
  Sun Apr 19 07:51:41 2020 corion [...] cpan.org - Ticket created
Subject: Global Destruction not properly detected (only?) on Travis (!?)
Struct::Dumb is an optional upstream prerequisite of WWW::Mechanize::Chrome and it fails (only on Travis) a compile test, because there, Struct::Dumb tries to load Data::Dump::Filtered from the destructor, see the error message below. This is really weird - the offending test merely loads the module to check it compiles, and that triggers the incorrect global destruction detection. It happens on Perl 5.22 through 5.30, but not on the system perl (5.22 as well). Maybe using ${^GLOBAL_PHASE} eq "DESTRUCT" is better, at least for Perl versions above 5.14 ? A fairly reduced case to reproduce this is just loading Struct::Dumb: package StructDumbDebug; use strict; #use IO::Async::Loop; #use IO::Async::Stream; use Struct::Dumb; our $VERSION = '0.48'; 1; If I can be of help here, please tell me - hopefully, the cause is obvious to you, because I'm not sure what the difference between the Perl versions is that makes your detection of Global Destruction different from ${^GLOBAL_PHASE} and why this breaks now (instead of earlier). Thanks, -max https://travis-ci.org/github/Corion/WWW-Mechanize-Chrome/jobs/676846818 # lib/StructDumbDebug.pm syntax OK # (in cleanup) Can't locate Data/Dump/Filtered.pm in @INC (you may need to install the Data::Dump::Filtered module) (@INC contains: /home/travis/build/Corion/WWW-Mechanize-Chrome/blib/arch /home/travis/build/Corion/WWW-Mechanize-Chrome/blib/lib /home/travis/build/Corion/WWW-Mechanize-Chrome/lib /home/travis/build/Corion/WWW-Mechanize-Chrome/blib/lib /home/travis/build/Corion/WWW-Mechanize-Chrome/blib/arch /home/travis/perl5/perlbrew/perls/5.22.4/lib/site_perl/5.22.4/x86_64-linux /home/travis/perl5/perlbrew/perls/5.22.4/lib/site_perl/5.22.4 /home/travis/perl5/perlbrew/perls/5.22.4/lib/5.22.4/x86_64-linux /home/travis/perl5/perlbrew/perls/5.22.4/lib/5.22.4 .) at /home/travis/perl5/perlbrew/perls/5.22.4/lib/site_perl/5.22.4/Struct/Dumb.pm line 364 during global destruction. not ok 29 - lib/StructDumbDebug.pm
  Mon Apr 20 07:13:12 2020 haarg [...] haarg.org - Correspondence added
This will happen on any system that doesn't have Data::Dump installed. Data::Dump seems to be a test prerequisite, but is not listed as such. One easy fix for the module itself is checking that Data::Dump was actually loaded before trying to continue loading, rather than checking for global destruction. { package Struct::Dumb::_DestroyWatch; sub DESTROY { ${$_[0]}->() if $INC{"Data/Dump.pm"} } } Data::Dump should either be added to the test prerequisites, or the test using it should skip if it is not installed. Everything regarding overloading in this module seems really questionable. None of this would be required if you just let it be a normal object.
  Mon Apr 20 07:13:13 2020 The RT System itself - Status changed from 'new' to 'open'
  Mon Apr 20 18:59:02 2020 leonerd-cpan [...] leonerd.org.uk - Correspondence added
On Mon Apr 20 07:13:12 2020, haarg wrote: Show quoted text
> This will happen on any system that doesn't have Data::Dump installed. > > Data::Dump seems to be a test prerequisite, but is not listed as such.
Oh oops. I'm usually good at remembering to add "Test::" modules to the list, but I forgot to add this one. Show quoted text
> One easy fix for the module itself is checking that Data::Dump was > actually loaded before trying to continue loading, rather than > checking for global destruction. > > { > package Struct::Dumb::_DestroyWatch; > sub DESTROY { ${$_[0]}->() if $INC{"Data/Dump.pm"} } > }
Yeah that seems good. Show quoted text
> Data::Dump should either be added to the test prerequisites, or the > test using it should skip if it is not installed.
Might as well just have it skip to be honest. No need to force it to be installed if the user isn't going to be using it. If they didn't have it installed they wouldn't need to check the filtering worked anyway :) Show quoted text
> Everything regarding overloading in this module seems really > questionable. None of this would be required if you just let it be a > normal object.
Well the point of overloading `@{}` is to forbid most other code from using these instances as if they were plain arrayrefs. It helps when replacing existing code that might have been using those, in case of some stray `$obj->[0]` style code sitting around. But then having done that, Data::Dump can't look into them any more, so I added the filter to put that back and at the same time neaten the output to match the construction function style. -- Paul Evans
  Mon Apr 20 19:08:53 2020 leonerd-cpan [...] leonerd.org.uk - Correspondence added
On Mon Apr 20 18:59:02 2020, PEVANS wrote: Show quoted text
> > Data::Dump seems to be a test prerequisite, but is not listed as > > such.
> > Oh oops. I'm usually good at remembering to add "Test::" modules to > the list, but I forgot to add this one.
Correction: It's right here https://metacpan.org/source/PEVANS/Struct-Dumb-0.11/Build.PL#L9 Show quoted text
> Might as well just have it skip to be honest. No need to force it to > be installed if the user isn't going to be using it. If they didn't > have it installed they wouldn't need to check the filtering worked > anyway :)
That said, it's not required other than to check if it works so I still might as well make it optional. -- Paul Evans
  Tue Apr 21 04:53:11 2020 haarg [...] haarg.org - Correspondence added
On Mon Apr 20 19:08:53 2020, PEVANS wrote: Show quoted text
> On Mon Apr 20 18:59:02 2020, PEVANS wrote:
> > > Data::Dump seems to be a test prerequisite, but is not listed as > > > such.
> > > > Oh oops. I'm usually good at remembering to add "Test::" modules to > > the list, but I forgot to add this one.
> > Correction: It's right here > > https://metacpan.org/source/PEVANS/Struct-Dumb-0.11/Build.PL#L9
Oh woops, missed that. Still, using filters requires version 1.16, so the checks should be adjusted to consider that. Show quoted text
>
> > Might as well just have it skip to be honest. No need to force it to > > be installed if the user isn't going to be using it. If they didn't > > have it installed they wouldn't need to check the filtering worked > > anyway :)
> > That said, it's not required other than to check if it works so I > still might as well make it optional.