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

Remove Config from Storable by hardcoding CAN_FLOCK #16227

Closed
p5pRT opened this issue Nov 6, 2017 · 13 comments
Closed

Remove Config from Storable by hardcoding CAN_FLOCK #16227

p5pRT opened this issue Nov 6, 2017 · 13 comments

Comments

@p5pRT
Copy link

p5pRT commented Nov 6, 2017

Migrated from rt.perl.org#132406 (status was 'resolved')

Searchable as RT132406$

@p5pRT
Copy link
Author

p5pRT commented Nov 6, 2017

From @atoomic

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​:
atoomic@9033a55

Does anyone have any concern with such a solution? Are you aware of any limitations/problems this might raise?

@p5pRT
Copy link
Author

p5pRT commented Nov 6, 2017

From @atoomic

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)

@p5pRT
Copy link
Author

p5pRT commented Nov 6, 2017

From @cpansprout

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​:
atoomic@9033a55

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

@p5pRT
Copy link
Author

p5pRT commented Nov 6, 2017

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

@p5pRT
Copy link
Author

p5pRT commented Nov 6, 2017

From @atoomic

On Mon, 06 Nov 2017 14​:35​:47 -0800, sprout wrote​:

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​:
atoomic@9033a55

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.

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.

@p5pRT
Copy link
Author

p5pRT commented Nov 7, 2017

From @demerphq

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-archive.perl.org/perl5/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​:
atoomic@9033a55
ccdac43c72

Does anyone have any concern with such a solution? Are you aware of any
limitations/problems this might raise?

From 9033a55 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)

@p5pRT
Copy link
Author

p5pRT commented Nov 7, 2017

From @atoomic

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-archive.perl.org/perl5/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​:
atoomic@9033a55
ccdac43c72

Does anyone have any concern with such a solution? Are you aware of any
limitations/problems this might raise?

From 9033a55 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)

@p5pRT
Copy link
Author

p5pRT commented Nov 7, 2017

From @atoomic

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​:
atoomic@977a333

thanks for any feedbacks

On Tue, 07 Nov 2017 09​:38​:48 -0800, atoomic@​cpan.org wrote​:

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-archive.perl.org/perl5/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​:
atoomic@9033a55
ccdac43c72

Does anyone have any concern with such a solution? Are you aware of
any
limitations/problems this might raise?

From 9033a55 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)

@p5pRT
Copy link
Author

p5pRT commented Nov 7, 2017

From @atoomic

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)

@p5pRT
Copy link
Author

p5pRT commented Nov 14, 2017

From @atoomic

merged to blead with ad2ec6b

@p5pRT
Copy link
Author

p5pRT commented Nov 14, 2017

@atoomic - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented Jun 23, 2018

From @khwilliamson

Thank 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
resolved.

Perl 5.28.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.28.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Author

p5pRT commented Jun 23, 2018

@khwilliamson - Status changed from 'pending release' to 'resolved'

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

No branches or pull requests

1 participant