-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
build_ext: always import _CONFIG_VARS from distutils.sysconfig instead of sysconfig #572
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,15 +16,32 @@ | |
except ImportError: | ||
_build_ext = _du_build_ext | ||
|
||
try: | ||
# Python 2.7 or >=3.2 | ||
from sysconfig import _CONFIG_VARS | ||
except ImportError: | ||
from distutils.sysconfig import get_config_var | ||
from distutils.sysconfig import get_config_var | ||
|
||
get_config_var("LDSHARED") # make sure _config_vars is initialized | ||
del get_config_var | ||
from distutils.sysconfig import _config_vars as _CONFIG_VARS | ||
|
||
|
||
def _customize_compiler_for_shlib(compiler): | ||
if sys.platform == "darwin": | ||
# building .dylib requires additional compiler flags on OSX; here we | ||
# temporarily substitute the pyconfig.h variables so that distutils' | ||
# 'customize_compiler' uses them before we build the shared libraries. | ||
tmp = _CONFIG_VARS.copy() | ||
try: | ||
# XXX Help! I don't have any idea whether these are right... | ||
_CONFIG_VARS['LDSHARED'] = ( | ||
"gcc -Wl,-x -dynamiclib -undefined dynamic_lookup") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As the comment above shows, also the original author wasn't 100% sure whether these are the correct flags. I found that I need to remove the
Others have encountered similar issues: e.g. see https://bitbucket.org/premake/premake-dev/issues/263/os-x-gcc-clang-and-the-infamous-wl-x clang also gives me a warning about the
Hopefully some else in the setuptools community knows more about how to compile dynamic libraries for OS X. |
||
_CONFIG_VARS['CCSHARED'] = " -dynamiclib" | ||
_CONFIG_VARS['SO'] = ".dylib" | ||
customize_compiler(compiler) | ||
finally: | ||
_CONFIG_VARS.clear() | ||
_CONFIG_VARS.update(tmp) | ||
else: | ||
customize_compiler(compiler) | ||
|
||
get_config_var("LDSHARED") # make sure _config_vars is initialized | ||
del get_config_var | ||
from distutils.sysconfig import _config_vars as _CONFIG_VARS | ||
|
||
have_rtld = False | ||
use_stubs = False | ||
|
@@ -124,20 +141,7 @@ def setup_shlib_compiler(self): | |
compiler = self.shlib_compiler = new_compiler( | ||
compiler=self.compiler, dry_run=self.dry_run, force=self.force | ||
) | ||
if sys.platform == "darwin": | ||
tmp = _CONFIG_VARS.copy() | ||
try: | ||
# XXX Help! I don't have any idea whether these are right... | ||
_CONFIG_VARS['LDSHARED'] = ( | ||
"gcc -Wl,-x -dynamiclib -undefined dynamic_lookup") | ||
_CONFIG_VARS['CCSHARED'] = " -dynamiclib" | ||
_CONFIG_VARS['SO'] = ".dylib" | ||
customize_compiler(compiler) | ||
finally: | ||
_CONFIG_VARS.clear() | ||
_CONFIG_VARS.update(tmp) | ||
else: | ||
customize_compiler(compiler) | ||
_customize_compiler_for_shlib(compiler) | ||
|
||
if self.include_dirs is not None: | ||
compiler.set_include_dirs(self.include_dirs) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems to be moving in the wrong direction, conflicting with the intention of the author to support the newer, preferred module. I don't know what the implications are, but what would prevent someone from making the reverse change again?
I think this behavior needs a unit test in the test suite to capture the desired behavior.
Also, is it possible to allow the newer, preferred module to still be used, but always invoke the
get_config_var
? What is the relationship betweensysconfig._CONFIG_VARS
anddistutils.sysconfig._config_vars
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for having a look.
Yes, you're right about the need for a unit test in the test suite.
Here the intent of the "original" author (before c04abca6 broke it by importing the newer module) was to import the global
distutils.sysconfig._config_vars
dictionary, not to get the values, but to temporarily set them before callingdistutils.sysconfig.customize_compiler
. This is only for darwin, where building dynamic load libraries requires special compiler flags.The problem is the
customize_compiler
function uses the_config_vars
which are defined in thedistutils.sysconfig
module, not the one from the newsysconfig
module. Therefore, importing_CONFIG_VARS
from the latter, and modifying them, does not have any effect oncustomize_compiler
.I'm not sure about the relationship between
sysconfig._CONFIG_VARS
anddistutils.sysconfig._config_vars
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification. After re-reading the ticket, I see you also detailed much of this there. Sorry to ask you to repeat yourself.
An alternative to a unit test would be to have a function in build_ext which documents the intention, such that a subsequent change when reviewed would be clearly in violation of the stated intention.