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
Remove Config from Storable by hardcoding CAN_FLOCK #16227
Comments
From @atoomicThis idea is coming following a discussion we had during last perl 5 meeting in Amsterdam. After discussion, it seems sane to simply assume that we can flock on any Linux system. I'm opening a discussion here by providing this attached patch, that I consider submitting to blead. readable online here: Does anyone have any concern with such a solution? Are you aware of any limitations/problems this might raise? |
From @atoomic0001-assume-any-linux-darwin-system-can-flock.patchFrom 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)
|
From @cpansproutOn Mon, 06 Nov 2017 14:25:33 -0800, atoomic@cpan.org wrote:
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 |
The RT System itself - Status changed from 'new' to 'open' |
From @atoomicOn Mon, 06 Nov 2017 14:35:47 -0800, sprout wrote:
That CAN_FLOCK is just used in two functions after one open call Maybe a better option would be to let flock fail by itself? and remove that extra check.
I agree using a template Storable_pm.PL to generate Storable_pm is a better solution. 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 @demerphqCan 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:
|
From @atoomicIndeed 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... 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:
|
From @atoomicAfter the last exchanges, I came with this updated patch. An online version is also available on GitHub here: thanks for any feedbacks On Tue, 07 Nov 2017 09:38:48 -0800, atoomic@cpan.org wrote:
|
From @atoomic0001-Storable-remove-Config-dependency.patchFrom 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)
|
@atoomic - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release yesterday of Perl 5.28.0, this and 185 other issues have been Perl 5.28.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#132406 (status was 'resolved')
Searchable as RT132406$
The text was updated successfully, but these errors were encountered: