Skip Menu |
Report information
Id: 132406
Status: pending release
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: atoomic [at] cpan.org
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type: unknown
Perl Version: (no value)
Fixed In: (no value)

Attachments


Subject: Remove Config from Storable by hardcoding CAN_FLOCK
Download (untitled) / with headers
text/plain 716b
This idea is coming following a discussion we had during last perl 5 meeting in Amsterdam. cPanel is used to remove the extra 'Config' requirement from Storable which will trigger Config_heavy to be loaded, by hardcoding 'sub CAN_FLOCK { return 1 }' # only for linux system After discussion, it seems sane to simply assume that we can flock on any Linux system. I've also added Darwin to the list. I'm opening a discussion here by providing this attached patch, that I consider submitting to blead. readable online here: https://github.com/atoomic/perl5/commit/9033a5571461e739babfa292a376c2ccdac43c72 Does anyone have any concern with such a solution? Are you aware of any limitations/problems this might raise?
Subject: 0001-assume-any-linux-darwin-system-can-flock.patch
From 9033a5571461e739babfa292a376c2ccdac43c72 Mon Sep 17 00:00:00 2001 From: Nicolas R <atoomic@cpan.org> Date: Mon, 6 Nov 2017 15:05:17 -0700 Subject: [PATCH] assume any linux/darwin system can flock Avoid loading Config_heavy from Storable. We can probably safely assume that any linux or darwin system can Flock at this point. --- dist/Storable/Storable.pm | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm index ae96a6c7ed..dacd3a3d03 100644 --- a/dist/Storable/Storable.pm +++ b/dist/Storable/Storable.pm @@ -83,14 +83,8 @@ XSLoader::load('Storable', $Storable::VERSION); # Determine whether locking is possible, but only when needed. # -sub CAN_FLOCK; my $CAN_FLOCK; sub CAN_FLOCK { - return $CAN_FLOCK if defined $CAN_FLOCK; - require Config; import Config; - return $CAN_FLOCK = - $Config{'d_flock'} || - $Config{'d_fcntl_can_lock'} || - $Config{'d_lockf'}; -} +# assume any linux/darwin system can flock +use constant CAN_FLOCK => $^O =~ qr{^(?:linux|darwin)$} ? 1 : 0; sub show_file_magic { print <<EOM; -- 2.13.6 (Apple Git-96)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.1k
On Mon, 06 Nov 2017 14:25:33 -0800, atoomic@cpan.org wrote: Show quoted text
> This idea is coming following a discussion we had during last perl 5 > meeting in Amsterdam. > cPanel is used to remove the extra 'Config' requirement from Storable > which will trigger > Config_heavy to be loaded, by hardcoding 'sub CAN_FLOCK { return 1 }' > # only for linux system > > After discussion, it seems sane to simply assume that we can flock on > any Linux system. > I've also added Darwin to the list. > > I'm opening a discussion here by providing this attached patch, that I > consider submitting to blead. > > readable online here: > https://github.com/atoomic/perl5/commit/9033a5571461e739babfa292a376c2ccdac43c72 > > Does anyone have any concern with such a solution? Are you aware of > any limitations/problems this might raise?
The first problem with the patch as it is (and I realize it is intended for discussion, and not for application per se) is that it assumed that any system other than linux or darwin cannot flock. In any case, for maximum maintainability, wouldn’t it be better to do this detection in a Storable_pm.PL that generates Storable.pm, as we do for XS::Loader? -- Father Chrysostomos
Download (untitled) / with headers
text/plain 1.9k
On Mon, 06 Nov 2017 14:35:47 -0800, sprout wrote: Show quoted text
> On Mon, 06 Nov 2017 14:25:33 -0800, atoomic@cpan.org wrote:
> > This idea is coming following a discussion we had during last perl 5 > > meeting in Amsterdam. > > cPanel is used to remove the extra 'Config' requirement from Storable > > which will trigger > > Config_heavy to be loaded, by hardcoding 'sub CAN_FLOCK { return 1 }' > > # only for linux system > > > > After discussion, it seems sane to simply assume that we can flock on > > any Linux system. > > I've also added Darwin to the list. > > > > I'm opening a discussion here by providing this attached patch, that > > I > > consider submitting to blead. > > > > readable online here: > > https://github.com/atoomic/perl5/commit/9033a5571461e739babfa292a376c2ccdac43c72 > > > > Does anyone have any concern with such a solution? Are you aware of > > any limitations/problems this might raise?
> > The first problem with the patch as it is (and I realize it is > intended for discussion, and not for application per se) is that it > assumed that any system other than linux or darwin cannot flock. >
That CAN_FLOCK is just used in two functions after one open call and before a flock call to return earlier if the system does not provide flock. In both cases, it would not be able to read or write... and make the module partially useless... Maybe a better option would be to let flock fail by itself? and remove that extra check. Show quoted text
> In any case, for maximum maintainability, wouldn’t it be better to do > this detection in a Storable_pm.PL that generates Storable.pm, as we > do for XS::Loader?
I agree using a template Storable_pm.PL to generate Storable_pm is a better solution. Just probably make it a little harder on the maintenance side, but it's probably fine. As Storable is one XS module, there is no risk of fatpacking it and sending it to a different server :-) We could even keep using the same existing logic and would preserve the original behavior by removing that dependency. This sounds a good plan to me.
From: demerphq <demerphq [...] gmail.com>
To: Perl5 Porteros <perl5-porters [...] perl.org>
Subject: Re: [perl #132406] Remove Config from Storable by hardcoding CAN_FLOCK
Date: Tue, 7 Nov 2017 18:10:31 +0100
CC: bugs-bitbucket [...] rt.perl.org
Download (untitled) / with headers
text/plain 2.3k
Can we just move this setting out if config heavy into the lite config?

Just a thought....

Sorry for top posting!

Yves

On 6 Nov 2017 23:26, "Nicolas R." <perlbug-followup@perl.org> wrote:
Show quoted text
# New Ticket Created by  Nicolas R.
# Please include the string:  [perl #132406]
# in the subject line of all future correspondence about this issue.
# <URL: https://rt.perl.org/Ticket/Display.html?id=132406 >


This idea is coming following a discussion we had during last perl 5 meeting in Amsterdam.
cPanel is used to remove the extra 'Config' requirement from Storable which will trigger
Config_heavy to be loaded, by hardcoding 'sub CAN_FLOCK { return 1 }' # only for linux system

After discussion, it seems sane to simply assume that we can flock on any Linux system.
I've also added Darwin to the list.

I'm opening a discussion here by providing this attached patch, that I consider submitting to blead.

readable online here:
https://github.com/atoomic/perl5/commit/9033a5571461e739babfa292a376c2ccdac43c72

Does anyone have any concern with such a solution? Are you aware of any limitations/problems this might raise?
>From 9033a5571461e739babfa292a376c2ccdac43c72 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Mon, 6 Nov 2017 15:05:17 -0700
Subject: [PATCH] assume any linux/darwin system can flock

Avoid loading Config_heavy from Storable.
We can probably safely assume that any linux
or darwin system can Flock at this point.
---
 dist/Storable/Storable.pm | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm
index ae96a6c7ed..dacd3a3d03 100644
--- a/dist/Storable/Storable.pm
+++ b/dist/Storable/Storable.pm
@@ -83,14 +83,8 @@ XSLoader::load('Storable', $Storable::VERSION);
 # Determine whether locking is possible, but only when needed.
 #

-sub CAN_FLOCK; my $CAN_FLOCK; sub CAN_FLOCK {
-       return $CAN_FLOCK if defined $CAN_FLOCK;
-       require Config; import Config;
-       return $CAN_FLOCK =
-               $Config{'d_flock'} ||
-               $Config{'d_fcntl_can_lock'} ||
-               $Config{'d_lockf'};
-}
+# assume any linux/darwin system can flock
+use constant CAN_FLOCK => $^O =~ qr{^(?:linux|darwin)$} ? 1 : 0;

 sub show_file_magic {
     print <<EOM;
--
2.13.6 (Apple Git-96)


Download (untitled) / with headers
text/plain 3.1k
Indeed I also considered moving some keys (not only these ones) from heavy to light Config, which is a different topic and I think one analysis could be made to detect the most common ones... ( problem is to define the "most common", counting grep matches on CPAN is not good enough... some modules could be so generic that it should give a key some weight too... so this is all based on human assumption and depending on your programs... ) For now, I like the idea of using a Storable.pm.PL template, which will allow to preserve the existing logic, and reduce the Config bloat at runtime. On Tue, 07 Nov 2017 09:10:54 -0800, demerphq wrote: Show quoted text
> Can we just move this setting out if config heavy into the lite config? > > Just a thought.... > > Sorry for top posting! > > Yves > > On 6 Nov 2017 23:26, "Nicolas R." <perlbug-followup@perl.org> wrote: >
> > # New Ticket Created by Nicolas R. > > # Please include the string: [perl #132406] > > # in the subject line of all future correspondence about this issue. > > # <URL: https://rt.perl.org/Ticket/Display.html?id=132406 > > > > > > > This idea is coming following a discussion we had during last perl 5 > > meeting in Amsterdam. > > cPanel is used to remove the extra 'Config' requirement from Storable > > which will trigger > > Config_heavy to be loaded, by hardcoding 'sub CAN_FLOCK { return 1 }' # > > only for linux system > > > > After discussion, it seems sane to simply assume that we can flock on any > > Linux system. > > I've also added Darwin to the list. > > > > I'm opening a discussion here by providing this attached patch, that I > > consider submitting to blead. > > > > readable online here: > > https://github.com/atoomic/perl5/commit/9033a5571461e739babfa292a376c2 > > ccdac43c72 > > > > Does anyone have any concern with such a solution? Are you aware of any > > limitations/problems this might raise?
> > >From 9033a5571461e739babfa292a376c2ccdac43c72 Mon Sep 17 00:00:00 2001
> > From: Nicolas R <atoomic@cpan.org> > > Date: Mon, 6 Nov 2017 15:05:17 -0700 > > Subject: [PATCH] assume any linux/darwin system can flock > > > > Avoid loading Config_heavy from Storable. > > We can probably safely assume that any linux > > or darwin system can Flock at this point. > > --- > > dist/Storable/Storable.pm | 10 ++-------- > > 1 file changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm > > index ae96a6c7ed..dacd3a3d03 100644 > > --- a/dist/Storable/Storable.pm > > +++ b/dist/Storable/Storable.pm > > @@ -83,14 +83,8 @@ XSLoader::load('Storable', $Storable::VERSION); > > # Determine whether locking is possible, but only when needed. > > # > > > > -sub CAN_FLOCK; my $CAN_FLOCK; sub CAN_FLOCK { > > - return $CAN_FLOCK if defined $CAN_FLOCK; > > - require Config; import Config; > > - return $CAN_FLOCK = > > - $Config{'d_flock'} || > > - $Config{'d_fcntl_can_lock'} || > > - $Config{'d_lockf'}; > > -} > > +# assume any linux/darwin system can flock > > +use constant CAN_FLOCK => $^O =~ qr{^(?:linux|darwin)$} ? 1 : 0; > > > > sub show_file_magic { > > print <<EOM; > > -- > > 2.13.6 (Apple Git-96) > > > > > >
Download (untitled) / with headers
text/plain 3.7k
After the last exchanges, I came with this updated patch. Feel free to add some comments. Without any firm opposition, I think this is suitable to merge. An online version is also available on GitHub here: https://github.com/atoomic/perl5/commit/977a333c4c0a2303b7c6df1a752f495e437c78ff thanks for any feedbacks On Tue, 07 Nov 2017 09:38:48 -0800, atoomic@cpan.org wrote: Show quoted text
> Indeed I also considered moving some keys (not only these ones) from > heavy to light Config, which is a different topic and I think one > analysis could be made to detect the most common ones... > ( problem is to define the "most common", counting grep matches on > CPAN is not good enough... some modules could be so generic that it > should give a key some weight too... so this is all based on human > assumption and depending on your programs... ) > > For now, I like the idea of using a Storable.pm.PL template, which > will allow to preserve the existing logic, and reduce the Config bloat > at runtime. > > > On Tue, 07 Nov 2017 09:10:54 -0800, demerphq wrote:
> > Can we just move this setting out if config heavy into the lite > > config? > > > > Just a thought.... > > > > Sorry for top posting! > > > > Yves > > > > On 6 Nov 2017 23:26, "Nicolas R." <perlbug-followup@perl.org> wrote: > >
> > > # New Ticket Created by Nicolas R. > > > # Please include the string: [perl #132406] > > > # in the subject line of all future correspondence about this > > > issue. > > > # <URL: https://rt.perl.org/Ticket/Display.html?id=132406 > > > > > > > > > > This idea is coming following a discussion we had during last perl > > > 5 > > > meeting in Amsterdam. > > > cPanel is used to remove the extra 'Config' requirement from > > > Storable > > > which will trigger > > > Config_heavy to be loaded, by hardcoding 'sub CAN_FLOCK { return 1 > > > }' # > > > only for linux system > > > > > > After discussion, it seems sane to simply assume that we can flock > > > on any > > > Linux system. > > > I've also added Darwin to the list. > > > > > > I'm opening a discussion here by providing this attached patch, > > > that I > > > consider submitting to blead. > > > > > > readable online here: > > > https://github.com/atoomic/perl5/commit/9033a5571461e739babfa292a376c2 > > > ccdac43c72 > > > > > > Does anyone have any concern with such a solution? Are you aware of > > > any > > > limitations/problems this might raise?
> > > > From 9033a5571461e739babfa292a376c2ccdac43c72 Mon Sep 17 00:00:00 > > > > 2001
> > > From: Nicolas R <atoomic@cpan.org> > > > Date: Mon, 6 Nov 2017 15:05:17 -0700 > > > Subject: [PATCH] assume any linux/darwin system can flock > > > > > > Avoid loading Config_heavy from Storable. > > > We can probably safely assume that any linux > > > or darwin system can Flock at this point. > > > --- > > > dist/Storable/Storable.pm | 10 ++-------- > > > 1 file changed, 2 insertions(+), 8 deletions(-) > > > > > > diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm > > > index ae96a6c7ed..dacd3a3d03 100644 > > > --- a/dist/Storable/Storable.pm > > > +++ b/dist/Storable/Storable.pm > > > @@ -83,14 +83,8 @@ XSLoader::load('Storable', $Storable::VERSION); > > > # Determine whether locking is possible, but only when needed. > > > # > > > > > > -sub CAN_FLOCK; my $CAN_FLOCK; sub CAN_FLOCK { > > > - return $CAN_FLOCK if defined $CAN_FLOCK; > > > - require Config; import Config; > > > - return $CAN_FLOCK = > > > - $Config{'d_flock'} || > > > - $Config{'d_fcntl_can_lock'} || > > > - $Config{'d_lockf'}; > > > -} > > > +# assume any linux/darwin system can flock > > > +use constant CAN_FLOCK => $^O =~ qr{^(?:linux|darwin)$} ? 1 : 0; > > > > > > sub show_file_magic { > > > print <<EOM; > > > -- > > > 2.13.6 (Apple Git-96) > > > > > > > > >
Subject: 0001-Storable-remove-Config-dependency.patch
From 977a333c4c0a2303b7c6df1a752f495e437c78ff Mon Sep 17 00:00:00 2001 From: Nicolas R <atoomic@cpan.org> Date: Tue, 7 Nov 2017 12:22:27 -0600 Subject: [PATCH] Storable: remove Config dependency Avoid loading Config/Config_heavy from Storable. Make Storable.pm a template file and check if the system can use flock at compile time. __Storable__.pm is the template file to edit, whereas Storable.pm.PL is the script generating Storable.pm from __Storable__.pm. Using a separate file for the template make it easier to edit. Also note that Storable.pm is now ignored by git. --- MANIFEST | 3 ++- dist/Storable/.gitignore | 1 + dist/Storable/Makefile.PL | 18 +++++++++++-- dist/Storable/Storable.pm.PL | 35 ++++++++++++++++++++++++++ dist/Storable/{Storable.pm => __Storable__.pm} | 9 +------ 5 files changed, 55 insertions(+), 11 deletions(-) create mode 100644 dist/Storable/.gitignore create mode 100644 dist/Storable/Storable.pm.PL rename dist/Storable/{Storable.pm => __Storable__.pm} (99%) diff --git a/MANIFEST b/MANIFEST index cfa1ecdf93..06dbb2d337 100644 --- a/MANIFEST +++ b/MANIFEST @@ -3655,6 +3655,7 @@ dist/SelfLoader/lib/SelfLoader.pm Load functions only on demand dist/SelfLoader/t/01SelfLoader.t See if SelfLoader works dist/SelfLoader/t/02SelfLoader-buggy.t See if SelfLoader works dist/SelfLoader/t/03taint.t See if SelfLoader works under taint +dist/Storable/__Storable__.pm Template to generate Storable.pm dist/Storable/ChangeLog Storable extension dist/Storable/hints/gnukfreebsd.pl Hint for Storable for named architecture dist/Storable/hints/gnuknetbsd.pl Hint for Storable for named architecture @@ -3662,8 +3663,8 @@ dist/Storable/hints/hpux.pl Hint for Storable for named architecture dist/Storable/hints/linux.pl Hint for Storable for named architecture dist/Storable/Makefile.PL Storable extension dist/Storable/README Storable extension -dist/Storable/Storable.pm Storable extension dist/Storable/Storable.xs Storable extension +dist/Storable/Storable.pm.PL perl script to generate Storable.pm from template dist/Storable/t/attach.t Check STORABLE_attach doesn't create objects unnecessarily dist/Storable/t/attach_errors.t Trigger and test STORABLE_attach errors dist/Storable/t/attach_singleton.t Test STORABLE_attach for the Singleton pattern diff --git a/dist/Storable/.gitignore b/dist/Storable/.gitignore new file mode 100644 index 0000000000..acf5f9a324 --- /dev/null +++ b/dist/Storable/.gitignore @@ -0,0 +1 @@ +/Storable.pm diff --git a/dist/Storable/Makefile.PL b/dist/Storable/Makefile.PL index 23111299f5..a1f0b703da 100644 --- a/dist/Storable/Makefile.PL +++ b/dist/Storable/Makefile.PL @@ -13,17 +13,31 @@ WriteMakefile( DISTNAME => "Storable", # We now ship this in t/ # PREREQ_PM => { 'Test::More' => '0.41' }, + PL_FILES => { 'Storable.pm.PL' => 'Storable.pm' }, + PM => { 'Storable.pm' => '$(INST_ARCHLIB)/Storable.pm' }, PREREQ_PM => { XSLoader => 0 }, INSTALLDIRS => ($] >= 5.007 && $] < 5.012) ? 'perl' : 'site', - VERSION_FROM => 'Storable.pm', + VERSION_FROM => '__Storable__.pm', + ABSTRACT_FROM => '__Storable__.pm', ($ExtUtils::MakeMaker::VERSION > 6.45 ? (META_MERGE => { resources => - { bugtracker => 'http://rt.perl.org/perlbug/' } + { bugtracker => 'http://rt.perl.org/perlbug/' }, + provides => { + 'Storable' => { + file => 'Storable_pm.PL', + version => $NEW_VERSION, + }, + }, + }, ) : ()), dist => { SUFFIX => 'gz', COMPRESS => 'gzip -f' }, + clean => { FILES => 'Storable-* Storable.pm' }, ); +# Unlink the .pm file included with the distribution +1 while unlink "Storable.pm"; + my $ivtype = $Config{ivtype}; # I don't know if the VMS folks ever supported long long on 5.6.x diff --git a/dist/Storable/Storable.pm.PL b/dist/Storable/Storable.pm.PL new file mode 100644 index 0000000000..df979c09a9 --- /dev/null +++ b/dist/Storable/Storable.pm.PL @@ -0,0 +1,35 @@ +use strict; +use warnings; + +use Config; + +my $template; +{ # keep all the code in an external template to keep it easy to update + local $/; + open my $FROM, '<', '__Storable__.pm' or die $!; + $template = <$FROM>; + close $FROM or die $!; +} + +sub CAN_FLOCK { + return + $Config{'d_flock'} || + $Config{'d_fcntl_can_lock'} || + $Config{'d_lockf'} + ? 1 : 0; +} + +my $CAN_FLOCK = CAN_FLOCK(); + +# populate the sub and preserve it if used outside +$template =~ s{^sub CAN_FLOCK;.*$}{sub CAN_FLOCK { ${CAN_FLOCK} } # computed by Storable.pm.PL}m; +# alternatively we could remove the sub +#$template =~ s{^sub CAN_FLOCK;.*$}{}m; +# replace local function calls to hardcoded value +$template =~ s{&CAN_FLOCK}{${CAN_FLOCK}}g; + +{ + open my $OUT, '>', 'Storable.pm' or die $!; + print {$OUT} $template or die $!; + close $OUT or die $!; +} diff --git a/dist/Storable/Storable.pm b/dist/Storable/__Storable__.pm similarity index 99% rename from dist/Storable/Storable.pm rename to dist/Storable/__Storable__.pm index ae96a6c7ed..1c9b19c5c1 100644 --- a/dist/Storable/Storable.pm +++ b/dist/Storable/__Storable__.pm @@ -83,14 +83,7 @@ XSLoader::load('Storable', $Storable::VERSION); # Determine whether locking is possible, but only when needed. # -sub CAN_FLOCK; my $CAN_FLOCK; sub CAN_FLOCK { - return $CAN_FLOCK if defined $CAN_FLOCK; - require Config; import Config; - return $CAN_FLOCK = - $Config{'d_flock'} || - $Config{'d_fcntl_can_lock'} || - $Config{'d_lockf'}; -} +sub CAN_FLOCK; # TEMPLATE - replace by Storable.pm.PL sub show_file_magic { print <<EOM; -- 2.13.6 (Apple Git-96)
merged to blead with ad2ec6b54c9968999b7c70984b5a203fa72fd540


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org