Switch some core modules to XSLoader #16147
Comments
From @atoomicThis is a bug report for perl from atoomic@cpan.org, The traditional boiler plate to use XSLoader or DynaLoader depending on the This is ok for CPAN modules which can be installed on any perl version, but IMO with perl 5.28+ we have no reason to keep that legacy pattern. Also note that the extra eval was removed in PathTools as part of this The commit is attached to this message and can also be read online at: Flags: Site configuration information for perl 5.26.0: Configured by cPanel at Thu Aug 31 17:31:39 CDT 2017. Summary of my perl5 (revision 5 version 26 subversion 0) configuration: Platform: Locally applied patches: @INC for perl 5.26.0: /usr/local/cpanel/3rdparty/perl/526/lib64/perl5/cpanel_lib/x86_64-linux-64int /usr/local/cpanel/3rdparty/perl/526/lib64/perl5/5.26.0/x86_64-linux-64int Environment for perl 5.26.0: PATH=/usr/local/cpanel/3rdparty/perl/526/bin:/usr/local/cpanel/3rdparty/perl/524/bin:/usr/local/cpanel/3rdparty/perl/522/bin:/usr/local/cpanel/3rdparty/perl/514/bin:/usr/local/cpanel/3rdparty/bin:/root/bin/:/opt/local/bin:/opt/local/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/opt/cpanel/composer/bin:/root/.dotfiles/bin:/root/perl5/bin:/root/.rvm/bin:/root/bin |
From @atoomic0001-Switch-some-core-modules-to-XSLoader.patchFrom b917b3622159892d1fc540f100c7536700512b70 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Wed, 13 Sep 2017 11:22:32 -0600
Subject: [PATCH] Switch some core modules to XSLoader
Remove perl 5.006 compatibilities with DynaLoader
and use XSLoader directly.
The traditional boiler plate to use XSLoader
for perl > 5.006 or DynaLoader, do not make sense
for core modules in perl 5.28+.
This is wanted for CPAN modules which can be install
on any perl version.
---
dist/PathTools/Cwd.pm | 18 ++++--------------
dist/PathTools/lib/File/Spec/Unix.pm | 14 +++-----------
dist/Time-HiRes/HiRes.pm | 8 ++++----
dist/Unicode-Normalize/Normalize.pm | 8 ++++----
4 files changed, 15 insertions(+), 33 deletions(-)
diff --git a/dist/PathTools/Cwd.pm b/dist/PathTools/Cwd.pm
index cc77e58e2b..90d9e6e47c 100644
--- a/dist/PathTools/Cwd.pm
+++ b/dist/PathTools/Cwd.pm
@@ -3,7 +3,7 @@ use strict;
use Exporter;
use vars qw(@ISA @EXPORT @EXPORT_OK $VERSION);
-$VERSION = '3.68';
+$VERSION = '3.69';
my $xs_version = $VERSION;
$VERSION =~ tr/_//d;
@@ -77,19 +77,9 @@ sub _vms_efs {
# If loading the XS stuff doesn't work, we can fall back to pure perl
-if(! defined &getcwd && defined &DynaLoader::boot_DynaLoader) {
- eval {#eval is questionable since we are handling potential errors like
- #"Cwd object version 3.48 does not match bootstrap parameter 3.50
- #at lib/DynaLoader.pm line 216." by having this eval
- if ( $] >= 5.006 ) {
- require XSLoader;
- XSLoader::load( __PACKAGE__, $xs_version);
- } else {
- require DynaLoader;
- push @ISA, 'DynaLoader';
- __PACKAGE__->bootstrap( $xs_version );
- }
- };
+if(! defined &getcwd && defined &DynaLoader::boot_DynaLoader) { # skipped on miniperl
+ require XSLoader;
+ XSLoader::load( __PACKAGE__, $xs_version);
}
# Big nasty table of function aliases
diff --git a/dist/PathTools/lib/File/Spec/Unix.pm b/dist/PathTools/lib/File/Spec/Unix.pm
index e1a30f8ca1..6249843fd0 100644
--- a/dist/PathTools/lib/File/Spec/Unix.pm
+++ b/dist/PathTools/lib/File/Spec/Unix.pm
@@ -3,22 +3,14 @@ package File::Spec::Unix;
use strict;
use vars qw($VERSION);
-$VERSION = '3.68';
+$VERSION = '3.69';
my $xs_version = $VERSION;
$VERSION =~ tr/_//d;
-#dont try to load XSLoader and DynaLoader only to ultimately fail on miniperl
-if(!defined &canonpath && defined &DynaLoader::boot_DynaLoader) {
- eval {#eval is questionable since we are handling potential errors like
- #"Cwd object version 3.48 does not match bootstrap parameter 3.50
- #at lib/DynaLoader.pm line 216." by having this eval
- if ( $] >= 5.006 ) {
+#dont try to load XSLoader only to ultimately fail on miniperl
+if(!defined &canonpath && defined &DynaLoader::boot_DynaLoader) { # skipped on miniperl
require XSLoader;
XSLoader::load("Cwd", $xs_version);
- } else {
- require Cwd;
- }
- };
}
=head1 NAME
diff --git a/dist/Time-HiRes/HiRes.pm b/dist/Time-HiRes/HiRes.pm
index fbdc2b4aca..a57c77147d 100644
--- a/dist/Time-HiRes/HiRes.pm
+++ b/dist/Time-HiRes/HiRes.pm
@@ -4,9 +4,9 @@ package Time::HiRes;
use strict;
require Exporter;
-require DynaLoader;
+use XSLoader ();
-our @ISA = qw(Exporter DynaLoader);
+our @ISA = qw(Exporter);
our @EXPORT = qw( );
our @EXPORT_OK = qw (usleep sleep ualarm alarm gettimeofday time tv_interval
@@ -28,7 +28,7 @@ our @EXPORT_OK = qw (usleep sleep ualarm alarm gettimeofday time tv_interval
stat lstat utime
);
-our $VERSION = '1.9743';
+our $VERSION = '1.9744';
our $XS_VERSION = $VERSION;
$VERSION = eval $VERSION;
@@ -69,7 +69,7 @@ sub import {
Time::HiRes->export_to_level(1, $this, @_);
}
-bootstrap Time::HiRes;
+XSLoader::load( 'Time::HiRes', $XS_VERSION );
# Preloaded methods go here.
diff --git a/dist/Unicode-Normalize/Normalize.pm b/dist/Unicode-Normalize/Normalize.pm
index b53f5c728b..adf3db50d8 100644
--- a/dist/Unicode-Normalize/Normalize.pm
+++ b/dist/Unicode-Normalize/Normalize.pm
@@ -16,7 +16,7 @@ use Carp;
no warnings 'utf8';
-our $VERSION = '1.25';
+our $VERSION = '1.26';
our $PACKAGE = __PACKAGE__;
our @EXPORT = qw( NFC NFD NFKC NFKD );
@@ -56,9 +56,9 @@ require Exporter;
##### The above part is common to XS and PP #####
-our @ISA = qw(Exporter DynaLoader);
-require DynaLoader;
-bootstrap Unicode::Normalize $VERSION;
+our @ISA = qw(Exporter);
+use XSLoader ();
+XSLoader::load( 'Unicode::Normalize', $VERSION );
##### The below part is common to XS and PP #####
--
2.14.1
|
From zefram@fysh.orgNicolas R. wrote:
All the modules you've removed this from are dual-life, and so in However, there's an independent argument that the value in portability to
PathTools is dual-life, and the eval supports installing from CPAN in -zefram |
|
The RT System itself - Status changed from 'new' to 'open' |
From @atoomicOn Wed, 13 Sep 2017 16:58:02 -0700, zefram@fysh.org wrote:
Thanks for the correction indeed, all the modules updated as part of this patch are dual-life.
Concerning the CPAN modules, I've already patched most of them in our local cpan mirror, to switch them to XSLoader. But as you mentioned this involve dropping support for versions earlier than 5.6 and I think opinions might differ.
I can restore the extra 'eval' protection from PathTools which I thought was mainly there to protect against miniperl, and was a duplicate of the logic in the if statement.
|
From @toddrOn Wed, 13 Sep 2017 16:58:02 -0700, zefram@fysh.org wrote:
Yes I think that's the proposal here. For the handful of people using 5.6, isn't that what barnyard is for? http://cp5.6.2an.barnyard.co.uk/
I'd challenge this too. Is there really a big demand for compilerless use of PathTools? I'm not aware of many other modules that have this behavior or if they do, they solve it with Cwd::PP right? I'm guessing this was put in long ago and even when it was there was no evidence it had value. Todd |
From @LeontOn Thu, Sep 14, 2017 at 1:42 AM, Nicolas R. <perlbug-followup@perl.org>
ExtUtils::MakeMaker dropped 5.5 support almost a decade ago, and so did 5.6 is on intensive care, but 5.5 is dead and buried. Leon |
From zefram@fysh.orgTodd Rinaldo via RT wrote:
They'll still be OK because 5.6 supports "our". The support being dropped
Compilerless installations are regrettably common. Reducing the support
Some of mine have such dual implementation in a single distro, such as
There are a few arranged like that (or the other way, with a ::XS), Putting the two implementations in the same distro, behind the same -zefram |
From @toddrOn Thu, 14 Sep 2017 15:10:00 -0700, zefram@fysh.org wrote:
I'm going to bring this up for discussion since I've never seen it discussed before. It is not meant as a spark for flameage. We seem to take the stance on dist modules that we have to support an unspecified lesser version of Perl. I think the lack of policy makes for some confusion when we discuss patches like this. I suggest a rather draconian policy: "The modules maintained in the dist tree of the perl source code should support blead and *MAYBE* the 2 maint versions but not have to consider support for older versions of Perl." IMO and within reason, I suggest that it should be the dual life maintainer's job to get the dist module working on CPAN. This makes it so that core does not have to make its code do back flips in blead perl to work with perl 5.6. I think this could potentially simplify the code and potentially make programs smaller/faster. Keep in mind that we are not spectacular at keeping dist up with what is on CPAN. IMO, that's perfectly ok. If you want the new hotness, upgrade! If module X has a versioning issue working on the blead version vs the CPAN version, then do a release or ask for a release. This may be a weak example, but version.pm seems to do just this the last time I looked at it. Its CPAN version looks nothing like the code in CORE since some of the work its CPAN counterpart does is built in to modern versions of Perl. Todd |
From zefram@fysh.orgTodd Rinaldo via RT wrote:
A basic aspect of how we manage dual-life modules is that the code in Backward compatibility of module code is a CPAN authoring issue, and for What actually happens is that, because we don't smoke dist/ modules Generally, backcompat code in a module isn't expensive on new perl -zefram |
From @xsawyerxOn 09/19/2017 05:52 PM, Todd Rinaldo via RT wrote:
While I understand the desire for such a policy, I'm not sure it is in Personally, I fail to understand why we should support 5.6, but I would In any case where such backcompat is in the way of a patch and that |
From @LeontOn Tue, Sep 19, 2017 at 5:52 PM, Todd Rinaldo via RT <
That's rather missing an essential point though: why would they drop This may be a weak example, but version.pm seems to do just this the last
version.pm is special it's functionality is used in implementing core Leon |
From zefram@fysh.orgSawyer X wrote:
Because some people still use it, and the cost of supporting it is Relative to 5.6, the main gain of a 5.8 minimum is proper handling of -zefram |
From @xsawyerxOn 09/23/2017 06:41 PM, Zefram wrote:
True, this was a general statement, but I hoped to provide enough
"Impractical" in this context could also mean "too high a cost."
It appears toolchain is putting a lot of effort in tracking the lowest
Agreed. I wouldn't mind having a feature that promotes everyone to upgrade just |
From @toddrOn Sun, 24 Sep 2017 13:16:57 -0700, xsawyerx@gmail.com wrote:
Perhaps it was a mistake to bring up this policy with regards to these patches. The patches listed are 5.6 compatible which seems to be our minimum toolchain version support is. Is there another reason not to merge this then? The value here is that as long as Perl ships with modules that suggest Dynaloader is still needed, it's hard to set a good example elsewhere on CPAN. Todd |
From @toddrOn Tue, 03 Oct 2017 13:50:15 -0700, TODDR wrote:
Barring any further concerns, we're going to merge this on on Monday. Thanks, |
|
@toddr - 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' |
From @atoomicThank you! Le sam. 23 juin 2018 à 09:32, Karl Williamson via RT <
|
Migrated from rt.perl.org#132080 (status was 'resolved')
Searchable as RT132080$
The text was updated successfully, but these errors were encountered: