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] EU::CBuilder shouldn't override $Config{ccflags} with $ENV{CFLAGS} #11279

Closed
p5pRT opened this issue Apr 28, 2011 · 5 comments
Closed

[PATCH] EU::CBuilder shouldn't override $Config{ccflags} with $ENV{CFLAGS} #11279

p5pRT opened this issue Apr 28, 2011 · 5 comments

Comments

@p5pRT
Copy link

@p5pRT p5pRT commented Apr 28, 2011

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

Searchable as RT89478$

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Apr 28, 2011

From @ntyni

On Wed, Apr 27, 2011 at 05​:37​:56PM -0500, Craig A. Berry wrote​:

On Wed, Apr 27, 2011 at 4​:39 PM, Niko Tyni <ntyni@​debian.org> wrote​:

Failed 4 tests out of 2037, 99.80% okay.
       ../cpan/Module-Build/t/xs.t
       ../dist/ExtUtils-ParseXS/t/basic.t
       ../dist/ExtUtils-ParseXS/t/more.t
       ../dist/ExtUtils-ParseXS/t/usage.t

What's happening seems to be that when CFLAGS is set, it overrides
other flags in ExtUtils​::CBuilder. One of these, -D_FILE_OFFSET_BITS=64,
is apparently crucial on this platform for some reason.

Bisecting shows the crucial thing that breaks this is
-D_FILE_OFFSET_BITS=64 missing. Don't know why.

It looks like this is a change in ExtUtils​::CBuilder, maybe
 Perl-Toolchain-Gang/ExtUtils-CBuilder@e653d24

I doubt there's anything crucial about the particular flag, but rather
it's the fact that you're building extensions using flags that give
you code that is binary incompatible with the perl binary it's being
built against. It looks like your expectation is that you can add an
optimization flag by specifying only that flag in CFLAGS. Sounds
reasonable, but clearly what EU​::CB is doing is dropping everything it
got from $Config{ccflags} and replacing rather than supplementing it
with what you gave it in CFLAGS.

Yes, I see. Thanks. Perlbugging this with the attached proposed patch.
FWIW, I think LDFLAGS should be treated in the same way.

With options like -D_LARGEFILE_SOURCE and -D_FILE_OFFSET_BITS=64' used
to build Perl but dropped when testing extension building, you could
be getting a different and incompatible stat structure or other binary
incompatible differences between the extension and the Perl core.

It seems like the principle of least surprise might be to make EU​::CB
take CFLAGS as a supplement to $Config{ccflags} rather than as a
replacement. It would still be possible to create binary
incompatibilities, but perhaps less likely.
--
Niko Tyni ntyni@​debian.org

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Apr 28, 2011

From @ntyni

0001-Append-CFLAGS-and-LDFLAGS-to-their-Config.pm-counter.patch
From bb249b0b26c2e79a6f55355ef94889070f07fd21 Mon Sep 17 00:00:00 2001
From: Niko Tyni <ntyni@debian.org>
Date: Thu, 28 Apr 2011 09:18:54 +0300
Subject: [PATCH] Append CFLAGS and LDFLAGS to their Config.pm counterparts in
 EU::CBuilder

Since ExtUtils::CBuilder 0.27_04 (bleadperl commit 06e8058f27e4),
CFLAGS and LDFLAGS from the environment have overridden the Config.pm
ccflags and ldflags settings. This can cause binary incompatibilities
between the core Perl and extensions built with EU::CBuilder.

Append to the Config.pm values rather than overriding them.
---
 .../lib/ExtUtils/CBuilder/Base.pm                  |    6 +++-
 dist/ExtUtils-CBuilder/t/04-base.t                 |   25 +++++++++++++++++++-
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Base.pm b/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Base.pm
index b572312..2255c51 100644
--- a/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Base.pm
+++ b/dist/ExtUtils-CBuilder/lib/ExtUtils/CBuilder/Base.pm
@@ -40,11 +40,13 @@ sub new {
     $self->{config}{$k} = $v unless exists $self->{config}{$k};
   }
   $self->{config}{cc} = $ENV{CC} if defined $ENV{CC};
-  $self->{config}{ccflags} = $ENV{CFLAGS} if defined $ENV{CFLAGS};
+  $self->{config}{ccflags} = join(" ", $self->{config}{ccflags}, $ENV{CFLAGS})
+     if defined $ENV{CFLAGS};
   $self->{config}{cxx} = $ENV{CXX} if defined $ENV{CXX};
   $self->{config}{cxxflags} = $ENV{CXXFLAGS} if defined $ENV{CXXFLAGS};
   $self->{config}{ld} = $ENV{LD} if defined $ENV{LD};
-  $self->{config}{ldflags} = $ENV{LDFLAGS} if defined $ENV{LDFLAGS};
+  $self->{config}{ldflags} = join(" ", $self->{config}{ldflags}, $ENV{LDFLAGS})
+     if defined $ENV{LDFLAGS};
 
   unless ( exists $self->{config}{cxx} ) {
     my ($ccpath, $ccbase, $ccsfx ) = fileparse($self->{config}{cc}, qr/\.[^.]*/);
diff --git a/dist/ExtUtils-CBuilder/t/04-base.t b/dist/ExtUtils-CBuilder/t/04-base.t
index c3bf6b5..1bb15aa 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 => 50;
+use Test::More tests => 64;
 use Config;
 use Cwd;
 use File::Path qw( mkpath );
@@ -326,6 +326,29 @@ is_deeply( $mksymlists_args,
     "_prepare_mksymlists_args(): got expected arguments for Mksymlists",
 );
 
+my %testvars = (
+    CFLAGS  => 'ccflags',
+    LDFLAGS => 'ldflags',
+);
+
+while (my ($VAR, $var) = each %testvars) {
+    local $ENV{$VAR};
+    $base = ExtUtils::CBuilder::Base->new( quiet => 1 );
+    ok( $base, "ExtUtils::CBuilder::Base->new() returned true value" );
+    isa_ok( $base, 'ExtUtils::CBuilder::Base' );
+    like($base->{config}{$var}, qr/\Q$Config{$var}/,
+        "honours $var from Config.pm");
+
+    $ENV{$VAR} = "-foo -bar";
+    $base = ExtUtils::CBuilder::Base->new( quiet => 1 );
+    ok( $base, "ExtUtils::CBuilder::Base->new() returned true value" );
+    isa_ok( $base, 'ExtUtils::CBuilder::Base' );
+    like($base->{config}{$var}, qr/\Q$ENV{$VAR}/,
+        "honours $VAR from the environment");
+    like($base->{config}{$var}, qr/\Q$Config{$var}/,
+        "doesn't override $var from Config.pm with $VAR from the environment");
+}
+
 #####
 
 for ($source_file, $object_file, $lib_file) {
-- 
1.7.4.4

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 19, 2011

From @cpansprout

On Thu Apr 28 08​:02​:02 2011, ntyni@​debian.org wrote​:

On Wed, Apr 27, 2011 at 05​:37​:56PM -0500, Craig A. Berry wrote​:

On Wed, Apr 27, 2011 at 4​:39 PM, Niko Tyni <ntyni@​debian.org> wrote​:

Failed 4 tests out of 2037, 99.80% okay.
� � � �../cpan/Module-Build/t/xs.t
� � � �../dist/ExtUtils-ParseXS/t/basic.t
� � � �../dist/ExtUtils-ParseXS/t/more.t
� � � �../dist/ExtUtils-ParseXS/t/usage.t

What's happening seems to be that when CFLAGS is set, it overrides
other flags in ExtUtils​::CBuilder. One of these,
-D_FILE_OFFSET_BITS=64,
is apparently crucial on this platform for some reason.

Bisecting shows the crucial thing that breaks this is
-D_FILE_OFFSET_BITS=64 missing. Don't know why.

It looks like this is a change in ExtUtils​::CBuilder, maybe
Perl-Toolchain-Gang/ExtUtils-CBuilder@e653d24

I doubt there's anything crucial about the particular flag, but rather
it's the fact that you're building extensions using flags that give
you code that is binary incompatible with the perl binary it's being
built against. It looks like your expectation is that you can add an
optimization flag by specifying only that flag in CFLAGS. Sounds
reasonable, but clearly what EU​::CB is doing is dropping everything it
got from $Config{ccflags} and replacing rather than supplementing it
with what you gave it in CFLAGS.

Yes, I see. Thanks. Perlbugging this with the attached proposed patch.
FWIW, I think LDFLAGS should be treated in the same way.

Thank you. Applied as 011e8fb.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 19, 2011

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

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 19, 2011

@cpansprout - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant