Improve CFLAGS handling

[jyasskin]

BPO	9189
Nosy	@malemburg, @loewis, @brettcannon, @ronaldoussoren, @mdickinson, @pitrou, @tarekziade, @merwok, @skrah

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2011-02-04.01:12:02.966>
created_at = <Date 2010-07-07.15:51:12.450>
labels = ['type-bug', 'build']
title = 'Improve CFLAGS handling'
updated_at = <Date 2013-05-20.15:04:19.190>
user = 'https://bugs.python.org/jyasskin'

bugs.python.org fields:

activity = <Date 2013-05-20.15:04:19.190>
actor = 'jedbrown'
assignee = 'none'
closed = True
closed_date = <Date 2011-02-04.01:12:02.966>
closer = 'brett.cannon'
components = ['Build']
creation = <Date 2010-07-07.15:51:12.450>
creator = 'jyasskin'
dependencies = []
files = []
hgrepos = []
issue_num = 9189
keywords = []
message_count = 18.0
messages = ['109481', '109494', '109506', '109512', '109763', '109782', '109791', '110742', '110764', '114018', '114081', '114099', '114583', '116758', '118332', '122243', '127848', '189669']
nosy_count = 13.0
nosy_names = ['lemburg', 'loewis', 'brett.cannon', 'ronaldoussoren', 'mark.dickinson', 'pitrou', 'jyasskin', 'tarek', 'eric.araujo', 'rpetrov', 'Arfrever', 'skrah', 'jedbrown']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue9189'
versions = ['Python 3.2']

[jyasskin]

Patch at http://codereview.appspot.com/1749042.

The idea here is to let the user set CFLAGS on either configure or make (or both), and have later settings appear later in the $CC command line. I left OPT, BASECFLAGS, and EXTRA_CFLAGS around in case people have written scripts using them, but I think they're superfluous as user-visible knobs after this patch.

I prevented AC_PROG_CC from setting a default $CFLAGS value because the values it would set are already put into $BASECFLAGS when appropriate, and @CFLAGS@ needs to appear after @BASECFLAGS@ to allow the user to override Python's defaults at configure time. We could also accomplish this by removing BASECFLAGS and OPT entirely and instead prepending their contents to $CFLAGS in configure. That's a bigger patch, but if any of you feel strongly about it I can do that instead.

I made the same changes for CPPFLAGS and LDFLAGS but no other user-settable variables. I don't have strong opinions about the exact set we support this for, as long as it includes CFLAGS, but these three seemed like a sensible set.

[brettcannon]

Just to say what I said in my Rietveld review of the patch: I support the approach Jeffrey is taking and the patch looks good.

[malemburg]

Jeffrey Yasskin wrote:

New submission from Jeffrey Yasskin <jyasskin@gmail.com>:

Patch at http://codereview.appspot.com/1749042.

The idea here is to let the user set CFLAGS on either configure or make (or both), and have later settings appear later in the $CC command line. I left OPT, BASECFLAGS, and EXTRA_CFLAGS around in case people have written scripts using them, but I think they're superfluous as user-visible knobs after this patch.

Passing in user values to CFLAGS on configure should already work
without the patch. I'm not sure why you'd want to modify CFLAGS
on Makefile.

I prevented AC_PROG_CC from setting a default $CFLAGS value because the values it would set are already put into $BASECFLAGS when appropriate, and @CFLAGS@ needs to appear after @BASECFLAGS@ to allow the user to override Python's defaults at configure time. We could also accomplish this by removing BASECFLAGS and OPT entirely and instead prepending their contents to $CFLAGS in configure. That's a bigger patch, but if any of you feel strongly about it I can do that instead.

We've already had a long discussion about the use of AC_PROG_CC on another
ticket. We are you trying to remove the default setting again ?

BASECFLAGS is not supported by autoconf, it's a Python invention. Do you
really want to put all the logic that autoconf uses to setup CFLAGS currently
and in the future into our own configure code ? Are you willing to maintain
that code ?

We've also discussed switching the order of $(OPT) and $(CFLAGS): that doesn't
work, since $(OPT) is meant to override the $(CFLAGS) settings.

I made the same changes for CPPFLAGS and LDFLAGS but no other user-settable variables. I don't have strong opinions about the exact set we support this for, as long as it includes CFLAGS, but these three seemed like a sensible set.

Some more comments:

I don't really like the approach you've taken with the variable naming.
It's better to stick to Makefile all caps standards and instead prepend
Python specific variables with a PY or PY_ prefix. The already existing
PY_CFLAGS should then be renamed to PY_RUNTIME_CFLAGS or just PYTHON_CFLAGS.

Please also keep the default values for CFLAGS et al around, ie.

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

Note that using ":=" will cause trouble with non-GNU makes. Why do
you need this ?

In summary: While we're breaking Makefile parsing anyway, we might as
well go for the big patch and remove the whole BASECFLAGS, OPT and
EXTRA_CFLAGS cruft. These cause all sorts of problem when building
extensions for Python and not even distutils always gets them
right (distutils needs to mimic the CFLAGS setup when setting up
the compiler, so it has to replicate what's going on in the Makefile
in distutils.utils.ccompiler.customize_compiler()).

[jyasskin]

Passing in user values to CFLAGS on configure should already work
without the patch.

Since the user's CFLAGS settings are overridden by Python's OPT
settings, it doesn't already work without the patch.

I'm not sure why you'd want to modify CFLAGS on Makefile.

If I run make clean; make CFLAGS=-Werror, I expect that to add
-Werror to the compilation line. I don't expect it to replace all of
the flags configure set up, and I don't expect to have to re-run
configure to add the -Werror. Doing this won't affect distutils'
behavior and won't automatically cause all of Python to get
recompiled, so it's not something people should do when installing
Python, but it's helpful during development.

We've already had a long discussion about the use of AC_PROG_CC on another
ticket. We are you trying to remove the default setting again ?

BASECFLAGS is not supported by autoconf, it's a Python invention. Do you
really want to put all the logic that autoconf uses to setup CFLAGS currently
and in the future into our own configure code ? Are you willing to maintain
that code ?

Given that AC_PROG_CC, in its entire history, has only grown the
ability to check for -g and -O2, I'm not too worried. Our configure
code already uses $ac_cv_prog_cc_g to take advantage of AC_PROG_CC's
detection of -g, and we have more sophisticated logic around -O2 than
it does. (Check the configure source if you don't believe that
AC_PROG_CC's CFLAGS handling is pretty simple.)

We've also discussed switching the order of $(OPT) and $(CFLAGS): that doesn't
work, since $(OPT) is meant to override the $(CFLAGS) settings.

It's meant to override AC_PROG_CC's bad CFLAGS settings. Once those
are out of the way, we don't need to surprise users with a non-working
CFLAGS anymore.

> I made the same changes for CPPFLAGS and LDFLAGS but no other user-settable variables. I don't have strong opinions about the exact set we support this for, as long as it includes CFLAGS, but these three seemed like a sensible set.

Some more comments:

I don't really like the approach you've taken with the variable naming.
It's better to stick to Makefile all caps standards and instead prepend
Python specific variables with a PY or PY_ prefix. The already existing
PY_CFLAGS should then be renamed to PY_RUNTIME_CFLAGS or just PYTHON_CFLAGS.

Sure.

Please also keep the default values for CFLAGS et al around, ie.

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

This doesn't quite work, since the make command line overrides that
CFLAGS assignment. I've saved the configure-provided values into
CONFIGURE_CFLAGS, etc.

Note that using ":=" will cause trouble with non-GNU makes. Why do
you need this ?

http://www.openbsd.org/cgi-bin/man.cgi?query=make and
http://www.freebsd.org/doc/en/books/pmake/variables.html say bsd make
supports :=. I'm not sure where to find docs for other makes. I don't
need := anymore, though, since I found another way to fix sysconfig.
Removed.

In summary: While we're breaking Makefile parsing anyway, we might as
well go for the big patch and remove the whole BASECFLAGS, OPT and
EXTRA_CFLAGS cruft. These cause all sorts of problem when building
extensions for Python and not even distutils always gets them
right (distutils needs to mimic the CFLAGS setup when setting up
the compiler, so it has to replicate what's going on in the Makefile
in distutils.utils.ccompiler.customize_compiler()).

Makefile parsing isn't the only thing we care about compatibility
with. I kept them around for people who've been setting them through
build scripts or who are just used to setting them on the command
line. Maybe they should go away too, but I think that's orthogonal to
this patch, and should happen with a lag (maybe a whole release) to
let people get used to CFLAGS first. That is, unless you can point out
practical problems with the change around AC_PROG_CC.

I've uploaded the new patch to http://codereview.appspot.com/1749042.

[jyasskin]

Hearing no further comments, I've committed this as r82746. Let me know if it breaks anything.

[mdickinson]

Jeffrey, the Linux buildbots are showing some distutils failures that look like they might be related to this change.

For example: http://www.python.org/dev/buildbot/all/builders/amd64%20gentoo%203.x/builds/1351

test output contains:

======================================================================
ERROR: test_parse_makefile_base (distutils.tests.test_sysconfig.SysconfigTestCase)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/buildbot/slave/py-build/3.x.norwitz-amd64/build/Lib/distutils/tests/support.py", line 16, in _capture_warnings
    return func(*args, **kw)
  File "/home/buildbot/slave/py-build/3.x.norwitz-amd64/build/Lib/distutils/tests/test_sysconfig.py", line 58, in test_parse_makefile_base
    d = sysconfig.parse_makefile(self.makefile)
  File "/home/buildbot/slave/py-build/3.x.norwitz-amd64/build/Lib/distutils/sysconfig.py", line 140, in parse_makefile
    return _sysconfig._parse_makefile(fn, g)
  File "/home/buildbot/slave/py-build/3.x.norwitz-amd64/build/Lib/sysconfig.py", line 265, in _parse_makefile
    done[var] = done['PY_' + var]
KeyError: 'PY_CFLAGS'

[jyasskin]

Oops. Thanks for telling me. Fixed in r82753.

[ronaldoussoren]

On 9 Jul, 2010, at 20:55, Jeffrey Yasskin wrote:

Jeffrey Yasskin <jyasskin@gmail.com> added the comment:

Oops. Thanks for telling me. Fixed in r82753.

I'm pretty sure this patch broke universal builds on OSX. Not the python build itself, but building 3th-party extensions. I'll commit a fix later on.

In the Makefile:

LDSHARED= $(CC) $(LDFLAGS) -bundle -undefined dynamic_lookup

'gcc'
>>> sysconfig.get_config_var('LDFLAGS')
'-arch i386 -arch ppc -arch x86_64 -isysroot /'
>>> sysconfig.get_config_var('LDSHARED')
'gcc  -bundle -undefined dynamic_lookup'

That is, the LDFLAGS aren't patched into the value of LDSHARED.

This is because LDFLAGS is actually PY_LDFLAGS in the makefile and the rename from PY_LDFLAGS to LDFLAGS happens *after* variable expansion in sysconfig.

This doesn't affect the build of python itself because the Makefile explictly sets LDFLAGS in the environment when running setup.py.

Ronald

[jyasskin]

Oops, sorry about that. Is the fix to change LDSHARED to
LDSHARED= $(CC) $(PY_LDFLAGS) -bundle -undefined dynamic_lookup
?

[skrah]

Starting with r82746 in py3k, I'm getting duplicate LDFLAGS in sysconfig:

make distclean
export BASECFLAGS="-ftest-coverage -fprofile-arcs"
export LDFLAGS="-fprofile-arcs"
./configure
make

$ ./python 
Python 3.2a0 (py3k:82746M, Aug 16 2010, 00:25:49) 
[GCC 4.1.3 20080623 (prerelease) (Ubuntu 4.1.2-23ubuntu3)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sysconfig
>>> sysconfig.get_config_var('LDFLAGS')
'-fprofile-arcs -fprofile-arcs'

test_sysconfig currently fails, so perhaps one of the duplicate flags
is mistakenly appended to LDFLAGS instead of LDSHARED:

$ ./python Lib/test/regrtest.py -uall test_sysconfig
[1/1] test_sysconfig
test test_sysconfig failed -- Traceback (most recent call last):
  File "/home/stefan/svn/py3k/Lib/test/test_sysconfig.py", line 285, in test_ldshared_value
    self.assertIn(ldflags, ldshared)
AssertionError: '-fprofile-arcs -fprofile-arcs' not found in 'gcc -pthread -shared'

[skrah]

This is what happens. In the Makefile, I have:

CONFIGURE_LDFLAGS= -fprofile-arcs
PY_LDFLAGS= $(CONFIGURE_LDFLAGS) $(LDFLAGS)

But since LDFLAGS=-fprofile-arcs is in the environment, too, we get the
duplication.

The sysconfig issue is related. In Lib/sysconfig.py, _parse_makefile
grabs LDFLAGS from the environment if it is set, so LDFLAGS ultimately
expands to '-fprofile-arcs -fprofile-arcs'.