Skip to content
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

distutils compiler switch ignored #50626

Closed
atuining mannequin opened this issue Jun 29, 2009 · 27 comments
Closed

distutils compiler switch ignored #50626

atuining mannequin opened this issue Jun 29, 2009 · 27 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@atuining
Copy link
Mannequin

atuining mannequin commented Jun 29, 2009

BPO 6377
Nosy @malemburg, @benjaminp, @tarekziade, @merwok, @zooba
Files
  • distutils_compiler.patch: Patch to fix --compiler switch
  • compiler.diff
  • 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 = 'https://github.com/tarekziade'
    closed_at = <Date 2021-02-03.18:26:27.973>
    created_at = <Date 2009-06-29.18:22:19.675>
    labels = ['type-bug', 'library']
    title = 'distutils compiler switch ignored'
    updated_at = <Date 2021-02-03.18:26:27.971>
    user = 'https://bugs.python.org/atuining'

    bugs.python.org fields:

    activity = <Date 2021-02-03.18:26:27.971>
    actor = 'steve.dower'
    assignee = 'tarek'
    closed = True
    closed_date = <Date 2021-02-03.18:26:27.973>
    closer = 'steve.dower'
    components = ['Distutils']
    creation = <Date 2009-06-29.18:22:19.675>
    creator = 'atuining'
    dependencies = []
    files = ['14390', '14455']
    hgrepos = []
    issue_num = 6377
    keywords = ['patch']
    message_count = 27.0
    messages = ['89871', '90045', '90154', '90155', '90156', '90176', '90179', '90274', '90276', '90277', '90278', '90279', '90280', '90281', '90284', '90289', '90299', '90309', '90313', '90315', '90316', '90334', '90382', '90383', '90385', '90439', '386387']
    nosy_count = 10.0
    nosy_names = ['lemburg', 'atuining', 'dalcinl', 'benjamin.peterson', 'tarek', 'nicdumz', 'eric.araujo', 'rpetrov', 'cmcqueen1975', 'steve.dower']
    pr_nums = []
    priority = 'high'
    resolution = 'out of date'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue6377'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @atuining
    Copy link
    Mannequin Author

    atuining mannequin commented Jun 29, 2009

    With the release of Python 3.1 the --compiler switch is ignored in
    Lib/distutils/command/build_ext.py. The attached patch fixes that issue.
    Once that was fixed there was another issue with get_version() in
    cygwincompiler but that appears to be fixed in the 3.1 branch. The
    revision that introduced this problem is 72596 (from 72593 in trunk).

    @atuining atuining mannequin assigned tarekziade Jun 29, 2009
    @atuining atuining mannequin added the stdlib Python modules in the Lib dir label Jun 29, 2009
    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Jul 3, 2009

    yes, the problem is that this option (string) is also used as an
    attribute (compiler object).

    But Python itself uses as an attribute..

    I have to check its type at this spot, and deprecate the usage of
    "compiler" as an attribute.

    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Jul 5, 2009

    Here's the patch. There's no simple way to deprecate the usage of
    "compiler" as an attribute, so I have just fixed Python setup.py.
    Using it as an attribute is just wrong.

    I have put Benjamin in the loop and I'll wait for his greenlight to
    commit this change.

    @benjaminp
    Copy link
    Contributor

    I think the patch is fine. (Did you try using the descriptor protocol to
    add a deprecation warning?)

    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Jul 5, 2009

    Right, turning compiler into a property and adding a warning on the set
    would be good enough I guess. Then turn it back into a simple string for
    python 3.3 and ... 2.8 :)

    I'll add that

    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Jul 6, 2009

    done in r73864, waiting for the buildbots to build trunk, then will be
    applied in 3.x

    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Jul 6, 2009

    merged in r73866 in py3k

    Thanks for the feedback

    @tarekziade tarekziade mannequin closed this as completed Jul 6, 2009
    @exarkun
    Copy link
    Mannequin

    exarkun mannequin commented Jul 8, 2009

    It looks like this change may have broken some parts of distutils. For
    example, Twisted's setup.py now produces this output when running the
    build_ext command:

    $ ~/Projects/python/trunk/python setup.py build_ext
    running build_ext
    Traceback (most recent call last):
      File "setup.py", line 97, in <module>
        main(sys.argv[1:])
      File "setup.py", line 92, in main
        setup(**setup_args)
      File "./twisted/python/dist.py", line 47, in setup
        return core.setup(**get_setup_args(**kw))
      File "/home/exarkun/Projects/python/trunk/Lib/distutils/core.py", line
    149, in setup
        dist.run_commands()
      File "/home/exarkun/Projects/python/trunk/Lib/distutils/dist.py", line
    926, in run_commands
        self.run_command(cmd)
      File "/home/exarkun/Projects/python/trunk/Lib/distutils/dist.py", line
    945, in run_command
        cmd_obj.run()
      File
    "/home/exarkun/Projects/python/trunk/Lib/distutils/command/build_ext.py", line
    380, in run
        self.build_extensions()
      File "./twisted/python/dist.py", line 327, in build_extensions
        self.prepare_extensions()
      File "./twisted/python/dist.py", line 318, in prepare_extensions
        if x.condition(self)]
      File "twisted/runner/topfiles/setup.py", line 14, in <lambda>
        condition=lambda builder: builder._check_header("rpc/rpc.h")),
      File "./twisted/python/dist.py", line 359, in _check_header
        self.compiler.announce("checking for %s ..." % header_name, 0)
    AttributeError: 'NoneType' object has no attribute 'announce'

    And PyCrypto produces this output:

    running build_ext
    Traceback (most recent call last):
      File "setup.py", line 163, in <module>
        core.setup(**kw)
      File "/tmp/python-buildbot/local/lib/python2.7/distutils/core.py",
    line 149, in setup
        dist.run_commands()
      File "/tmp/python-buildbot/local/lib/python2.7/distutils/dist.py",
    line 926, in run_commands
        self.run_command(cmd)
      File "/tmp/python-buildbot/local/lib/python2.7/distutils/dist.py",
    line 945, in run_command
        cmd_obj.run()
      File
    "/tmp/python-buildbot/local/lib/python2.7/distutils/command/install.py",
    line 580, in run
        self.run_command('build')
      File "/tmp/python-buildbot/local/lib/python2.7/distutils/cmd.py", line
    326, in run_command
        self.distribution.run_command(command)
      File "/tmp/python-buildbot/local/lib/python2.7/distutils/dist.py",
    line 945, in run_command
        cmd_obj.run()
      File
    "/tmp/python-buildbot/local/lib/python2.7/distutils/command/build.py",
    line 132, in run
        self.run_command(cmd_name)
      File "/tmp/python-buildbot/local/lib/python2.7/distutils/cmd.py", line
    326, in run_command
        self.distribution.run_command(command)
      File "/tmp/python-buildbot/local/lib/python2.7/distutils/dist.py",
    line 945, in run_command
        cmd_obj.run()
      File
    "/tmp/python-buildbot/local/lib/python2.7/distutils/command/build_ext.py",
    line 380, in run
        self.build_extensions()
      File "setup.py", line 115, in build_extensions
        self.detect_modules()
      File "setup.py", line 119, in detect_modules
        lib_dirs = self.compiler.library_dirs + ['/lib', '/usr/lib']
    AttributeError: 'NoneType' object has no attribute 'library_dirs'

    @malemburg
    Copy link
    Member

    Tarek, the .compiler attribute is needed by bdist_ext, so cannot just be
    removed or renamed to .compiler_obj. There's a lot of bdist_ext
    distutils code out there relying on having the .compiler object available.

    A much better fix would be to map the option to a new attribute in
    build_ext, e.g. .compiler_name or to remove the option altogether and
    always inherit the setting from the global dist option --compiler.

    In general, when making such changes, please test these against a few
    well-known packages such as twisted, pycrypto or egenix-mx-base which do
    include C extensions.

    Thanks.

    @malemburg
    Copy link
    Member

    Note that the config command also uses a .compiler instance for actually
    doing work, rather than as command option.

    @malemburg
    Copy link
    Member

    Added Python 2.7 since it fails there as well.

    @malemburg
    Copy link
    Member

    Marc-Andre Lemburg wrote:

    Tarek, the .compiler attribute is needed by bdist_ext, so cannot just be
    removed or renamed to .compiler_obj. There's a lot of bdist_ext
    distutils code out there relying on having the .compiler object available.

    Sorry, the above should have read build_ext, not bdist_ext...

    A much better fix would be to map the option to a new attribute in
    build_ext, e.g. .compiler_name or to remove the option altogether and
    always inherit the setting from the global dist option --compiler.

    @rpetrov
    Copy link
    Mannequin

    rpetrov mannequin commented Jul 8, 2009

    Trunk may be is not affected. I successfully cross-compile with GNU
    compiler for windows (see bpo-3871).

    @malemburg
    Copy link
    Member

    Roumen Petrov wrote:

    Roumen Petrov <bugtrack@roumenpetrov.info> added the comment:

    Trunk may be is not affected. I successfully cross-compile with GNU
    compiler for windows (see bpo-3871).

    It is affected in the sense that .compile was changed
    to .compile_obj and that change breaks 3rd party setup.py
    code which uses the compiler instance to run its own
    compilations or needs to tweak the setup by e.g. adding
    more libs or dirs to its configuration.

    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Jul 8, 2009

    I'll set back the compiler attribute when compiler_obj is set too,
    so third-party code will be able to work with it as before.

    The current code will deprecate this usage, by displaying a deprecation
    warning:

    • if the compiler is set to anything else than a string.
    • if the compiler is get and happens to be a compiler instance.

    so we can keep "compiler" as its initial purpose.

    And I'll test it on twisted and egenix. Thanks for the feedback

    @tarekziade tarekziade mannequin reopened this Jul 8, 2009
    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Jul 8, 2009

    done in r73895, r73896. (and tested with twisted trunk).

    @exarkun
    Copy link
    Mannequin

    exarkun mannequin commented Jul 9, 2009

    Cool, thanks. PyCrypto also works again now.

    @malemburg
    Copy link
    Member

    Tarek Ziadé wrote:

    Tarek Ziadé <ziade.tarek@gmail.com> added the comment:

    I'll set back the compiler attribute when compiler_obj is set too,
    so third-party code will be able to work with it as before.

    The current code will deprecate this usage, by displaying a deprecation
    warning:

    • if the compiler is set to anything else than a string.
    • if the compiler is get and happens to be a compiler instance.

    so we can keep "compiler" as its initial purpose.

    Could you please elaborate a bit on the reasoning behind
    deprecating using .compiler for the compiler instance ?

    The code did work before, so I'm not sure why you are trying
    to fix something that wasn't really broken.

    With the change you:

    • make the code more complex just to be able to raise
      a warning

    • introduce an cross-version incompatibility for tools
      using build_ext: they will now have to use .compiler
      for Python 2.3-2.6 and .compiler_obj for 2.7 and up

    Wouldn't it be better to either leave things are they have
    been for years (without problems) or find another solution ?

    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Jul 9, 2009

    The build_ext command cannot be run twice, because the first time, the
    "compiler" option may be set to "unix" for example, or left to None, and
    then is transformed into a compiler object. That's the bug.

    If you call it again, it'll break because the new_compiler() function
    will receive self.compiler, which is supposed to be a string not a
    compiler object.

    "compiler" is described as the "compiler type" in the options list of
    build_ext and should receive a string value.

    So what's broken is the fact that third-party code is using "compiler"
    as a compiler object attribute for years, because the command was
    creating the compiler object on the compiler option, rather than using
    its own attribute for that. And from an architectural point of view, if
    you have to tweak the compiler options by tweaking them direclty, it
    means that the build_ext command did a bad job with in the options
    it provides.

    For the cross-version incompatibility you are mentioning, it means that
    your code is working with "compiler" as a string option, then continue
    to work with it as a compiler object right after the command is run.
    But either way, there will be an incompatibility starting at 3.3 because
    we are going from the state "compiler is a string and also a compiler
    object, depending on when you use it" to "compiler is a only string option"

    This is an inconsistent behavior I am fixing here. While the code may be
    more complex with the descriptor, this will eventually go away in 3.3
    (and 2.8 if it exists).

    What is the other solution you where thinking about ? adding a
    compiler_type option and keep the compiler option woud introduce more
    incompatibilities since "compiler" is used to configure build_ext by
    many code out there, while tweaking the compiler object itself is an
    advanced usage done by less people, which may do the same thing in an
    easy way by using another attribute.

    @malemburg
    Copy link
    Member

    Tarek Ziadé wrote:

    Tarek Ziadé <ziade.tarek@gmail.com> added the comment:

    The build_ext command cannot be run twice, because the first time, the
    "compiler" option may be set to "unix" for example, or left to None, and
    then is transformed into a compiler object. That's the bug.

    If you call it again, it'll break because the new_compiler() function
    will receive self.compiler, which is supposed to be a string not a
    compiler object.

    You never run a command twice unless you explicitly reinitialize it
    (which then resets .compiler to None and then fetches the command line
    option again), so the above is not a problem.

    "compiler" is described as the "compiler type" in the options list of
    build_ext and should receive a string value.

    So what's broken is the fact that third-party code is using "compiler"
    as a compiler object attribute for years, because the command was
    creating the compiler object on the compiler option, rather than using
    its own attribute for that. And from an architectural point of view, if
    you have to tweak the compiler options by tweaking them direclty, it
    means that the build_ext command did a bad job with in the options
    it provides.

    I agree that it's not exactly ideal to have an attribute first
    be a string and then an object.

    For the cross-version incompatibility you are mentioning, it means that
    your code is working with "compiler" as a string option, then continue
    to work with it as a compiler object right after the command is run.

    Not quite: extensions of build_ext will likely customize the
    way the extension objects are built, ie. override .build_extensions()
    which is called after .compiler has been set to the compiler instance.

    So they always work with the compiler instance. And they don't really care
    about the option at all, since all that information is available from
    looking at the compiler object.

    But either way, there will be an incompatibility starting at 3.3 because
    we are going from the state "compiler is a string and also a compiler
    object, depending on when you use it" to "compiler is a only string option"

    This is an inconsistent behavior I am fixing here. While the code may be
    more complex with the descriptor, this will eventually go away in 3.3
    (and 2.8 if it exists).

    What is the other solution you where thinking about ? adding a
    compiler_type option and keep the compiler option woud introduce more
    incompatibilities since "compiler" is used to configure build_ext by
    many code out there, while tweaking the compiler object itself is an
    advanced usage done by less people, which may do the same thing in an
    easy way by using another attribute.

    First of all, you only use a single compiler for building an
    Python package, so using the global build compiler option will do
    just fine (and is also required if you have other commands
    rely on this information as well, such as the config command,
    the build_clib command or other custom commands).

    The "compiler" option on the build_ext and config commands
    are not really needed. Their .finalize_options() methods could
    easily pull in the build option value and place it into
    an .compiler_type attribute which then gets used as basis for
    creating the .compiler instance, or even better refactor the
    various commands to use a central method on the build
    command object to create a compiler object and avoid all
    the copy&paste code for this.

    Furthermore, the .finalize_options() methods could detect whether
    a per-command option as used and deprecate this use instead,
    redirecting to the build command option.

    Please note that it's common practice in distutils to have
    the compiler instance in an .compiler attribute, so either
    you change it in all cases or not at all.

    The fact that the options on some of those commands is
    named --compiler as well, is rather unfortunate.

    Regarding wide-spread use of the compiler command line option:
    I am only aware of the cygwin/mingw32 case where you'd really
    need it. In all other cases I know, the default choice based
    on the compiler with which Python itself was compiled will
    work fine.

    @malemburg
    Copy link
    Member

    FWIW: I've changed our mxSetup code to use a method for accessing
    the compiler instance.

    Perhaps that's the better way to go for standard distutils commands
    as well ?! E.g. .get_compiler_object()

    @nicdumz
    Copy link
    Mannequin

    nicdumz mannequin commented Jul 9, 2009

    It seems that the fix is still not perfect.

    At the moment ( r73906 ), if you try to build trunk using Python 2.6,
    you get:

    > python setup.py build
    running build
    running build_ext
    Traceback (most recent call last):
      File "setup.py", line 1901, in <module>
        main()
      File "setup.py", line 1896, in main
        'Lib/smtpd.py']
      File "/usr/lib/python2.6/distutils/core.py", line 152, in setup
        dist.run_commands()
      File "/usr/lib/python2.6/distutils/dist.py", line 975, in run_commands
        self.run_command(cmd)
      File "/usr/lib/python2.6/distutils/dist.py", line 995, in run_command
        cmd_obj.run()
      File "/usr/lib/python2.6/distutils/command/build.py", line 135, in run
        self.run_command(cmd_name)
      File "/usr/lib/python2.6/distutils/cmd.py", line 333, in run_command
        self.distribution.run_command(command)
      File "/usr/lib/python2.6/distutils/dist.py", line 995, in run_command
        cmd_obj.run()
      File "/usr/lib/python2.6/distutils/command/build_ext.py", line 345, in run
        self.build_extensions()
      File "setup.py", line 103, in build_extensions
        missing = self.detect_modules()
      File "setup.py", line 302, in detect_modules
        add_dir_to_list(self.compiler_obj.library_dirs, '/usr/local/lib')
      File "/usr/lib/python2.6/distutils/cmd.py", line 112, in __getattr__
        raise AttributeError, attr
    AttributeError: compiler_obj

    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Jul 10, 2009

    @nicolas : That's because you run it with Python 2.6 distutils, which
    doesn't have that change.

    If you want to build the current trunk with Python 2.6, you may want to
    install a standalone version of distutils. I have a nightly build of the
    trunk you may install in 2.6 for this:

    http://nightly.ziade.org/

    By the time 2.7 comes out, I'll probably publish an official standalone
    version for 2.6 as Marc André suggested in the past.

    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Jul 10, 2009

    You never run a command twice unless you explicitly reinitialize it
    (which then resets .compiler to None and then fetches the command line
    option again), so the above is not a problem.

    In practice yes that's true.

    The "compiler" option on the build_ext and config commands
    are not really needed. Their .finalize_options() methods could
    easily pull in the build option value and place it into
    an .compiler_type attribute which then gets used as basis for
    creating the .compiler instance, or even better refactor the
    various commands to use a central method on the build
    command object to create a compiler object and avoid all
    the copy&paste code for this.
    Furthermore, the .finalize_options() methods could detect whether
    a per-command option as used and deprecate this use instead,
    redirecting to the build command option.

    Having a single location sounds like the best idea with the current
    behavior, reachable from cmd.get_compiler_object() like you did.

    Now the question is, in practice, could someone force a
    different compiler in build_ext for instance, or with a different
    configuration than in build_clib ?

    I don't see a use case in practice for that, but if so, we would need to
    keep a different compiler instance per command.

    @malemburg
    Copy link
    Member

    Tarek Ziadé wrote:

    > The "compiler" option on the build_ext and config commands
    > are not really needed. Their .finalize_options() methods could
    > easily pull in the build option value and place it into
    > an .compiler_type attribute which then gets used as basis for
    > creating the .compiler instance, or even better refactor the
    > various commands to use a central method on the build
    > command object to create a compiler object and avoid all
    > the copy&paste code for this.
    > Furthermore, the .finalize_options() methods could detect whether
    > a per-command option as used and deprecate this use instead,
    > redirecting to the build command option.

    Having a single location sounds like the best idea with the current
    behavior, reachable from cmd.get_compiler_object() like you did.

    Now the question is, in practice, could someone force a
    different compiler in build_ext for instance, or with a different
    configuration than in build_clib ?

    I don't see a use case in practice for that, but if so, we would need to
    keep a different compiler instance per command.

    It is generally a bad idea to mix compiler types when compiling
    Python extensions and usually doesn't work unless you are
    very careful.

    In practice, I don't think that any extension package will use
    more than one compiler type for the various build parts.

    However, it is well possible that a package may use differently
    setup compiler instances on various commands, e.g. to point
    it to different libraries, dirs, etc., so having just
    one such instance on e.g. the top-level build command object
    would not work out.

    In fact, a single command object may even use multiple compiler
    instances to e.g. build different C libs or extensions using
    different sets of include and library search dirs.

    I think a workable solution to the problem with the compiler
    option would be to remove the option from the build_ext,
    build_clib and config commands (plus any others, if there are
    more) and only allow it on the build command.

    The build.get_compiler_object() could then return an instance
    of the chosen compiler type and the command object would then
    store it in .compiler.

    The various .finalize_options() method would need to propagate
    any used "compiler" option back to the build command and reset
    the .compiler attribute to None.

    You'd still have the situation that .compiler is used as
    option string and then as compiler instance, but only for
    the short phase between .initialize_options() and
    .finalize_options() which is not all that much of a problem
    (you cannot make any use of non-finalized command objects
    anyway).

    @tarekziade
    Copy link
    Mannequin

    tarekziade mannequin commented Jul 12, 2009

    I think a workable solution to the problem with the compiler
    option would be to remove the option from the build_ext,
    build_clib and config commands (plus any others, if there are
    more) and only allow it on the build command.

    The problem I see is that sometimes, people are using the build_ext
    command on its own:

      $ python setup.py build_ext -i --compiler mingw32

    So that means build_ext is not a subcommand of build in this case.
    While we can instanciate a build command on the fly to get our compiler
    and work with it, using get_command_obj().

    But the code will have to be careful and not use the cache with this
    method e.g. to re-create a new build command to avoid using the same
    command instance if another build_ is called.

    What about making it all simpler, by creating a base command class that
    manages a compiler through some methods (like get_compiler_obj), then
    make all our build+build_* command inherit from it ?

    That would resolve the code duplication and will make it simpler each
    command to deal with a compiler.

    This base command would have one single option. e.g. "compiler"

    You'd still have the situation that .compiler is used as
    option string and then as compiler instance, but only for
    the short phase between .initialize_options() and
    .finalize_options() which is not all that much of a problem

    I am not sure to understand how .compiler will become a string
    again after .finalize_options() has been called. Could you provide
    an example ?

    @bitdancer bitdancer added the type-bug An unexpected behavior, bug, or error label Dec 14, 2010
    @zooba
    Copy link
    Member

    zooba commented Feb 3, 2021

    Distutils is now deprecated (see PEP-632) and all tagged issues are being closed. From now until removal, only release blocking issues will be considered for distutils.

    If this issue does not relate to distutils, please remove the component and reopen it. If you believe it still requires a fix, most likely the issue should be re-reported at https://github.com/pypa/setuptools

    @zooba zooba closed this as completed Feb 3, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants