Bug 727364 - Setting environ['CFLAGS'] in a distutils setup.py file results in missing compile flags
Setting environ['CFLAGS'] in a distutils setup.py file results in missing com...
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: python (Show other bugs)
6.1
i386 Linux
medium Severity medium
: rc
: ---
Assigned To: Dave Malcolm
Petr Šplíchal
: Reopened
Depends On:
Blocks: 849994
  Show dependency treegraph
 
Reported: 2011-08-01 19:45 EDT by Bob Arendt
Modified: 2016-05-31 21:42 EDT (History)
2 users (show)

See Also:
Fixed In Version: python-2.6.6-28.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 849994 (view as bug list)
Environment:
Last Closed: 2011-12-06 05:24:25 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Partial side-by-side diff of /usr/lib/python-2.x/distutils/sysconfig.py (44.60 KB, patch)
2011-08-01 19:45 EDT, Bob Arendt
no flags Details | Diff
Partial side-by-side diff of /usr/lib/python-2.x/distutils/sysconfig.py (3.46 KB, text/plain)
2011-08-01 19:51 EDT, Bob Arendt
no flags Details
Possible candidate patch (1.28 KB, patch)
2011-08-11 19:48 EDT, Dave Malcolm
no flags Details | Diff
Alternate, simpler patch (740 bytes, patch)
2011-08-12 17:52 EDT, Dave Malcolm
no flags Details | Diff

  None (edit)
Description Bob Arendt 2011-08-01 19:45:07 EDT
Created attachment 516223 [details]
Partial side-by-side diff of /usr/lib/python-2.x/distutils/sysconfig.py

Description of problem:  (Build-system regression between RHEL5 -> RHEL6)

When building a custom python C or C++ module we use the python distutils module, part of the python rpm. The user creates a setup.py config file.  To append flags to the compile line the user adds lines like this:
  from os import environ
  environ['CFLAGS']='-Wno-write-strings'  # your options may vary
This is supposed to get *merged* with all the flags that were originally used to build the python baseline.  This works in the python 2.4 supplied with RHEL5.  There is a regression in RHEL6.  When the user specifies CFLAGS like this, some critical build flags are inhibited.  Looking at the python source, this appears to be an obvious coding error on line 186 of /usr/lib/python2.6/distutils/sysconfig.py.

Version-Release number of selected component (if applicable):
python-2.6.6-20.el6.i686  (and all prior python-2.6 packages)

How reproducible:
Always

Steps to Reproduce:
1. Use a distutils "setup.py" file to build a python module
2. Add a build flag using environ['CFLAGS'] = '-yourflaghere'
3. Build using: python setup.py build

Actual results:
Build errors, due to the lack of -fno-strict-aliasing flag. The CFLAGS should be *added* to the existing CFLAGS from the original build, but they result in a subset of the original flags.

Expected results:
No build errors, since CFLAGS is appended to the original python build string.

Additional info:
It looks like this error was introduced in python 2.5, according to this:
http://bugs.python.org/issue969718
.. and it's propagated up into python 3.x

Comparing /usr/lib/python2.6/distutils/sysconfig.py to the RHEL 5 version (python 2.4) the error in customize_compiler() is obvious (see attached side-by-side diff).

In 2.4, the original 'OPT' flags gets 'BASECFLAGS' merged with it, then any user-provided optional CFLAGS and CPPFLAGS are merged with it to form the final cc_cmd.

In 2.6, upstream mucked it up.  Looking at what gets recorded in get_config_vars() 'OPT' and 'CFLAGS', it appears that CFLAGS is a super-set of OPT (all the OPT flags are duplicated at the end of CFLAGS).  The problem happens at lines:
   185          if 'CFLAGS' in os.environ:
   186              cflags = opt + ' ' + os.environ['CFLAGS']
.. snip ..
   193          cc_cmd = cc + ' ' + cflags

Now if the user specifies CFLAGS, the original build's CFLAGS gets replaced with OPT.  Which is a subset, most notably missing '-fno-strict-aliasing' which is fairly critical to ensure correct code compilation.  This looks like a simple coding mistake;  When modifying this section to use cflags instead of opt as the container for compile options, line 186 *should* have been:
   186              cflags = cflags + ' ' + os.environ['CFLAGS']
 .. or ..
   186              cflags = cflags + ' ' + opt + ' ' + os.environ['CFLAGS']

At this point, python upstream doesn't care about the 2.6.x tree.  I would be great if Red Hat would patch this bug in the RHEL 6 python.
Comment 1 Bob Arendt 2011-08-01 19:51:09 EDT
Created attachment 516224 [details]
Partial side-by-side diff of /usr/lib/python-2.x/distutils/sysconfig.py
Comment 3 Bob Arendt 2011-08-01 19:52:22 EDT
Comment on attachment 516224 [details]
Partial side-by-side diff of /usr/lib/python-2.x/distutils/sysconfig.py

Python 2.4 on the left, 2.6 source on the right.
Comment 4 Dave Malcolm 2011-08-11 19:27:27 EDT
Thanks for filing this bug report.

The usage of BASECFLAGS was originally added to distutils.sysconfig back in 2003 in:
  http://svn.python.org/view?view=revision&revision=31317

It was removed in 2005 with:
  http://svn.python.org/view?view=revision&revision=38848

So it looks like this change affects 2.5 tarballs onwards

One possible workaround might be something like this in each setup.py, to get the desired append behavior:

   from distutils.sysconfig import get_config_var
   cv = get_config_var('CFLAGS')this contains 
   environ['CFLAGS'] = cv + ' ' + '-yourflaghere'

In particular, this does pull in the "-fno-strict-aliasing" flag.

However, am I right in thinking that you have a number of such setup.py files?

I'm looking at possible patches to distutils.sysconfig for this.
Comment 5 Dave Malcolm 2011-08-11 19:48:06 EDT
Created attachment 517911 [details]
Possible candidate patch

The attached candidate patch changes the handling of when CFLAGS is specified in the environment

With this patch, if CFLAGS is in the environment, it regenerates the cflags based on:
  (BASECFLAGS from Makefile)[$CFLAGS from env](OPT from Makefile)(EXTRA_CFLAGS from Makefile)
rather than appending them to OPT from the Makefile.

Given that CFLAGS in the Makefile is currently:
   $(BASECFLAGS) [copy of OPT without the -DNDEBUG] $(OPT) $(EXTRA_CFLAGS)
this would allow code that overrides CFLAGS to get the full build flags.

Unlike the Debian patch referred to in http://bugs.python.org/issue969718 it doesn't directly try to add support for additional Makefile variables being overridden by environment variables (OPT, BASECFLAGS); this is an attempt to minimize the risk of this change.  (Though parse_makefile() can already fall back to reading vars from the environment, if they aren't defined).

I'm in two minds about this versus the more simple:
  -            cflags = opt + ' ' + os.environ['CFLAGS']
  +            cflags = cflags + ' ' + os.environ['CFLAGS']
but need to think through the possible risks of either candidate change, in the light of possible pre-existing code that may set arbitrary combinations of BASECFLAGS, OPT, CFLAGS, EXTRA_CFLAGS in the build environment.
Comment 6 Bob Arendt 2011-08-11 21:42:25 EDT
Great detective work looking up the change history!

Actually, I have a single very simple setup.py:
----------------
environ["CFLAGS"] = "-Wno-write-string"

module1 = Extension('psm',
                    include_dirs = [src+'/inc'], 
                    library_dirs = [bin+'/lib'],
                    libraries = ['psm','stdc++'],
                    sources = ['psm_module.cc'])

setup (name = 'psm',
       version = '1.0',
       description = 'PSM support',
       ext_modules = [module1])
----------------

But our customers execute this build on RHEL3 .. RHEL6 and Fedora.  The additional flag was just to suppress a compiler warning, in lieu of (char*) casting a bunch of static initializer strings (tuple KW lists).  I came across CFLAGS here:  http://docs.python.org/release/2.6.7/install/index.html?highlight=cflags

    Compiler flags can also be supplied through setting the CFLAGS
    environment variable. If set, the contents of CFLAGS will be
    added to the compiler flags specified in the Setup file.

The problem with the proposed work-around is that it looks like it's not backward compatible;  I'd have to check python versions then set CFLAGS accordingly.  Awkward, but it should work.

But there could be other folks out there that also used CFLAGS prior to RHEL6 that would hit this bug (showing some altruism :) ).  It would be really nice to be able to span RHEL versions without version checking.

The thing that I found confusing is the in python 2.6 sysconfig.py is that 'opt' is *only* used if CFLAGS is specified.  Otherwise it's ignored. That's what led me to think that this was search&replace error in the patch - they missed changing this instance of 'opt' to 'cflags'. In the revised build process, it appears that CFLAGS is a super-set of OPT.

The real question (that I don't have the background to answer) is what is the real intent of CFLAGS with respect to setup.py & distutils?  Should it augment or replace the original CFLAGS?  The python-docs suggest that it should be additive to the base set of flags.  If get_config_vars('CFLAGS') now contains the full set of build flags, it seems that just appending os.environ['CFLAGS'] would be best.  By re-creating CFLAGS, we're vulnerable to future changes in the python Makefile composition.  If the way the Makefile assembles CFLAGS changes, then distutils might break again.

Thanks for looking into this.
Comment 7 Dave Malcolm 2011-08-12 17:52:51 EDT
Created attachment 518113 [details]
Alternate, simpler patch

As far as patching RHEL, the simple solution seems best:
  -            cflags = opt + ' ' + os.environ['CFLAGS']
  +            cflags = cflags + ' ' + os.environ['CFLAGS']

Case (i): a build that doesn't have CFLAGS set in the environment isn't affected.

Case (ii): a  build that does, changes: the compiler flags are changed from OPT + user-cflags to CFLAGS + cflags:

Given that CFLAGS in the Makefile is currently:
   $(BASECFLAGS) [copy of OPT without the -DNDEBUG] $(OPT) $(EXTRA_CFLAGS)
then for case (ii), the compilation flags change from:
   $(OPT) + $(user CFLAGS)
to:
   $(BASECFLAGS) [copy of OPT without the -DNDEBUG] $(OPT) $(EXTRA_CFLAGS) + $(user CFLAGS)

This means that the compilation flags gain:
   -fno-strict-aliasing
along with various duplicates of other options.

Both with and without the proposed change, a build that has OPT or BASECFLAGS in the environment isn't affected by them.

As is, a build that has EXTRA_CFLAGS set in the env is affected by it until CFLAGS is set, which stops it taking effect.  With this proposed change, EXTRA_CFLAGS takes effect regardless of whether CFLAGS is set.  Arguably this is more correct.
Comment 8 Dave Malcolm 2011-08-12 17:56:16 EDT
(In reply to comment #6)
> casting a bunch of static initializer strings (tuple KW lists).  I came across
> CFLAGS here: 
> http://docs.python.org/release/2.6.7/install/index.html?highlight=cflags
> 
>     Compiler flags can also be supplied through setting the CFLAGS
>     environment variable. If set, the contents of CFLAGS will be
>     added to the compiler flags specified in the Setup file.

[...snip...]

> The real question (that I don't have the background to answer) is what is the
> real intent of CFLAGS with respect to setup.py & distutils?  Should it augment
> or replace the original CFLAGS?  The python-docs suggest that it should be
> additive to the base set of flags.  If get_config_vars('CFLAGS') now contains

Thanks for the documentation link!  I'm inclined to regard those docs as describing the intent: that it should augment, rather than replacing.
Comment 14 RHEL Product and Program Management 2011-08-14 15:05:07 EDT
Development Management has reviewed and declined this request.  You may appeal
this decision by reopening this request.
Comment 23 errata-xmlrpc 2011-12-06 05:24:25 EST
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2011-1564.html

Note You need to log in before you can comment on or make changes to this bug.