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

ExtUtils-CBuilder non-deterministic define option order #14599

Closed
p5pRT opened this issue Mar 18, 2015 · 9 comments
Closed

ExtUtils-CBuilder non-deterministic define option order #14599

p5pRT opened this issue Mar 18, 2015 · 9 comments

Comments

@p5pRT
Copy link

p5pRT commented Mar 18, 2015

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

Searchable as RT124106$

@p5pRT
Copy link
Author

p5pRT commented Mar 18, 2015

From zefram@fysh.org

(Redirected from [rt.cpan.org #102830].)

ExtUtils​::CBuilder puts cpp-definition options into the cc command line
in non-deterministic order. This produces noise when diffing build logs.
Attached patch makes it deterministic.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Mar 18, 2015

From zefram@fysh.org

deterministic_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 {

@p5pRT
Copy link
Author

p5pRT commented Mar 19, 2015

From @jkeenan

On Wed Mar 18 03​:44​:13 2015, zefram@​fysh.org wrote​:

(Redirected from [rt.cpan.org #102830].)

ExtUtils​::CBuilder puts cpp-definition options into the cc command line
in non-deterministic order. This produces noise when diffing build logs.
Attached patch makes it deterministic.

-zefram

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.

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

@p5pRT
Copy link
Author

p5pRT commented Mar 19, 2015

From @jkeenan

124106-0001-Impose-deterministic-order-on-cpp-definition-options.patch
From 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

@p5pRT
Copy link
Author

p5pRT commented Mar 19, 2015

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

@p5pRT
Copy link
Author

p5pRT commented Mar 22, 2015

From ambs@zbr.pt

Hello, 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
ambs

@p5pRT
Copy link
Author

p5pRT commented Mar 22, 2015

From @jkeenan

On Sun Mar 22 06​:55​:20 2015, ambs wrote​:

Hello, 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
ambs

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.

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

@p5pRT
Copy link
Author

p5pRT commented Jun 2, 2015

From @jkeenan

On Sun Mar 22 08​:08​:10 2015, jkeenan wrote​:

On Sun Mar 22 06​:55​:20 2015, ambs wrote​:

Hello, 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
ambs

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.

Applied to blead​:
#####
commit 4392616
Author​: Zefram <zefram@​fysh.org>
Date​: Wed Mar 18 19​:49​:28 2015 -0400

  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. Remove commented-out line.
 
  For​: RT #124106
#####

ambs​: Please update the CPAN distribution at your earliest convenience.

Thank you very much.
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT p5pRT closed this as completed Jun 2, 2015
@p5pRT
Copy link
Author

p5pRT commented Jun 2, 2015

@jkeenan - Status changed from 'open' 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