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 test errors when CXX is not set #79078

Closed
aixtools opened this issue Oct 4, 2018 · 8 comments
Closed

distutils test errors when CXX is not set #79078

aixtools opened this issue Oct 4, 2018 · 8 comments
Labels
3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@aixtools
Copy link
Contributor

aixtools commented Oct 4, 2018

BPO 34897
Nosy @ncoghlan, @taleinat, @merwok, @dstufft, @aixtools, @physicalist
PRs
  • bpo-34897: distutils test errors when CXX is not set #9706
  • bpo-11191: fix test_distutils for AIX with xlc #8709
  • bpo-11191: fix test_distutils for AIX with xlc #8709
  • 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 = None
    closed_at = <Date 2018-12-28.14:06:32.300>
    created_at = <Date 2018-10-04.20:22:26.034>
    labels = ['3.8', 'type-bug', 'library']
    title = 'distutils test errors when CXX is not set'
    updated_at = <Date 2021-10-14.13:06:04.192>
    user = 'https://github.com/aixtools'

    bugs.python.org fields:

    activity = <Date 2021-10-14.13:06:04.192>
    actor = 'physicalist'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-12-28.14:06:32.300>
    closer = 'ncoghlan'
    components = ['Distutils']
    creation = <Date 2018-10-04.20:22:26.034>
    creator = 'Michael.Felt'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34897
    keywords = ['patch']
    message_count = 8.0
    messages = ['327083', '328553', '328851', '329155', '329318', '329346', '332532', '403903']
    nosy_count = 6.0
    nosy_names = ['ncoghlan', 'taleinat', 'eric.araujo', 'dstufft', 'Michael.Felt', 'physicalist']
    pr_nums = ['9706', '8709', '8709']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue34897'
    versions = ['Python 3.8']

    @aixtools
    Copy link
    Contributor Author

    aixtools commented Oct 4, 2018

    while researching bpo-11191 I cam across 6 additional errors.

    There is a test in Lib/test/support/init.py

    def missing_compiler_executable(cmd_names=[]):
        """Check if the compiler components used to build the interpreter exist.
    Check for the existence of the compiler executables whose names are listed
    in 'cmd_names' or all the compiler executables when 'cmd_names' is empty
    and return the first missing executable or None when none is found
    missing.
    
    """
    from distutils import ccompiler, sysconfig, spawn
    compiler = ccompiler.new_compiler()
    sysconfig.customize_compiler(compiler)
    for name in compiler.executables:
        if cmd_names and name not in cmd_names:
            continue
        cmd = getattr(compiler, name)
        if cmd_names:
            assert cmd is not None, \
                    "the '%s' executable is not configured" % name
        elif cmd is None:
            continue
        if spawn.find_executable(cmd[0]) is None:
            return cmd[0]
    

    The "elif cmd is None:" is not successful because cmd maybe '' (null string)

    Initially I thought to change to
    "elif cmd is None or (not cmd):" but I hope I found a better resolution!

    In: Lib/distutils/sysconfig.py the final bits of customize_compiler is:

            compiler.set_executables(
                preprocessor=cpp,
                compiler=cc_cmd,
                compiler_so=cc_cmd + ' ' + ccshared,
                compiler_cxx=cxx,
                linker_so=ldshared,
                linker_exe=cc,
                archiver=archiver)

    the value for cxx come from os.environ, if set, otherwise it comes from get_sys_vars()

        (cc, cxx, opt, cflags, ccshared, ldshared, shlib_suffix, ar, ar_flags) = \
            get_config_vars('CC', 'CXX', 'OPT', 'CFLAGS',
                            'CCSHARED', 'LDSHARED', 'SHLIB_SUFFIX', 'AR', 'ARFLAGS')
    

    If, during the build, CXX was not set - this sets cxx to the null string ('').

    So the fix is to assign cxx = None when len(cxx) == 0

            if 'CXX' in os.environ:
                cxx = os.environ['CXX']
            if not len(cxx):
                cxx = None

    While this only seems to happen for cxx - maybe this should be extended to all the variables in (cc, cxx, opt, cflags, ccshared, ldshared, shlib_suffix, ar, ar_flags)?

    So, ultimately - I choose to go with changing : compiler.set_executables()

    diff --git a/Lib/distutils/ccompiler.py b/Lib/distutils/ccompiler.py
    index b71d1d39bc..2e08c4abd2 100644
    --- a/Lib/distutils/ccompiler.py
    +++ b/Lib/distutils/ccompiler.py
    @@ -148,7 +148,7 @@ class CCompiler:
                 if key not in self.executables:
                     raise ValueError("unknown executable '%s' for class %s" %
                           (key, self.__class__.__name__))
    -            self.set_executable(key, kwargs[key])
    +            self.set_executable(key, kwargs[key] if len(kwargs[key]) else None)
         def set_executable(self, key, value):
             if isinstance(value, str):

    Was:
    ======================================================================
    ERROR: test_run (distutils.tests.test_build_clib.BuildCLibTestCase)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/data/prj/python/git/python3-3.8/Lib/distutils/tests/test_build_clib.py", line 121, in test_run
        ccmd = missing_compiler_executable()
      File "/data/prj/python/git/python3-3.8/Lib/test/support/__init__.py", line 2730, in missing_compiler_executable
        if spawn.find_executable(cmd[0]) is None:
    IndexError: list index out of range

    ======================================================================
    ERROR: test_build_ext (distutils.tests.test_build_ext.BuildExtTestCase)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/data/prj/python/git/python3-3.8/Lib/distutils/tests/test_build_ext.py", line 62, in test_build_ext
        cmd = support.missing_compiler_executable()
      File "/data/prj/python/git/python3-3.8/Lib/test/support/__init__.py", line 2730, in missing_compiler_executable
        if spawn.find_executable(cmd[0]) is None:
    IndexError: list index out of range

    ======================================================================
    ERROR: test_get_outputs (distutils.tests.test_build_ext.BuildExtTestCase)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/data/prj/python/git/python3-3.8/Lib/distutils/tests/test_build_ext.py", line 308, in test_get_outputs
        cmd = support.missing_compiler_executable()
      File "/data/prj/python/git/python3-3.8/Lib/test/support/__init__.py", line 2730, in missing_compiler_executable
        if spawn.find_executable(cmd[0]) is None:
    IndexError: list index out of range

    ======================================================================
    ERROR: test_build_ext (distutils.tests.test_build_ext.ParallelBuildExtTestCase)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/data/prj/python/git/python3-3.8/Lib/distutils/tests/test_build_ext.py", line 62, in test_build_ext
        cmd = support.missing_compiler_executable()
      File "/data/prj/python/git/python3-3.8/Lib/test/support/__init__.py", line 2730, in missing_compiler_executable
        if spawn.find_executable(cmd[0]) is None:
    IndexError: list index out of range

    ======================================================================
    ERROR: test_get_outputs (distutils.tests.test_build_ext.ParallelBuildExtTestCase)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/data/prj/python/git/python3-3.8/Lib/distutils/tests/test_build_ext.py", line 308, in test_get_outputs
        cmd = support.missing_compiler_executable()
      File "/data/prj/python/git/python3-3.8/Lib/test/support/__init__.py", line 2730, in missing_compiler_executable
        if spawn.find_executable(cmd[0]) is None:
    IndexError: list index out of range

    ======================================================================
    ERROR: test_record_extensions (distutils.tests.test_install.InstallTestCase)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/data/prj/python/git/python3-3.8/Lib/distutils/tests/test_install.py", line 200, in test_record_extensions
        cmd = test_support.missing_compiler_executable()
      File "/data/prj/python/git/python3-3.8/Lib/test/support/__init__.py", line 2730, in missing_compiler_executable
        if spawn.find_executable(cmd[0]) is None:
    IndexError: list index out of range

    @aixtools aixtools added 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Oct 4, 2018
    @taleinat
    Copy link
    Contributor

    I'm not sure that the resolution currently suggested, changing compiler.set_executables(), is the right way to go.

    This change to distutils is a break of backwards compatibility. Though it is a minor change, it could still break existing code.

    Fixing test.support seems just as good to me in terms of code design, and better in that it is only used internally for our tests.

    (BTW, instead of `elif cmd is None or (not cmd):`, you can just use `elif not cmd:`.)

    @aixtools
    Copy link
    Contributor Author

    On 10/26/2018 5:36 PM, Tal Einat wrote:
    Tal Einat <taleinat@gmail.com> added the comment:

    I'm not sure that the resolution currently suggested, changing compiler.set_executables(), is the right way to go.

    This change to distutils is a break of backwards compatibility. Though it is a minor change, it could still break existing code.

    Fixing test.support seems just as good to me in terms of code design, and better in that it is only used internally for our tests.

    (BTW, instead of `elif cmd is None or (not cmd):`, you can just use `elif not cmd:`.)
    Thx. One of the python bits I need to embrace.

    Although - during my testing I saw that the null string "" was not being
    accepted as None, but was accepted as "not cmd".

    My reading error was taking None to mean a value was not initialized
    while it seems to be None is a 'value' that is assigned, while ´not xxx'
    can be either - never assigned or a null-length string.

    This is the "ambiguity", at least for myself, that I feel I tracked down.

    It is "unfortunate" if this breaks backward compatibility - IF the
    backward compatibility has been based on an ambiguity. Or I again, miss
    a fine-point of Python coding. Won´'t be the last time I expect.

    Comments requested.

    ----------
    nosy: +taleinat


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue34897\>


    @taleinat
    Copy link
    Contributor

    taleinat commented Nov 2, 2018

    not cmd will be true if cmd is None, so it is completely equivalent to cmd is None or not cmd. This is a purely stylistic change which doesn't alter the logic.

    To get a clear understanding of what's going on, I recommend reading the short "Truth Value Testing" section in the docs:
    https://docs.python.org/library/stdtypes.html#truth-value-testing

    @aixtools
    Copy link
    Contributor Author

    aixtools commented Nov 5, 2018

    Thx.

    So, while "" is not None (i.e., "" is not False), it does test as a Boolean expression as 'False' so the test for None or "" is the same as testing for "" (but not the same as testing for None).

    I accept your logic - and shall make the change in the test in Lib/test/support/init.py rather than in the assignment in Lib/distutils/ccompiler.py

    @taleinat
    Copy link
    Contributor

    taleinat commented Nov 6, 2018

    Michael, please read more about comparisons[1] and the None object[2].

    [1] https://docs.python.org/3/library/stdtypes.html#comparisons
    [2] https://docs.python.org/3/library/stdtypes.html#the-null-object

    @ncoghlan
    Copy link
    Contributor

    New changeset 259c159 by Nick Coghlan (Michael Felt) in branch 'master':
    bpo-34897: avoid distutils test error when CXX is not set (GH-9706)
    259c159

    @physicalist
    Copy link
    Mannequin

    physicalist mannequin commented Oct 14, 2021

    This issue also still affects Python versions 3.6.15 and 3.7.12

    IMHO it would make sense to backport this patch to the 3.6 and 3.7 branches, especially as it only affects one line of code and doesn't seem to affect anything else, but solves the same issue for prior python versions as the merged patch did for 3.8+

    @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
    3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants