Skip to content

bpo-36235: Fix CFLAGS in distutils customize_compiler() #12236

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 15, 2019
Merged

bpo-36235: Fix CFLAGS in distutils customize_compiler() #12236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09232ca
bpo-36235: Fix CFLAGS in distutils customize_compiler()
vstinner Mar 8, 2019
ea99e61
Remove 'opt' local variable
vstinner Mar 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Lib/distutils/sysconfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,8 @@ def customize_compiler(compiler):
_osx_support.customize_compiler(_config_vars)
_config_vars['CUSTOMIZED_OSX_COMPILER'] = 'True'

(cc, cxx, opt, cflags, ccshared, ldshared, shlib_suffix, ar, ar_flags) = \
get_config_vars('CC', 'CXX', 'OPT', 'CFLAGS',
(cc, cxx, cflags, ccshared, ldshared, shlib_suffix, ar, ar_flags) = \
get_config_vars('CC', 'CXX', 'CFLAGS',
'CCSHARED', 'LDSHARED', 'SHLIB_SUFFIX', 'AR', 'ARFLAGS')

if 'CC' in os.environ:
Expand All @@ -205,7 +205,7 @@ def customize_compiler(compiler):
if 'LDFLAGS' in os.environ:
ldshared = ldshared + ' ' + os.environ['LDFLAGS']
if 'CFLAGS' in os.environ:
cflags = opt + ' ' + os.environ['CFLAGS']
cflags = cflags + ' ' + os.environ['CFLAGS']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe opt should be retained since there are compiler flags captured during ./configure which would get dropped when the CFLAGS environment variable is set invoking a build via distutils.

For example, on my macOS machine, the following compiler flags would be lost if CFLAGS was set:

>>> sysconfig.get_config_var('OPT')
'-DNDEBUG -g -fwrapv -O3 -Wall'

To the best of my understanding, it appears the goal is to have the compiler flags used when building the interpreter to be passed along to C/C++ extensions with the ability to add additional user-specified flags via the CFLAGS environment variable.

Given that the intent appears to be appending to the flags already captured during the configuration phase of building the interpreter, I would suggest:

Suggested change
cflags = cflags + ' ' + os.environ['CFLAGS']
cflags = opt + ' ' + cflags + ' ' + os.environ['CFLAGS']

If the propose change is accepted, then my follow-up feedback would be updating the commit message and news announcement accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sysconfig reads PY_FLAGS from Makefile and expose it as "CFLAGS". In the Makefile, PY_CFLAGS is composed from other variables:

PY_CFLAGS=	$(BASECFLAGS) $(OPT) $(CONFIGURE_CFLAGS) $(CFLAGS) $(EXTRA_CFLAGS)

So "OPT" sysconfig variable is a subset of "CFLAGS" sysconfig variable.

Note: I'm talking about the specific case of sysconfig variables.

Python compiled manually:

$ ./python
Python 3.8.0a2+ (heads/common_config2-dirty:3fe7fa316f, Mar 14 2019, 23:17:45) 
>>> import shlex, sysconfig
>>> cflags = set(shlex.split(sysconfig.get_config_var('CFLAGS')))
>>> opt = set(shlex.split(sysconfig.get_config_var('OPT')))
>>> opt.issubset(cflags)
True
>>> cflags - opt
{'-Wno-unused-result', '-O0', '-Wsign-compare'}
>>> opt - cflags
set()

/usr/bin/python3:

vstinner@apu$ python3
Python 3.7.2 (default, Jan 16 2019, 19:49:22) 
>>> import shlex, sysconfig
>>> cflags = set(shlex.split(sysconfig.get_config_var('CFLAGS')))
>>> opt = set(shlex.split(sysconfig.get_config_var('OPT')))
>>> opt.issubset(cflags)
True
>>> cflags - opt
{'-Wsign-compare', '-Wno-unused-result'}
>>> opt - cflags
set()

Copy link
Contributor

@ericvw ericvw Mar 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vstinner, thanks for the walkthrough and I have arrived at the same conclusion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If my concern about opt turns out to not be valid and the decision is to move forward with the original proposed changed, the opt variable, and it's acquired value fromget_config_vars([...], 'OPT', [...]), can be removed since it would be unused code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Fixed.

ldshared = ldshared + ' ' + os.environ['CFLAGS']
if 'CPPFLAGS' in os.environ:
cpp = cpp + ' ' + os.environ['CPPFLAGS']
Expand Down
15 changes: 11 additions & 4 deletions Lib/distutils/tests/test_sysconfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from distutils import sysconfig
from distutils.ccompiler import get_default_compiler
from distutils.tests import support
from test.support import TESTFN, run_unittest, check_warnings
from test.support import TESTFN, run_unittest, check_warnings, swap_item

class SysconfigTestCase(support.EnvironGuard, unittest.TestCase):
def setUp(self):
Expand Down Expand Up @@ -78,7 +78,9 @@ def test_srcdir_independent_of_cwd(self):
'not testing if default compiler is not unix')
def test_customize_compiler(self):
os.environ['AR'] = 'my_ar'
os.environ['ARFLAGS'] = '-arflags'
os.environ['CC'] = 'my_cc'
os.environ['ARFLAGS'] = '--myarflags'
os.environ['CFLAGS'] = '--mycflags'

# make sure AR gets caught
class compiler:
Expand All @@ -87,9 +89,14 @@ class compiler:
def set_executables(self, **kw):
self.exes = kw

# Make sure that sysconfig._config_vars is initialized
sysconfig.get_config_vars()

comp = compiler()
sysconfig.customize_compiler(comp)
self.assertEqual(comp.exes['archiver'], 'my_ar -arflags')
with swap_item(sysconfig._config_vars, 'CFLAGS', '--sysconfig-cflags'):
sysconfig.customize_compiler(comp)
self.assertEqual(comp.exes['archiver'], 'my_ar --myarflags')
self.assertEqual(comp.exes['compiler'], 'my_cc --sysconfig-cflags --mycflags')

def test_parse_makefile_base(self):
self.makefile = TESTFN
Expand Down
4 changes: 4 additions & 0 deletions Misc/NEWS.d/next/Library/2019-03-08-13-32-21.bpo-36235._M72wU.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Fix ``CFLAGS`` in ``customize_compiler()`` of ``distutils.sysconfig``: when
the ``CFLAGS`` environment variable is defined, don't override ``CFLAGS``
variable with the ``OPT`` variable anymore. Initial patch written by David
Malcolm.