This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author jyasskin
Recipients Arfrever, brett.cannon, jyasskin, lemburg, loewis, mark.dickinson, pitrou
Date 2010-07-08.04:45:32
SpamBayes Score 0.0009898274
Marked as misclassified No
Message-id <AANLkTinsptl2j3XncD8obVYFy9rI0W9tjTnmYgqxz7fx@mail.gmail.com>
In-reply-to <4C34FDDD.90604@egenix.com>
Content
> 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.
History
Date User Action Args
2010-07-08 04:45:52jyasskinsetrecipients: + jyasskin, lemburg, loewis, brett.cannon, mark.dickinson, pitrou, Arfrever
2010-07-08 04:45:49jyasskinlinkissue9189 messages
2010-07-08 04:45:33jyasskincreate