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

vstinner · 2019-03-08T12:34:53Z

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.

Co-Authored-By: David Malcolm dmalcolm@redhat.com

https://bugs.python.org/issue36235

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. Co-Authored-By: David Malcolm <dmalcolm@redhat.com>

vstinner · 2019-03-08T12:36:05Z

cc @izbyshev @tirkarthi

ericvw · 2019-03-08T16:52:02Z

@vstinner, I just ran into this exact issue today when needing to add to the CFLAGS captured during ./configure time.

There is also #7997 I have out there for address CPPFLAGS being passed through to distutils from ./configure.

vstinner · 2019-03-14T16:48:13Z

@ned-deily @stratakis: Would you mind to review this change?

vstinner · 2019-03-14T16:49:13Z

@ericvw @pablogsal: Would you mind to review this PR?

ericvw · 2019-03-15T02:01:18Z

@ericvw @pablogsal: Would you mind to review this PR?

Sure — will review soon.

ericvw · 2019-03-15T02:03:57Z

Lib/distutils/sysconfig.py

@@ -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']


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.

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()

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

ericvw · 2019-03-15T02:11:32Z

Lib/distutils/sysconfig.py

@@ -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']


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.

Right. Fixed.

get_config_var('OPT') is no longer used.

ericvw

lgtm.

Excited for this fix :)

ericvw · 2019-03-15T12:09:45Z

Lib/distutils/sysconfig.py

@@ -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']


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

miss-islington · 2019-03-15T13:57:55Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.7.
🐍🍒⛏🤖

bedevere-bot · 2019-03-15T14:16:45Z

GH-12348 is a backport of this pull request to the 3.7 branch.

bedevere-bot · 2019-03-15T14:19:38Z

GH-12349 is a backport of this pull request to the 2.7 branch.

…H-12348) 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. Co-Authored-By: David Malcolm <dmalcolm@redhat.com> (cherry picked from commit 86082c2)

…H-12349) 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. Co-Authored-By: David Malcolm <dmalcolm@redhat.com> (cherry picked from commit 86082c2)

the-knights-who-say-ni added the CLA signed label Mar 8, 2019

bedevere-bot added the awaiting merge label Mar 8, 2019

vstinner requested a review from pablogsal March 8, 2019 12:35

vstinner added needs backport to 2.7 labels Mar 8, 2019

ericvw suggested changes Mar 15, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting merge labels Mar 15, 2019

Remove 'opt' local variable

ea99e61

get_config_var('OPT') is no longer used.

ericvw approved these changes Mar 15, 2019

View reviewed changes

vstinner merged commit 86082c2 into python:master Mar 15, 2019

bedevere-bot removed the awaiting core review label Mar 15, 2019

vstinner deleted the distutils_cflags branch March 15, 2019 13:58

bedevere-bot removed the needs backport to 3.7 label Mar 15, 2019

bedevere-bot removed the needs backport to 2.7 label Mar 15, 2019

ericvw mentioned this pull request Mar 15, 2019

bpo-33990: Add CPPFLAGS during ./configure when customizing the compiler in distutils #7997

Closed

yan12125 mentioned this pull request Mar 17, 2019

bpo-36235: make test_customize_compiler more robust #12380

Closed

jjhelmus mentioned this pull request Oct 15, 2019

python v3.8.0 conda-forge/python-feedstock#274

Merged

3 tasks

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

Uh oh!

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

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

Uh oh!

Conversation

vstinner commented Mar 8, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Mar 8, 2019

Uh oh!

ericvw commented Mar 8, 2019

Uh oh!

vstinner commented Mar 14, 2019

Uh oh!

vstinner commented Mar 14, 2019

Uh oh!

ericvw commented Mar 15, 2019

Uh oh!

ericvw Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

ericvw Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericvw Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

ericvw left a comment

Choose a reason for hiding this comment

Uh oh!

ericvw Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Mar 15, 2019

Uh oh!

bedevere-bot commented Mar 15, 2019

Uh oh!

bedevere-bot commented Mar 15, 2019

Uh oh!

Uh oh!

vstinner commented Mar 8, 2019 •

edited by bedevere-bot

Loading

ericvw Mar 15, 2019 •

edited

Loading

ericvw Mar 15, 2019 •

edited

Loading