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

[PATCH] Clean up Storable build process #17082

Closed
p5pRT opened this issue Jul 5, 2019 · 11 comments
Closed

[PATCH] Clean up Storable build process #17082

p5pRT opened this issue Jul 5, 2019 · 11 comments

Comments

@p5pRT
Copy link

p5pRT commented Jul 5, 2019

Migrated from rt.perl.org#134265 (status was 'pending release')

Searchable as RT134265$

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2019

From @haarg

The build process for Storable is currently needlessly complex. It
involves using a module as a template and has remnants of other build
time code that has since been removed.

These two patches clean up the remnants of the stack size detection
code that used to run at build time, and move the flock detection into
the XS code, so Storable.pm can be an entirely normal module.


Mentioning perl in attempt to bypass the spam filter.

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2019

From @haarg

0001-remove-remains-of-Storable-stack-size-detection.patch
From b4bf25393ddf36c0b1f6753b5e204285dd12ef51 Mon Sep 17 00:00:00 2001
From: Graham Knop <haarg@haarg.org>
Date: Sat, 20 Apr 2019 12:44:52 +0200
Subject: [PATCH 1/2] remove remains of Storable stack size detection

At build time, Storable used to detect the stack limits and store them.
This was removed in 2a0bbd31bbcc94c7c511f9575df619c5fdf3164c.  That
removed the Limit module and the setup for building it, but left behind
the script used to detect the limits.

Clean up the script and some related bits in the Makefile.PL.
---
 MANIFEST                  |   1 -
 dist/Storable/MANIFEST    |   1 -
 dist/Storable/Makefile.PL |   8 --
 dist/Storable/stacksize   | 193 --------------------------------------
 4 files changed, 203 deletions(-)
 delete mode 100644 dist/Storable/stacksize

diff --git a/MANIFEST b/MANIFEST
index 882d8f7c31..a68670aaf4 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -3639,7 +3639,6 @@ dist/Storable/hints/linux.pl		Hint for Storable for named architecture
 dist/Storable/Makefile.PL		Storable extension
 dist/Storable/MANIFEST			Storable MANIFEST file
 dist/Storable/README			Storable extension
-dist/Storable/stacksize			compute stack sizes
 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
diff --git a/dist/Storable/MANIFEST b/dist/Storable/MANIFEST
index d30b94e133..a606c23bc8 100644
--- a/dist/Storable/MANIFEST
+++ b/dist/Storable/MANIFEST
@@ -10,7 +10,6 @@ META.json			Module JSON meta-data (added by MakeMaker)
 META.yml			Module meta-data (added by MakeMaker)
 ppport.h
 README
-stacksize
 Storable.pm.PL
 Storable.xs
 t/attach.t
diff --git a/dist/Storable/Makefile.PL b/dist/Storable/Makefile.PL
index 4a39125562..c86f5aba1b 100644
--- a/dist/Storable/Makefile.PL
+++ b/dist/Storable/Makefile.PL
@@ -67,16 +67,8 @@
 EOM
 }
 
-# compute the maximum stacksize, before and after linking
 package MY;
 
-# FORCE finish of INST_DYNAMIC, avoid loading the old Storable (failed XS_VERSION check)
-sub xlinkext {
-    my $s = shift->SUPER::linkext(@_);
-    $s =~ s|( :: .*)| $1 FORCE stacksize|;
-    $s
-}
-
 sub depend {
     "
 
diff --git a/dist/Storable/stacksize b/dist/Storable/stacksize
deleted file mode 100644
index f93eccce1a..0000000000
--- a/dist/Storable/stacksize
+++ /dev/null
@@ -1,193 +0,0 @@
-#!/usr/bin/perl
-# binary search maximum stack depth for arrays and hashes
-# and report it to stdout as code to set the limits
-
-use Config;
-use Cwd;
-use File::Spec;
-use strict;
-
-my $ptrsize = $Config{ptrsize};
-my ($bad1, $bad2) = (65001, 25000);
-sub QUIET () {
-    (defined $ENV{MAKEFLAGS} and $ENV{MAKEFLAGS} =~ /\b(s|silent|quiet)\b/
-     and !defined($ENV{TRAVIS})) || @ARGV && $ARGV[0] eq "-q"
-      ? 1 : 0
-}
-sub PARALLEL () {
-    if (defined $ENV{MAKEFLAGS}
-        and $ENV{MAKEFLAGS} =~ /\bj\s*(\d+)\b/
-        and $1 > 1) {
-        return 1;
-    } else {
-        return 0;
-    }
-}
-sub is_miniperl {
-    return !defined &DynaLoader::boot_DynaLoader;
-}
-
-if (is_miniperl()) {
-    die "Should not run using miniperl\n";
-}
-my $prefix = "";
-if ($^O eq "MSWin32") {
-    # prevent Windows popping up a dialog each time we overflow
-    # the stack
-    require Win32API::File;
-    Win32API::File->import(qw(SetErrorMode SEM_NOGPFAULTERRORBOX SEM_FAILCRITICALERRORS));
-    SetErrorMode(SEM_NOGPFAULTERRORBOX() | SEM_FAILCRITICALERRORS());
-}
-# the ; here is to ensure system() passes this to the shell
-elsif (system("ulimit -c 0 ;") == 0) {
-    # try to prevent core dumps
-    $prefix = "ulimit -c 0 ; ";
-}
-my $PERL = $^X;
-if ($^O eq "MSWin32") {
-    require Win32;
-    my ($str, $major, $minor) = Win32::GetOSVersion();
-    if ($major < 6 || $major == 6 && $minor < 1) {
-	print "# Using defaults for older Win32\n";
-	write_limits(500, 256);
-	exit;
-    }
-}
-my ($n, $good, $bad, $found) =
-    (65000, 100, $bad1, undef);
-print "# probe for max. stack sizes...\n" unless QUIET;
-# -I. since we're run before pm_to_blib (which is going to copy the
-# file we create) and need to load our Storable.pm, not the already
-# installed Storable.pm
-my $mblib = '';
-if (-d 'blib') {
-    $mblib = '-Mblib -I.';
-}
-elsif (-f "Configure") {
-    $mblib = '-Ilib';
-}
-
-sub cmd {
-    my ($i, $try, $limit_name) = @_;
-    die unless $i;
-    my $code = "my \$t; \$Storable::$limit_name = -1; $try for 1..$i;dclone(\$t); print qq/ok\n/";
-    my $q = ($^O eq 'MSWin32') ? '"' : "'";
-
-    "$prefix $PERL $mblib -MStorable=dclone -e$q$code$q"
-}
-# try more
-sub good {
-    my $i = shift; # this passed
-    my $j = $i + abs(int(($bad - $i) / 2));
-    print "# Storable: determining recursion limit: $i passed, try more $j ...\n" unless QUIET;
-    $good = $i;
-    if ($j <= $i) {
-        $found++;
-    }
-    return $j;
-}
-# try less
-sub bad {
-    my $i = shift; # this failed
-    my $j = $i - abs(int(($i - $good) / 2));
-    print "# Storable: determining recursion limit: $i too big, try less $j ...\n" unless QUIET;
-    $bad = $i;
-    if ($j >= $i) {
-        $j = $good;
-        $found++;
-    }
-    return $j;
-}
-
-sub array_cmd {
-    my $depth = shift;
-    return cmd($depth, '$t=[$t]', 'recursion_limit');
-}
-
-# first check we can successfully run with a minimum level
-my $cmd = array_cmd(1);
-unless ((my $output = `$cmd`) =~ /\bok\b/) {
-    die "Cannot run probe: '$output', aborting...\n";
-}
-
-unless ($ENV{STORABLE_NOISY}) {
-    # suppress Segmentation fault messages
-    open STDERR, ">", File::Spec->devnull;
-}
-
-while (!$found) {
-    my $cmd = array_cmd($n);
-    #print "$cmd\n" unless $QUIET;
-    if (`$cmd` =~ /\bok\b/) {
-        $n = good($n);
-    } else {
-        $n = bad($n);
-    }
-}
-print "# MAX_DEPTH = $n\n" unless QUIET;
-my $max_depth = $n;
-
-($n, $good, $bad, $found) =
-  (int($n/2), 50, $n, undef);
-# pack j only since 5.8
-my $max = ($] > 5.007 and length(pack "j", 0) < 8)
-  ? ($^O eq 'MSWin32' ? 3000 : 8000)
-  : $max_depth;
-$n = $max if $n > $max;
-$bad = $max if $bad > $max;
-while (!$found) {
-    my $cmd = cmd($n, '$t={1=>$t}', 'recursion_limit_hash');
-    #print "$cmd\n" unless $QUIET;
-    if (`$cmd` =~ /\bok\b/) {
-        $n = good($n);
-    } else {
-        $n = bad($n);
-    }
-}
-if ($max_depth == $bad1-1
-    and $n == $bad2-1)
-{
-    # more likely the shell. travis docker ubuntu, mingw e.g.
-    print "# Apparently your system(SHELLSTRING) cannot catch stack overflows\n"
-      unless QUIET;
-    $max_depth = 512;
-    $n = 256;
-    print "MAX_DEPTH = $max_depth\n" unless QUIET;
-}
-print "# MAX_DEPTH_HASH = $n\n" unless QUIET;
-my $max_depth_hash = $n;
-
-# Previously this calculation was done in the macro, calculate it here
-# instead so a user setting of either variable more closely matches
-# the limits the use sees.
-
-# be fairly aggressive in trimming this, smoke testing showed several
-# several apparently random failures here, eg. working in one
-# configuration, but not in a very similar configuration.
-$max_depth = int(0.6 * $max_depth);
-$max_depth_hash = int(0.6 * $max_depth_hash);
-
-my $stack_reserve = $^O eq "MSWin32" ? 32 : 16;
-if ($] ge "5.016" && !($^O eq "cygwin" && $ptrsize == 8)) {
-    $max_depth -= $stack_reserve;
-    $max_depth_hash -= $stack_reserve;
-}
-else {
-    # within the exception we need another stack depth to recursively
-    # cleanup the hash
-    $max_depth = ($max_depth >> 1) - $stack_reserve;
-    $max_depth_hash = ($max_depth_hash >> 1) - $stack_reserve * 2;
-}
-
-write_limits($max_depth, $max_depth_hash);
-
-sub write_limits {
-    my ($max_depth, $max_depth_hash) = @_;
-    print <<EOS;
-# bisected by stacksize
-\$Storable::recursion_limit = $max_depth
-  unless defined \$Storable::recursion_limit;
-\$Storable::recursion_limit_hash = $max_depth_hash
-  unless defined \$Storable::recursion_limit_hash;
-EOS
-}
-- 
2.22.0

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2019

From @haarg

0002-move-Storable-CAN_FLOCK-computation-into-XS.patch
From 8590d5443fb7f508a5c14ca8d02fc0497decf43b Mon Sep 17 00:00:00 2001
From: Graham Knop <haarg@haarg.org>
Date: Sat, 20 Apr 2019 12:53:06 +0200
Subject: [PATCH 2/2] move Storable CAN_FLOCK computation into XS

At build time, Storable would check if it could use flock based on some
Config values, and generate the final Storable.pm file using tha value.
This was done to avoid needing to load Config at runtime.  This adds
complexity to the build process.

A simpler option is to build the constant in the XS code.  This means
the build process can be entirely standard for XS.

Some slight adjustments to the code are needed to load the XS module at
BEGIN time, so that the contant is available in the rest of the file.
---
 MANIFEST                                      |  3 +-
 Porting/corelist.pl                           |  1 -
 dist/Storable/.gitignore                      |  2 --
 dist/Storable/MANIFEST                        |  3 +-
 dist/Storable/Makefile.PL                     | 29 +++------------
 .../Storable/{__Storable__.pm => Storable.pm} | 14 ++++----
 dist/Storable/Storable.pm.PL                  | 35 -------------------
 dist/Storable/Storable.xs                     |  8 +++++
 8 files changed, 22 insertions(+), 73 deletions(-)
 delete mode 100644 dist/Storable/.gitignore
 rename dist/Storable/{__Storable__.pm => Storable.pm} (99%)
 delete mode 100644 dist/Storable/Storable.pm.PL

diff --git a/MANIFEST b/MANIFEST
index a68670aaf4..9859ce4d8a 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -3630,7 +3630,6 @@ 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
@@ -3639,8 +3638,8 @@ dist/Storable/hints/linux.pl		Hint for Storable for named architecture
 dist/Storable/Makefile.PL		Storable extension
 dist/Storable/MANIFEST			Storable MANIFEST file
 dist/Storable/README			Storable extension
+dist/Storable/Storable.pm		Storable perl module
 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/Porting/corelist.pl b/Porting/corelist.pl
index ad5a4ad06d..ce74ed42c8 100755
--- a/Porting/corelist.pl
+++ b/Porting/corelist.pl
@@ -98,7 +98,6 @@
     sub {
         /(\.pm|_pm\.PL)$/ or return;
         /PPPort\.pm$/ and return;
-        /__Storable__\.pm$/ and return;
         my $module = $File::Find::name;
         $module =~ /\b(demo|t|private|corpus)\b/ and return;    # demo or test modules
         my $version = MM->parse_version($_);
diff --git a/dist/Storable/.gitignore b/dist/Storable/.gitignore
deleted file mode 100644
index de731b9d98..0000000000
--- a/dist/Storable/.gitignore
+++ /dev/null
@@ -1,2 +0,0 @@
-/Storable.pm
-/lib
diff --git a/dist/Storable/MANIFEST b/dist/Storable/MANIFEST
index a606c23bc8..4706312a10 100644
--- a/dist/Storable/MANIFEST
+++ b/dist/Storable/MANIFEST
@@ -1,4 +1,3 @@
-__Storable__.pm
 ChangeLog
 hints/gnukfreebsd.pl
 hints/gnuknetbsd.pl
@@ -10,7 +9,7 @@ META.json			Module JSON meta-data (added by MakeMaker)
 META.yml			Module meta-data (added by MakeMaker)
 ppport.h
 README
-Storable.pm.PL
+Storable.pm
 Storable.xs
 t/attach.t
 t/attach_errors.t
diff --git a/dist/Storable/Makefile.PL b/dist/Storable/Makefile.PL
index c86f5aba1b..cdcc3e0087 100644
--- a/dist/Storable/Makefile.PL
+++ b/dist/Storable/Makefile.PL
@@ -10,10 +10,6 @@
 use warnings;
 use ExtUtils::MakeMaker 6.31;
 use Config;
-use File::Copy qw(move copy);
-use File::Spec;
-
-my $pm = { 'Storable.pm' => '$(INST_ARCHLIB)/Storable.pm' };
 
 WriteMakefile(
     NAME                => 'Storable',
@@ -22,31 +18,26 @@
     DISTNAME            => "Storable",
 # We now ship this in t/
 #    PREREQ_PM           => { 'Test::More' => '0.41' },
-    PL_FILES        => { }, # prevent default behaviour
-    PM              => $pm,
     PREREQ_PM           => { XSLoader => 0 },
     INSTALLDIRS => ($] >= 5.007 && $] < 5.012) ? 'perl' : 'site',
-    VERSION_FROM    => '__Storable__.pm',
-    ABSTRACT_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/' },
                             provides    => {
                                 'Storable'  => {
-                                    file        => '__Storable__.pm',
-                                    version     => MM->parse_version('__Storable__.pm'),
+                                    file        => 'Storable.pm',
+                                    version     => MM->parse_version('Storable.pm'),
                                 },
                             },
 
                            },
     ) : ()),
     dist                => { SUFFIX => 'gz', COMPRESS => 'gzip -f' },
-    clean               => { FILES => 'Storable-* Storable.pm lib' },
+    clean               => { FILES => 'Storable-*' },
 );
 
-# 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
@@ -79,13 +70,3 @@ sub depend {
 	git push --tags
 "
 }
-
-sub postamble {
-'
-all :: Storable.pm
-	$(NOECHO) $(NOOP)
-
-Storable.pm :: Storable.pm.PL __Storable__.pm
-	$(PERLRUN) Storable.pm.PL
-'
-}
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 8ed247f96f..5804f475a1 100644
--- a/dist/Storable/__Storable__.pm
+++ b/dist/Storable/Storable.pm
@@ -8,7 +8,7 @@
 #  in the README file that comes with the distribution.
 #
 
-require XSLoader;
+BEGIN { require XSLoader }
 require Exporter;
 package Storable;
 
@@ -27,7 +27,9 @@ package Storable;
 
 our ($canonical, $forgive_me);
 
-our $VERSION = '3.16';
+BEGIN {
+  our $VERSION = '3.16';
+}
 
 our $recursion_limit;
 our $recursion_limit_hash;
@@ -104,14 +106,12 @@ sub CLONE {
 $Storable::downgrade_restricted = 1;
 $Storable::accept_future_minor = 1;
 
-XSLoader::load('Storable');
+BEGIN { XSLoader::load('Storable') };
 
 #
 # Determine whether locking is possible, but only when needed.
 #
 
-sub CAN_FLOCK; # TEMPLATE - replaced by Storable.pm.PL
-
 sub show_file_magic {
     print <<EOM;
 #
@@ -266,7 +266,7 @@ sub _store {
     local *FILE;
     if ($use_locking) {
         open(FILE, ">>", $file) || logcroak "can't write into $file: $!";
-        unless (&CAN_FLOCK) {
+        unless (CAN_FLOCK) {
             logcarp
               "Storable::lock_store: fcntl/flock emulation broken on $^O";
             return undef;
@@ -410,7 +410,7 @@ sub _retrieve {
     my $self;
     my $da = $@;			# Could be from exception handler
     if ($use_locking) {
-        unless (&CAN_FLOCK) {
+        unless (CAN_FLOCK) {
             logcarp
               "Storable::lock_store: fcntl/flock emulation broken on $^O";
             return undef;
diff --git a/dist/Storable/Storable.pm.PL b/dist/Storable/Storable.pm.PL
deleted file mode 100644
index df979c09a9..0000000000
--- a/dist/Storable/Storable.pm.PL
+++ /dev/null
@@ -1,35 +0,0 @@
-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.xs b/dist/Storable/Storable.xs
index 6a45d8adf2..83ce676823 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -104,6 +104,12 @@
 #  define strEQc(s,c) memEQ(s, ("" c ""), sizeof(c))
 #endif
 
+#if defined(HAS_FLOCK) || defined(FCNTL_CAN_LOCK) && defined(HAS_LOCKF)
+#define CAN_FLOCK &PL_sv_yes
+#else
+#define CAN_FLOCK &PL_sv_no
+#endif
+
 #ifdef DEBUGME
 
 #ifndef DASSERT
@@ -7794,6 +7800,8 @@ BOOT:
     newCONSTSUB(stash, "BIN_MINOR", newSViv(STORABLE_BIN_MINOR));
     newCONSTSUB(stash, "BIN_WRITE_MINOR", newSViv(STORABLE_BIN_WRITE_MINOR));
 
+    newCONSTSUB(stash, "CAN_FLOCK", CAN_FLOCK);
+
     init_perinterp(aTHX);
     gv_fetchpv("Storable::drop_utf8",   GV_ADDMULTI, SVt_PV);
 #ifdef DEBUGME
-- 
2.22.0

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2019

From @jkeenan

On Fri, 05 Jul 2019 11​:55​:21 GMT, haarg wrote​:

The build process for Storable is currently needlessly complex. It
involves using a module as a template and has remnants of other build
time code that has since been removed.

These two patches clean up the remnants of the stack size detection
code that used to run at build time, and move the flock detection into
the XS code, so Storable.pm can be an entirely normal module.

Made available for smoke-testing in this branch​:

smoke-me/jkeenan/haarg/134265-storable

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

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2019

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

@p5pRT
Copy link
Author

p5pRT commented Jul 15, 2019

From @tonycoz

On Fri, 05 Jul 2019 04​:55​:21 -0700, haarg wrote​:

The build process for Storable is currently needlessly complex. It
involves using a module as a template and has remnants of other build
time code that has since been removed.

These two patches clean up the remnants of the stack size detection
code that used to run at build time, and move the flock detection into
the XS code, so Storable.pm can be an entirely normal module.

I left the stacksize tool in so an interested user could use it to calculate the limits for their environment. I should probably have documented it though.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jul 15, 2019

From @haarg

On Sun, 14 Jul 2019 23​:14​:10 -0700, tonyc wrote​:

On Fri, 05 Jul 2019 04​:55​:21 -0700, haarg wrote​:

The build process for Storable is currently needlessly complex. It
involves using a module as a template and has remnants of other build
time code that has since been removed.

These two patches clean up the remnants of the stack size detection
code that used to run at build time, and move the flock detection
into
the XS code, so Storable.pm can be an entirely normal module.

I left the stacksize tool in so an interested user could use it to
calculate the limits for their environment. I should probably have
documented it though.

Tony

The main thing I wanted to clean up with my patch was the build process, so I wanted to remove the remnants of it from the Makefile.PL. Leaving the script itself seems fine.

@p5pRT
Copy link
Author

p5pRT commented Aug 6, 2019

From @tonycoz

On Sun, 14 Jul 2019 23​:14​:10 -0700, tonyc wrote​:

On Fri, 05 Jul 2019 04​:55​:21 -0700, haarg wrote​:

The build process for Storable is currently needlessly complex. It
involves using a module as a template and has remnants of other build
time code that has since been removed.

These two patches clean up the remnants of the stack size detection
code that used to run at build time, and move the flock detection
into
the XS code, so Storable.pm can be an entirely normal module.

I left the stacksize tool in so an interested user could use it to
calculate the limits for their environment. I should probably have
documented it though.

Here's an modification of your patches that retains stacksize.

Tony

@p5pRT
Copy link
Author

p5pRT commented Aug 6, 2019

From @tonycoz

cleanup-storable-build.patch
From a31752650991404ebfec36f52a4e49ea8836f2ef Mon Sep 17 00:00:00 2001
From: Graham Knop <haarg@haarg.org>
Date: Sat, 20 Apr 2019 12:44:52 +0200
Subject: remove remains of Storable stack size detection

At build time, Storable used to detect the stack limits and store them.
This was removed in 2a0bbd31bbcc94c7c511f9575df619c5fdf3164c.  That
removed the Limit module and the setup for building it, but left behind
the script used to detect the limits.

Clean up the script and some related bits in the Makefile.PL.

TonyC: retain stacksize, so end users can use it to choose limits
---
 dist/Storable/Makefile.PL | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/dist/Storable/Makefile.PL b/dist/Storable/Makefile.PL
index 4a39125562..c86f5aba1b 100644
--- a/dist/Storable/Makefile.PL
+++ b/dist/Storable/Makefile.PL
@@ -67,16 +67,8 @@ in the Storable documentation for instructions on how to read your data.
 EOM
 }
 
-# compute the maximum stacksize, before and after linking
 package MY;
 
-# FORCE finish of INST_DYNAMIC, avoid loading the old Storable (failed XS_VERSION check)
-sub xlinkext {
-    my $s = shift->SUPER::linkext(@_);
-    $s =~ s|( :: .*)| $1 FORCE stacksize|;
-    $s
-}
-
 sub depend {
     "
 
-- 
2.11.0


From 44f059f82fb895dc115b810b8270cf1e4c0c7099 Mon Sep 17 00:00:00 2001
From: Graham Knop <haarg@haarg.org>
Date: Sat, 20 Apr 2019 12:53:06 +0200
Subject: move Storable CAN_FLOCK computation into XS

At build time, Storable would check if it could use flock based on some
Config values, and generate the final Storable.pm file using tha value.
This was done to avoid needing to load Config at runtime.  This adds
complexity to the build process.

A simpler option is to build the constant in the XS code.  This means
the build process can be entirely standard for XS.

Some slight adjustments to the code are needed to load the XS module at
BEGIN time, so that the contant is available in the rest of the file.
---
 MANIFEST                                       |  3 +--
 Porting/corelist.pl                            |  1 -
 dist/Storable/.gitignore                       |  2 --
 dist/Storable/MANIFEST                         |  3 +--
 dist/Storable/Makefile.PL                      | 29 ++++-----------------
 dist/Storable/{__Storable__.pm => Storable.pm} | 14 +++++------
 dist/Storable/Storable.pm.PL                   | 35 --------------------------
 dist/Storable/Storable.xs                      |  8 ++++++
 8 files changed, 22 insertions(+), 73 deletions(-)
 delete mode 100644 dist/Storable/.gitignore
 rename dist/Storable/{__Storable__.pm => Storable.pm} (99%)
 delete mode 100644 dist/Storable/Storable.pm.PL

diff --git a/MANIFEST b/MANIFEST
index 9b7798e379..e3785eedd6 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -3630,7 +3630,6 @@ 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
@@ -3640,8 +3639,8 @@ dist/Storable/Makefile.PL		Storable extension
 dist/Storable/MANIFEST			Storable MANIFEST file
 dist/Storable/README			Storable extension
 dist/Storable/stacksize			compute stack sizes
+dist/Storable/Storable.pm		Storable perl module
 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/Porting/corelist.pl b/Porting/corelist.pl
index ad5a4ad06d..ce74ed42c8 100755
--- a/Porting/corelist.pl
+++ b/Porting/corelist.pl
@@ -98,7 +98,6 @@ find(
     sub {
         /(\.pm|_pm\.PL)$/ or return;
         /PPPort\.pm$/ and return;
-        /__Storable__\.pm$/ and return;
         my $module = $File::Find::name;
         $module =~ /\b(demo|t|private|corpus)\b/ and return;    # demo or test modules
         my $version = MM->parse_version($_);
diff --git a/dist/Storable/.gitignore b/dist/Storable/.gitignore
deleted file mode 100644
index de731b9d98..0000000000
--- a/dist/Storable/.gitignore
+++ /dev/null
@@ -1,2 +0,0 @@
-/Storable.pm
-/lib
diff --git a/dist/Storable/MANIFEST b/dist/Storable/MANIFEST
index d30b94e133..5e382d9524 100644
--- a/dist/Storable/MANIFEST
+++ b/dist/Storable/MANIFEST
@@ -1,4 +1,3 @@
-__Storable__.pm
 ChangeLog
 hints/gnukfreebsd.pl
 hints/gnuknetbsd.pl
@@ -11,7 +10,7 @@ META.yml			Module meta-data (added by MakeMaker)
 ppport.h
 README
 stacksize
-Storable.pm.PL
+Storable.pm
 Storable.xs
 t/attach.t
 t/attach_errors.t
diff --git a/dist/Storable/Makefile.PL b/dist/Storable/Makefile.PL
index c86f5aba1b..cdcc3e0087 100644
--- a/dist/Storable/Makefile.PL
+++ b/dist/Storable/Makefile.PL
@@ -10,10 +10,6 @@ use strict;
 use warnings;
 use ExtUtils::MakeMaker 6.31;
 use Config;
-use File::Copy qw(move copy);
-use File::Spec;
-
-my $pm = { 'Storable.pm' => '$(INST_ARCHLIB)/Storable.pm' };
 
 WriteMakefile(
     NAME                => 'Storable',
@@ -22,31 +18,26 @@ WriteMakefile(
     DISTNAME            => "Storable",
 # We now ship this in t/
 #    PREREQ_PM           => { 'Test::More' => '0.41' },
-    PL_FILES        => { }, # prevent default behaviour
-    PM              => $pm,
     PREREQ_PM           => { XSLoader => 0 },
     INSTALLDIRS => ($] >= 5.007 && $] < 5.012) ? 'perl' : 'site',
-    VERSION_FROM    => '__Storable__.pm',
-    ABSTRACT_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/' },
                             provides    => {
                                 'Storable'  => {
-                                    file        => '__Storable__.pm',
-                                    version     => MM->parse_version('__Storable__.pm'),
+                                    file        => 'Storable.pm',
+                                    version     => MM->parse_version('Storable.pm'),
                                 },
                             },
 
                            },
     ) : ()),
     dist                => { SUFFIX => 'gz', COMPRESS => 'gzip -f' },
-    clean               => { FILES => 'Storable-* Storable.pm lib' },
+    clean               => { FILES => 'Storable-*' },
 );
 
-# 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
@@ -79,13 +70,3 @@ release : dist
 	git push --tags
 "
 }
-
-sub postamble {
-'
-all :: Storable.pm
-	$(NOECHO) $(NOOP)
-
-Storable.pm :: Storable.pm.PL __Storable__.pm
-	$(PERLRUN) Storable.pm.PL
-'
-}
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 8ed247f96f..5804f475a1 100644
--- a/dist/Storable/__Storable__.pm
+++ b/dist/Storable/Storable.pm
@@ -8,7 +8,7 @@
 #  in the README file that comes with the distribution.
 #
 
-require XSLoader;
+BEGIN { require XSLoader }
 require Exporter;
 package Storable;
 
@@ -27,7 +27,9 @@ our @EXPORT_OK = qw(
 
 our ($canonical, $forgive_me);
 
-our $VERSION = '3.16';
+BEGIN {
+  our $VERSION = '3.16';
+}
 
 our $recursion_limit;
 our $recursion_limit_hash;
@@ -104,14 +106,12 @@ $Storable::flags = FLAGS_COMPAT;
 $Storable::downgrade_restricted = 1;
 $Storable::accept_future_minor = 1;
 
-XSLoader::load('Storable');
+BEGIN { XSLoader::load('Storable') };
 
 #
 # Determine whether locking is possible, but only when needed.
 #
 
-sub CAN_FLOCK; # TEMPLATE - replaced by Storable.pm.PL
-
 sub show_file_magic {
     print <<EOM;
 #
@@ -266,7 +266,7 @@ sub _store {
     local *FILE;
     if ($use_locking) {
         open(FILE, ">>", $file) || logcroak "can't write into $file: $!";
-        unless (&CAN_FLOCK) {
+        unless (CAN_FLOCK) {
             logcarp
               "Storable::lock_store: fcntl/flock emulation broken on $^O";
             return undef;
@@ -410,7 +410,7 @@ sub _retrieve {
     my $self;
     my $da = $@;			# Could be from exception handler
     if ($use_locking) {
-        unless (&CAN_FLOCK) {
+        unless (CAN_FLOCK) {
             logcarp
               "Storable::lock_store: fcntl/flock emulation broken on $^O";
             return undef;
diff --git a/dist/Storable/Storable.pm.PL b/dist/Storable/Storable.pm.PL
deleted file mode 100644
index df979c09a9..0000000000
--- a/dist/Storable/Storable.pm.PL
+++ /dev/null
@@ -1,35 +0,0 @@
-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.xs b/dist/Storable/Storable.xs
index 6a45d8adf2..83ce676823 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -104,6 +104,12 @@
 #  define strEQc(s,c) memEQ(s, ("" c ""), sizeof(c))
 #endif
 
+#if defined(HAS_FLOCK) || defined(FCNTL_CAN_LOCK) && defined(HAS_LOCKF)
+#define CAN_FLOCK &PL_sv_yes
+#else
+#define CAN_FLOCK &PL_sv_no
+#endif
+
 #ifdef DEBUGME
 
 #ifndef DASSERT
@@ -7794,6 +7800,8 @@ BOOT:
     newCONSTSUB(stash, "BIN_MINOR", newSViv(STORABLE_BIN_MINOR));
     newCONSTSUB(stash, "BIN_WRITE_MINOR", newSViv(STORABLE_BIN_WRITE_MINOR));
 
+    newCONSTSUB(stash, "CAN_FLOCK", CAN_FLOCK);
+
     init_perinterp(aTHX);
     gv_fetchpv("Storable::drop_utf8",   GV_ADDMULTI, SVt_PV);
 #ifdef DEBUGME
-- 
2.11.0


From 28d2e17aeab6516679b0c86f235e1e0e22f220ea Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 6 Aug 2019 10:54:25 +1000
Subject: minimally document the stacksize tool

---
 dist/Storable/Storable.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm
index 5804f475a1..bcc315ba77 100644
--- a/dist/Storable/Storable.pm
+++ b/dist/Storable/Storable.pm
@@ -986,6 +986,9 @@ modifying C<$Storable::recursion_limit> and
 C<$Storable::recursion_limit_hash> respectively.  Either can be set to
 C<-1> to prevent any depth checks, though this isn't recommended.
 
+If you want to test what the limits are, the F<stacksize> tool is
+included in the C<Storable> distribution.
+
 =item *
 
 You can create endless loops if the things you serialize via freeze()
-- 
2.11.0

@p5pRT
Copy link
Author

p5pRT commented Aug 8, 2019

From @tonycoz

On Mon, 05 Aug 2019 18​:06​:53 -0700, tonyc wrote​:

On Sun, 14 Jul 2019 23​:14​:10 -0700, tonyc wrote​:

On Fri, 05 Jul 2019 04​:55​:21 -0700, haarg wrote​:

The build process for Storable is currently needlessly complex. It
involves using a module as a template and has remnants of other build
time code that has since been removed.

These two patches clean up the remnants of the stack size detection
code that used to run at build time, and move the flock detection
into
the XS code, so Storable.pm can be an entirely normal module.

I left the stacksize tool in so an interested user could use it to
calculate the limits for their environment. I should probably have
documented it though.

Here's an modification of your patches that retains stacksize.

Applied as 2473bad, 95173f9 and d8d4e2e.

Tony

@p5pRT
Copy link
Author

p5pRT commented Aug 8, 2019

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

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