[PATCH] ExtUtils::ParseXS: use literals for VERSION in 'use Module VERSION' #16451
Comments
From @skajiCreated by @skajiCurrently in ExtUtils::ParseXS, the version check does not work ``` We must use literals for VERSION. Perl Info |
From @skaji0001-ExtUtils-ParseXS-use-literals-for-VERSION-in-use-Mod.patchFrom 8be55465b93598ec391b65984f862e86d6ff85bd Mon Sep 17 00:00:00 2001
From: Shoichi Kaji <skaji@cpan.org>
Date: Sun, 4 Mar 2018 15:41:51 +0900
Subject: [PATCH] ExtUtils::ParseXS: use literals for VERSION in 'use Module
VERSION'
---
dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS.pm | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS.pm b/dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS.pm
index 16359ccbce..444a55de3d 100644
--- a/dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS.pm
+++ b/dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS.pm
@@ -9,14 +9,11 @@ use File::Basename;
use File::Spec;
use Symbol;
-our $VERSION;
-BEGIN {
- $VERSION = '3.38';
-}
-use ExtUtils::ParseXS::Constants $VERSION;
-use ExtUtils::ParseXS::CountLines $VERSION;
-use ExtUtils::ParseXS::Utilities $VERSION;
-use ExtUtils::ParseXS::Eval $VERSION;
+our $VERSION = '3.38';
+use ExtUtils::ParseXS::Constants 3.38;
+use ExtUtils::ParseXS::CountLines 3.38;
+use ExtUtils::ParseXS::Utilities 3.38;
+use ExtUtils::ParseXS::Eval 3.38;
$VERSION = eval $VERSION if $VERSION =~ /_/;
use ExtUtils::ParseXS::Utilities qw(
--
2.16.2
|
From @jkeenanOn Sun, 04 Mar 2018 07:10:18 GMT, skaji@outlook.com wrote:
Please see the explanation provided by Steffen Mueller in commit 71a65ad back in 2011: ##### Since there have been certain problems with parts of ExtUtils::ParseXS being shadowed by older installations of the module, this commit adds an explicit $VERSION to all submodules and requires them to have the same version as the main module. This doesn't actually fix any problem, but makes them more apparent as early as possible instead of failing with obscure compile errors when bad C is generated from the original XS. Thank you very much. -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @cpansproutOn Sun, 04 Mar 2018 18:24:12 -0800, jkeenan wrote:
That explains why version checks are a good idea. It does not justify the fact that that commit failed to add any version checks. This: use ExtUtils::ParseXS::Constants $VERSION; is equivalent to: BEGIN { And a call to a nonexistent import method is silently ignored. I don’t like the patch, because of the maintenance burden it creates (multiple numbers to update). I wouldn’t mind a patch that calls ->VERSION($VERSION) explicitly in a BEGIN block. -- Father Chrysostomos |
From @skajiHi James, Thanks for your reply. Hi Father, Tanks for your comment.
I understand. I added another patch which call require() and ->VERSION() in BEGIN block. On Sun, 04 Mar 2018 18:56:17 -0800, sprout wrote:
|
From @skaji0001-RT-132935-correctly-check-VERSIONs-in-ExtUtils-Parse.patchFrom 62ebd127e4da7c5d982e3da3befe5931915d83ad Mon Sep 17 00:00:00 2001
From: Shoichi Kaji <skaji@cpan.org>
Date: Mon, 5 Mar 2018 23:25:49 +0900
Subject: [PATCH] RT #132935: correctly check VERSIONs in ExtUtils::ParseXS
The following version check does not work correctly:
BEGIN { $VERSION = '3.38' }
use ExtUtils::ParseXS::Constants $VERSION;
The reason is that we must use version "literals",
not "variables" in `use Module VERSION`.
For the sake of ease of maintenance,
we use "require" and "->VERSION", instead of "use" here.
---
dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS.pm | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS.pm b/dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS.pm
index 16359ccbce..a36cf14c06 100644
--- a/dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS.pm
+++ b/dist/ExtUtils-ParseXS/lib/ExtUtils/ParseXS.pm
@@ -12,11 +12,11 @@ use Symbol;
our $VERSION;
BEGIN {
$VERSION = '3.38';
+ require ExtUtils::ParseXS::Constants; ExtUtils::ParseXS::Constants->VERSION($VERSION);
+ require ExtUtils::ParseXS::CountLines; ExtUtils::ParseXS::CountLines->VERSION($VERSION);
+ require ExtUtils::ParseXS::Utilities; ExtUtils::ParseXS::Utilities->VERSION($VERSION);
+ require ExtUtils::ParseXS::Eval; ExtUtils::ParseXS::Eval->VERSION($VERSION);
}
-use ExtUtils::ParseXS::Constants $VERSION;
-use ExtUtils::ParseXS::CountLines $VERSION;
-use ExtUtils::ParseXS::Utilities $VERSION;
-use ExtUtils::ParseXS::Eval $VERSION;
$VERSION = eval $VERSION if $VERSION =~ /_/;
use ExtUtils::ParseXS::Utilities qw(
--
2.16.2
|
From @skajiTypo: (False) I think I understand ExtUtils::ParseXS explicitly require current version of submodules. On Mon, 05 Mar 2018 06:50:06 -0800, skaji@cpan.org wrote:
|
From @skajiHi James, Thanks for your reply. My point is here that For example, the following code UNEXPECTEDLY prints "NEVER REACH HERE!". ``` my $version; warn "NEVER REACH HERE!\n"; ________________________________________ On Sun, 04 Mar 2018 07:10:18 GMT, skaji@outlook.com wrote:
Please see the explanation provided by Steffen Mueller in commit 71a65ad back in 2011: ##### Since there have been certain problems with parts of ExtUtils::ParseXS being shadowed by older installations of the module, this commit adds an explicit $VERSION to all submodules and requires them to have the same version as the main module. This doesn't actually fix any problem, but makes them more apparent as early as possible instead of failing with obscure compile errors when bad C is generated from the original XS. Thank you very much. -- |
From @cpansproutOn Mon, 05 Mar 2018 06:50:06 -0800, skaji@cpan.org wrote:
Thank you. Applied as 8a89137. -- Father Chrysostomos |
|
@cpansprout - Status changed from 'open' to 'pending release' |
From @skajiThanks! On Mon, 05 Mar 2018 08:48:30 -0800, sprout wrote:
|
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#132935 (status was 'resolved')
Searchable as RT132935$
The text was updated successfully, but these errors were encountered: