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
ExtUtils-CBuilder non-deterministic define option order #14599
Comments
From zefram@fysh.org(Redirected from [rt.cpan.org #102830].) ExtUtils::CBuilder puts cpp-definition options into the cc command line -zefram |
From zefram@fysh.orgdeterministic_define_order.patch--- a/lib/ExtUtils/CBuilder/Base.pm 2014-09-27 16:50:33.000000000 +0000
+++ b/lib/ExtUtils/CBuilder/Base.pm 2015-03-17 17:23:13.031211647 +0000
@@ -128,7 +128,7 @@
sub arg_defines {
my ($self, %args) = @_;
- return map "-D$_=$args{$_}", keys %args;
+ return map "-D$_=$args{$_}", sort keys %args;
}
sub compile {
--- a/lib/ExtUtils/CBuilder/Platform/VMS.pm 2014-09-27 16:50:33.000000000 +0000
+++ b/lib/ExtUtils/CBuilder/Platform/VMS.pm 2015-03-17 17:24:54.708388683 +0000
@@ -31,7 +31,7 @@
. join(',',
@config_defines,
map "\"$_" . ( length($args{$_}) ? "=$args{$_}" : '') . "\"",
- keys %args)
+ sort keys %args)
. ')');
}
--- a/lib/ExtUtils/CBuilder/Platform/Windows.pm 2014-09-27 16:50:33.000000000 +0000
+++ b/lib/ExtUtils/CBuilder/Platform/Windows.pm 2015-03-17 17:25:34.365727471 +0000
@@ -76,7 +76,7 @@
sub arg_defines {
my ($self, %args) = @_;
s/"/\\"/g foreach values %args;
- return map qq{"-D$_=$args{$_}"}, keys %args;
+ return map qq{"-D$_=$args{$_}"}, sort keys %args;
}
sub compile {
|
From @jkeenanOn Wed Mar 18 03:44:13 2015, zefram@fysh.org wrote:
The patch's objective is sound, but it was drawn against lib/ rather than dist/ExtUtils-CBuilder/. I reworked it and added some tests. Now smoke-testing in branch smoke-me/jkeenan/124106-cbuilder-args. If no one objects, I will apply this patch for perl-5.23.1. Thank you very much. -- |
From @jkeenan124106-0001-Impose-deterministic-order-on-cpp-definition-options.patchFrom e0d340f6c7d2dcb960f5fa6cb9ccdb57538886ea Mon Sep 17 00:00:00 2001
From: Zefram <zefram@fysh.org>
Date: Wed, 18 Mar 2015 19:49:28 -0400
Subject: [PATCH] Impose deterministic order on cpp-definition options.
Heretofore, ExtUtils::CBuilder put cpp-definition options into the cc command line in
non-deterministic order. This produced noise when diffing build logs. Make
this order deterministic.
Add tests for ascii-betical order in t/04-base.t. Increment $VERSION in three
packages.
For: RT #124106
---
.../lib/ExtUtils/CBuilder/Base.pm | 38 +++++++++++-----------
.../lib/ExtUtils/CBuilder/Platform/VMS.pm | 34 +++++++++----------
.../lib/ExtUtils/CBuilder/Platform/Windows.pm | 8 ++---
dist/ExtUtils-CBuilder/t/04-base.t | 24 +++++++++++---
4 files changed, 59 insertions(+), 45 deletions(-)
diff --git a/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Base.pm b/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Base.pm
index 7df61e4..a077cfe 100644
--- a/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Base.pm
+++ b/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Base.pm
@@ -1,5 +1,5 @@
package ExtUtils::CBuilder::Base;
-$ExtUtils::CBuilder::Base::VERSION = '0.280221';
+$ExtUtils::CBuilder::Base::VERSION = '0.280222';
use strict;
use File::Spec;
use File::Basename;
@@ -128,20 +128,20 @@ sub arg_exec_file {
sub arg_defines {
my ($self, %args) = @_;
- return map "-D$_=$args{$_}", keys %args;
+ return map "-D$_=$args{$_}", sort keys %args;
}
sub compile {
my ($self, %args) = @_;
die "Missing 'source' argument to compile()" unless defined $args{source};
-
+
my $cf = $self->{config}; # For convenience
-
+
my $object_file = $args{object_file}
? $args{object_file}
: $self->object_file($args{source});
- my $include_dirs_ref =
+ my $include_dirs_ref =
(exists($args{include_dirs}) && ref($args{include_dirs}) ne "ARRAY")
? [ $args{include_dirs} ]
: $args{include_dirs};
@@ -149,9 +149,9 @@ sub compile {
@{ $include_dirs_ref || [] },
$self->perl_inc(),
);
-
+
my @defines = $self->arg_defines( %{$args{defines} || {}} );
-
+
my @extra_compiler_flags =
$self->split_like_shell($args{extra_compiler_flags});
my @cccdlflags = $self->split_like_shell($cf->{cccdlflags});
@@ -168,7 +168,7 @@ sub compile {
$self->arg_object_file($object_file),
);
my @cc = $self->split_like_shell($args{'C++'} ? $cf->{cxx} : $cf->{cc});
-
+
$self->do_system(@cc, @flags, $args{source})
or die "error building $object_file from '$args{source}'";
@@ -222,7 +222,7 @@ sub lib_file {
my ($self, $dl_file, %args) = @_;
$dl_file =~ s/\.[^.]+$//;
$dl_file =~ tr/"//d;
-
+
if (defined $args{module_name} and length $args{module_name}) {
# Need to create with the same name as DynaLoader will load with.
require DynaLoader;
@@ -232,7 +232,7 @@ sub lib_file {
$dl_file = File::Spec->catpath($dev, $lib_dir, $lib);
}
}
-
+
$dl_file .= ".$self->{config}{dlext}";
return $dl_file;
@@ -266,7 +266,7 @@ sub prelink {
sub _prepare_mksymlists_args {
my $args = shift;
($args->{dl_file} = $args->{dl_name}) =~ s/.*::// unless $args->{dl_file};
-
+
my %mksymlists_args = (
DL_VARS => $args->{dl_vars} || [],
DL_FUNCS => $args->{dl_funcs} || {},
@@ -294,16 +294,16 @@ sub _do_link {
my ($self, $type, %args) = @_;
my $cf = $self->{config}; # For convenience
-
+
my $objects = delete $args{objects};
$objects = [$objects] unless ref $objects;
my $out = $args{$type} || $self->$type($objects->[0], %args);
-
+
my @temp_files;
@temp_files =
$self->prelink(%args, dl_name => $args{module_name})
if $args{lddl} && $self->need_prelink;
-
+
my @linker_flags = (
$self->split_like_shell($args{extra_linker_flags}),
$self->extra_link_args_after_prelink(
@@ -316,10 +316,10 @@ sub _do_link {
: $self->arg_exec_file($out);
my @shrp = $self->split_like_shell($cf->{shrpenv});
my @ld = $self->split_like_shell($cf->{ld});
-
+
$self->do_system(@shrp, @ld, @output, @$objects, @linker_flags)
or die "error building $out from @$objects";
-
+
return wantarray ? ($out, @temp_files) : $out;
}
@@ -332,17 +332,17 @@ sub do_system {
sub split_like_shell {
my ($self, $string) = @_;
-
+
return () unless defined($string);
return @$string if UNIVERSAL::isa($string, 'ARRAY');
$string =~ s/^\s+|\s+$//g;
return () unless length($string);
-
+
# Text::ParseWords replaces all 'escaped' characters with themselves, which completely
# breaks paths under windows. As such, we forcibly replace backwards slashes with forward
# slashes on windows.
$string =~ s@\\@/@g if $^O eq 'MSWin32';
-
+
return Text::ParseWords::shellwords($string);
}
diff --git a/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Platform/VMS.pm b/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Platform/VMS.pm
index 6285e33..940dc50 100644
--- a/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Platform/VMS.pm
+++ b/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Platform/VMS.pm
@@ -1,5 +1,5 @@
package ExtUtils::CBuilder::Platform::VMS;
-$ExtUtils::CBuilder::Platform::VMS::VERSION = '0.280221';
+$ExtUtils::CBuilder::Platform::VMS::VERSION = '0.280222';
use strict;
use ExtUtils::CBuilder::Base;
@@ -27,11 +27,11 @@ sub arg_defines {
return '' unless keys(%args) || @config_defines;
- return ('/define=('
- . join(',',
+ return ('/define=('
+ . join(',',
@config_defines,
- map "\"$_" . ( length($args{$_}) ? "=$args{$_}" : '') . "\"",
- keys %args)
+ map "\"$_" . ( length($args{$_}) ? "=$args{$_}" : '') . "\"",
+ sort keys %args)
. ')');
}
@@ -50,7 +50,7 @@ sub arg_include_dirs {
# We override the compile method because we consume the includes and defines
# parts of ccflags in the process of compiling but don't save those parts
# anywhere, so $self->{config}{ccflags} needs to be reset for each compile
-# operation.
+# operation.
sub compile {
my ($self, %args) = @_;
@@ -63,10 +63,10 @@ sub compile {
sub _do_link {
my ($self, $type, %args) = @_;
-
+
my $objects = delete $args{objects};
$objects = [$objects] unless ref $objects;
-
+
if ($args{lddl}) {
# prelink will call Mksymlists, which creates the extension-specific
@@ -77,7 +77,7 @@ sub _do_link {
# We now add the rest of what we need to the linker options file. We
# should replicate the functionality of C<ExtUtils::MM_VMS::dlsyms>,
# but there is as yet no infrastructure for handling object libraries,
- # so for now we depend on object files being listed individually on the
+ # so for now we depend on object files being listed individually on the
# command line, which should work for simple cases. We do bring in our
# own version of C<ExtUtils::Liblist::Kid::ext> so that any additional
# libraries (including PERLSHR) can be added to the options file.
@@ -85,7 +85,7 @@ sub _do_link {
my @optlibs = $self->_liblist_ext( $args{'libs'} );
my $optfile = 'sys$disk:[]' . $temp_files[0];
- open my $opt_fh, '>>', $optfile
+ open my $opt_fh, '>>', $optfile
or die "_do_link: Unable to open $optfile: $!";
for my $lib (@optlibs) {print $opt_fh "$lib\n" if length $lib }
close $opt_fh;
@@ -136,7 +136,7 @@ sub _liblist_ext {
# which a system-wide logical may point.
if ($self->perl_src) {
my($lib,$locspec,$type);
- foreach $lib (@crtls) {
+ foreach $lib (@crtls) {
if (($locspec,$type) = $lib =~ m{^([\w\$-]+)(/\w+)?} and $locspec =~ /perl/i) {
if (lc $type eq '/share') { $locspec .= $self->{'config'}{'exe_ext'}; }
elsif (lc $type eq '/library') { $locspec .= $self->{'config'}{'lib_ext'}; }
@@ -188,8 +188,8 @@ sub _liblist_ext {
next;
}
warn "Resolving directory $dir\n" if $verbose;
- if (!File::Spec->file_name_is_absolute($dir)) {
- $dir = catdir($cwd,$dir);
+ if (!File::Spec->file_name_is_absolute($dir)) {
+ $dir = catdir($cwd,$dir);
}
}
@dirs = grep { length($_) } @dirs;
@@ -243,14 +243,14 @@ sub _liblist_ext {
$type = 'SHR';
$name = $fullname unless $fullname =~ /exe;?\d*$/i;
}
- elsif (not length($ctype) and # If we've got a lib already,
+ elsif (not length($ctype) and # If we've got a lib already,
# don't bother
( -f ($fullname = VMS::Filespec::rmsexpand($name,$lib_ext)) or
-f ($fullname = VMS::Filespec::rmsexpand($name,'.olb')))) {
$type = 'OLB';
$name = $fullname unless $fullname =~ /olb;?\d*$/i;
}
- elsif (not length($ctype) and # If we've got a lib already,
+ elsif (not length($ctype) and # If we've got a lib already,
# don't bother
( -f ($fullname = VMS::Filespec::rmsexpand($name,$obj_ext)) or
-f ($fullname = VMS::Filespec::rmsexpand($name,'.obj')))) {
@@ -264,9 +264,9 @@ sub _liblist_ext {
last if $ctype eq 'SHR';
}
}
- if ($ctype) {
+ if ($ctype) {
push @{$found{$ctype}}, $cand;
- warn "\tFound as $cand (really $fullname), type $ctype\n"
+ warn "\tFound as $cand (really $fullname), type $ctype\n"
if $verbose > 1;
push @flibs, $name unless $libs_seen{$fullname}++;
next LIB;
diff --git a/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Platform/Windows.pm b/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Platform/Windows.pm
index 472c801..6c77fd4 100644
--- a/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Platform/Windows.pm
+++ b/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Platform/Windows.pm
@@ -1,5 +1,5 @@
package ExtUtils::CBuilder::Platform::Windows;
-$ExtUtils::CBuilder::Platform::Windows::VERSION = '0.280221';
+$ExtUtils::CBuilder::Platform::Windows::VERSION = '0.280222';
use strict;
use warnings;
@@ -57,7 +57,7 @@ sub split_like_shell {
# array) to the target program and make the program parse it itself,
# we don't actually need to do any processing here.
(my $self, local $_) = @_;
-
+
return @$_ if defined() && UNIVERSAL::isa($_, 'ARRAY');
return unless defined() && length();
return ($_);
@@ -76,7 +76,7 @@ sub do_system {
sub arg_defines {
my ($self, %args) = @_;
s/"/\\"/g foreach values %args;
- return map qq{"-D$_=$args{$_}"}, keys %args;
+ return map qq{"-D$_=$args{$_}"}, sort keys %args;
}
sub compile {
@@ -85,7 +85,7 @@ sub compile {
die "Missing 'source' argument to compile()" unless defined $args{source};
- $args{include_dirs} = [ $args{include_dirs} ]
+ $args{include_dirs} = [ $args{include_dirs} ]
if exists($args{include_dirs}) && ref($args{include_dirs}) ne "ARRAY";
my ($basename, $srcdir) =
diff --git a/dist/ExtUtils-CBuilder/t/04-base.t b/dist/ExtUtils-CBuilder/t/04-base.t
index 3b525b7..b8b5e69 100644
--- a/dist/ExtUtils-CBuilder/t/04-base.t
+++ b/dist/ExtUtils-CBuilder/t/04-base.t
@@ -1,7 +1,7 @@
#! perl -w
use strict;
-use Test::More tests => 64;
+use Test::More tests => 65;
use Config;
use Cwd;
use File::Path qw( mkpath );
@@ -152,7 +152,6 @@ my ($lib, @temps);
ok( -d $include_dir, "perl_inc() returned directory" );
}
-#
$base = ExtUtils::CBuilder::Base->new( quiet => 1 );
ok( $base, "ExtUtils::CBuilder::Base->new() returned true value" );
isa_ok( $base, 'ExtUtils::CBuilder::Base' );
@@ -165,13 +164,28 @@ my %args = ();
my @defines = $base->arg_defines( %args );
ok( ! @defines, "Empty hash passed to arg_defines() returns empty list" );
-%args = ( alpha => 'beta', gamma => 'delta' );
-my $defines_seen_ref = { map { $_ => 1 } $base->arg_defines( %args ) };
+my @epsilon = ( epsilon => 'zeta' );
+my @eta = ( eta => 'theta' );
+my @alpha = ( alpha => 'beta' );
+my @gamma = ( gamma => 'delta' );
+my @all = (\@epsilon, \@eta, \@alpha, \@gamma);
+
+%args = map { @{$_} } @all;
+@defines = $base->arg_defines( %args );
+my $defines_seen_ref = { map { $_ => 1 } @defines };
+my $defines_expected_ref;
+#for my $r (\@epsilon, \@eta, \@alpha, \@gamma) {
+for my $r (@all) {
+ $defines_expected_ref->{"-D$r->[0]=$r->[1]"} = 1;
+}
is_deeply(
$defines_seen_ref,
- { '-Dalpha=beta' => 1, '-Dgamma=delta' => 1 },
+ $defines_expected_ref,
"arg_defines(): got expected defines",
);
+my $ordered_defines_expected_ref = [ sort keys %{$defines_expected_ref} ];
+is_deeply(\@defines, $ordered_defines_expected_ref,
+ "Got expected order of defines: RT #124106");
my $include_dirs_seen_ref =
{ map {$_ => 1} $base->arg_include_dirs( qw| alpha beta gamma | ) };
--
1.9.1
|
The RT System itself - Status changed from 'new' to 'open' |
From ambs@zbr.ptHello, James Was preparing to patch ExtUtils-CBuilder (outside Perl) and propose a patch to blead Perl. But in case you already apply it, we can do it the other way around. Let me know how you prefer to do it. Cheers |
From @jkeenanOn Sun Mar 22 06:55:20 2015, ambs wrote:
ambs, good to hear from you! Since the core distribution is now in code freeze, we wouldn't be applying any patch until after perl-5.22.0 is released in about two months. So, IMO, feel free to apply a patch to the CPAN version now and we'll synch-up for 5.23.1. Thank you very much. -- |
From @jkeenanOn Sun Mar 22 08:08:10 2015, jkeenan wrote:
Applied to blead: Impose deterministic order on cpp-definition options. ambs: Please update the CPAN distribution at your earliest convenience. Thank you very much. |
@jkeenan - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#124106 (status was 'resolved')
Searchable as RT124106$
The text was updated successfully, but these errors were encountered: