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

Merged
merged 2 commits into from May 19, 2016

Conversation

Projects
None yet
2 participants
@anthrotype
Contributor

anthrotype commented May 4, 2016

otherwise distutils.sysconfig.customize_compiler does not configure the OS X compiler for -dynamiclib

See #571

build_ext: always import _CONFIG_VARS from distutils.sysconfig instea…
…d of sysconfig

otherwise `distutils.sysconfig.customize_compiler` does not configure OSX compiler for -dynamiclib

See #571
@@ -16,15 +16,11 @@
except ImportError:
_build_ext = _du_build_ext
try:
# Python 2.7 or >=3.2

This comment has been minimized.

@jaraco

jaraco May 6, 2016

Member

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 between sysconfig._CONFIG_VARS and distutils.sysconfig._config_vars?

This comment has been minimized.

@anthrotype

anthrotype May 6, 2016

Contributor

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 c04abca 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 calling distutils.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 the distutils.sysconfig module, not the one from the new sysconfig module. Therefore, importing _CONFIG_VARS from the latter, and modifying them, does not have any effect on customize_compiler.

I'm not sure about the relationship between sysconfig._CONFIG_VARS and distutils.sysconfig._config_vars.

This comment has been minimized.

@jaraco

jaraco May 6, 2016

Member

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.

@anthrotype

This comment has been minimized.

Contributor

anthrotype commented May 18, 2016

I moved the block customizing the compiler for dylib to a separate function, preceded with an underscore to make it private. I also added a comment explaining the intention.
Please take a look and let me know if you require further changes.
Thank you.

try:
# XXX Help! I don't have any idea whether these are right...
_CONFIG_VARS['LDSHARED'] = (
"gcc -Wl,-x -dynamiclib -undefined dynamic_lookup")

This comment has been minimized.

@anthrotype

anthrotype May 18, 2016

Contributor

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 -Wl,-x flags so that clang can compile a C++ library I'm working on, otherwise it throws a cryptic error like this:

ld: internal error: atom not found in symbolIndex(__ZNKSt3__115basic_stringbufIcNS_11char_traitsIcEENS_9allocatorIcEEE3strEv) for architecture x86_64`

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 -dynamiclib flag which is passed here via _CONFIG_VARS['CCSHARED']:

clang: warning: argument unused during compilation: '-dynamiclib'

Hopefully some else in the setuptools community knows more about how to compile dynamic libraries for OS X.

@jaraco

This comment has been minimized.

Member

jaraco commented May 19, 2016

Sounds good to me. Let's get it out and see how it fares.

@jaraco jaraco merged commit ccaead8 into pypa:master May 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jaraco added a commit that referenced this pull request May 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment