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 tests fail if PATH is not defined #56629

Closed
henryprecheur mannequin opened this issue Jun 27, 2011 · 22 comments
Closed

distutils tests fail if PATH is not defined #56629

henryprecheur mannequin opened this issue Jun 27, 2011 · 22 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@henryprecheur
Copy link
Mannequin

henryprecheur mannequin commented Jun 27, 2011

BPO 12420
Nosy @terryjreedy, @ncoghlan, @tarekziade, @merwok, @zware, @zooba, @dstufft
Files
  • fix_empty_path.diff
  • fix_distutils_tests.diff
  • distutils-12420.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 = None
    closed_at = <Date 2017-06-18.18:05:59.084>
    created_at = <Date 2011-06-27.12:33:17.129>
    labels = ['type-bug', 'library']
    title = 'distutils tests fail if PATH is not defined'
    updated_at = <Date 2017-06-18.18:05:59.082>
    user = 'https://bugs.python.org/henryprecheur'

    bugs.python.org fields:

    activity = <Date 2017-06-18.18:05:59.082>
    actor = 'terry.reedy'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-06-18.18:05:59.084>
    closer = 'terry.reedy'
    components = ['Distutils']
    creation = <Date 2011-06-27.12:33:17.129>
    creator = 'henry.precheur'
    dependencies = []
    files = ['22492', '22504', '35798']
    hgrepos = []
    issue_num = 12420
    keywords = ['patch']
    message_count = 22.0
    messages = ['139261', '139273', '139277', '139280', '139282', '139297', '139301', '139318', '139342', '140131', '196748', '221752', '221773', '221775', '221776', '221802', '221813', '250601', '250613', '250615', '250621', '296282']
    nosy_count = 10.0
    nosy_names = ['terry.reedy', 'ncoghlan', 'tarek', 'eric.araujo', 'henry.precheur', 'alexis', 'BreamoreBoy', 'zach.ware', 'steve.dower', 'dstufft']
    pr_nums = []
    priority = 'normal'
    resolution = 'out of date'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue12420'
    versions = ['Python 2.7']

    @henryprecheur
    Copy link
    Mannequin Author

    henryprecheur mannequin commented Jun 27, 2011

    The function find_executable crashes if PATH is not defined.

    I admit that it's an extreme case, but it's probably better to on the safe side of things.

    What about using the current directory only if PATH is not defined? This seems to be a reasonable workaround.

    @henryprecheur henryprecheur mannequin added the stdlib Python modules in the Lib dir label Jun 27, 2011
    @merwok
    Copy link
    Member

    merwok commented Jun 27, 2011

    Thanks for the report. Can you tell how you ran into this? Did you call the function directly, or did you get the bug while running a setup.py command? Also, what do you mean by crash? We use that for CPython segmentation faults, not regular misbehavior. If you could attach the exact command or script that triggered the bug and the full traceback, it would help.

    I don’t think using the current directory is a good idea: it’s a security hazard.

    @merwok merwok assigned merwok and unassigned tarekziade Jun 27, 2011
    @merwok merwok added the type-bug An unexpected behavior, bug, or error label Jun 27, 2011
    @henryprecheur
    Copy link
    Mannequin Author

    henryprecheur mannequin commented Jun 27, 2011

    Sorry "crash" wasn't the right term. It's just that distutils tests fail.

    I ran into that when trying to run unit tests without any environment
    variable (see bpo-12401).

    $ env -i ./python ./Lib/test/regrtest.py test_distutils
    [1/1] test_distutils
    test test_distutils crashed -- Traceback (most recent call last):
      File "./Lib/test/regrtest.py", line 987, in runtest_inner
      File "/home/henry/code/cpython/Lib/test/test_distutils.py", line 13, in test_main
        test.support.run_unittest(distutils.tests.test_suite())
      File "/home/henry/code/cpython/Lib/distutils/tests/__init__.py", line 29, in test_suite
        __import__(modname)
      File "/home/henry/code/cpython/Lib/distutils/tests/test_archive_util.py", line 33, in <module>
        unittest.TestCase):
      File "/home/henry/code/cpython/Lib/distutils/tests/test_archive_util.py", line 96, in ArchiveUtilTestCase
        @unittest.skipUnless(find_executable('tar') and find_executable('gzip')
      File "/home/henry/code/cpython/Lib/distutils/spawn.py", line 154, in find_executable
        path = os.environ['PATH']
      File "/home/henry/code/cpython/Lib/os.py", line 450, in __getitem__
        value = self._data[self.encodekey(key)]
    KeyError: b'PATH'

    1 test failed:
    test_distutils
    [98227 refs]

    Maybe it's not really a problem, and having a system without PATH
    defined shouldn't be supported because it's too "weird".

    @henryprecheur
    Copy link
    Mannequin Author

    henryprecheur mannequin commented Jun 27, 2011

    I don't know exactly in which context find_executable should be used,
    but after taking a closer look it seems that returning None when PATH is
    not defined could "work".

    @merwok
    Copy link
    Member

    merwok commented Jun 27, 2011

    Okay, I see the original use case (bpo-12401). I think the proper thing to do is to skip tests that rely on the environment being non-empty.

    @henryprecheur
    Copy link
    Mannequin Author

    henryprecheur mannequin commented Jun 27, 2011

    I think that returning None would be a better option. The function
    documentation says:

    Tries to find 'executable' in the directories listed in 'path'.

    A string listing directories separated by 'os.pathsep'; defaults to
    os.environ['PATH']. Returns the complete filename or None if not found.

    If os.environ['PATH'] is empty the function returns None:

      >>> from distutils.spawn import find_executable
      >>> find_executable('does not exist', path='/bin:/usr/bin') is None
      True
      >>> find_executable('test', path='') is None
      True

    This would be consistent with the function definition. If PATH is
    undefined, the executable cannot be found, therefor returning None seems
    like the right thing to do.

    On Mon, Jun 27, 2011 at 02:33:09PM +0000, ??ric Araujo wrote:

    ??ric Araujo <merwok@netwok.org> added the comment:

    Okay, I see the original use case (bpo-12401). I think the proper thing to do is to skip tests that rely on the environment being non-empty.

    ----------


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue12420\>


    @henryprecheur henryprecheur mannequin changed the title distutils crashes if PATH is not defined distutils tests fail if PATH is not defined Jun 27, 2011
    @merwok
    Copy link
    Member

    merwok commented Jun 27, 2011

    To explain my position: distutils is a very brittle codebase that’s used and monkey-patched by a lot of third-party code. In the past, before the forking of distutils/packaging, Tarek tried to gradually improve distutils but he got a lot of pushback when his changes broke this third-party code that relied on known bugs or undocumented behavior or worked around it. That’s why a feature freeze is now in effect, and behavior is not changed unless it is to fix a bug.

    In this case, it is not documented that distutils should run under python -E, and nobody reported it as a bug before, that’s why I have the position that the tests that require $PATH should be skipped, and the code left untouched.

    For distutils2 (named packaging in the 3.3 standard library), we can improve find_executable. We even want to extract it and move it to shutil: bpo-444582

    @henryprecheur
    Copy link
    Mannequin Author

    henryprecheur mannequin commented Jun 27, 2011

    OK it makes sense.

    I'm working on fixing the tests. I'm not done yet, but I've attached a patch with my work in progress (some tests still fail). Do you have any comments?

    I'll post a complete version later today or tomorrow.

    @henryprecheur
    Copy link
    Mannequin Author

    henryprecheur mannequin commented Jun 28, 2011

    I've fixed the last failing tests, but I'm unsure it's the right way to do it.

    Take a look at the part for Lib/distutils/tests/test_build_ext.py. I just check that os.environ['PATH'] is defined. I'm not 100% certain that this will work on every platforms.

    @merwok
    Copy link
    Member

    merwok commented Jul 11, 2011

    Sorry, I don’t want to monkey-patch find_executable during tests. We want the tests to work with the real code, with the same behavior and errors as real setup.py calls. If a test does not work with python -E, then it should be skipped.

    (Implementation idea:
    skip_if_empty_env = unittest.skipUnless(
    os.environ, 'test cannot run with an empty environment')
    using sys.flags is not possible due to backward compatibility policy)

    @terryjreedy
    Copy link
    Member

    I just ran into this issue (same I believe) running the test suite with installed, versus repository, python on win7, but get a different traceback.

    Traceback (most recent call last):
     ...\lib\distutils\tests\test_build_ext.py"
       line 156, in test_optional_extension # or
       line 316, in test_get_outputs
        cmd.run)  # should raise an error
     ...\lib\unittest\case.py", line 570, in assertRaises
        return context.handle('assertRaises', callableObj, args, kwargs)
     ...\lib\unittest\case.py", line 135, in handle
        callable_obj(*args, **kwargs)
     ...\lib\distutils\command\build_ext.py", line 354, in run
        self.build_extensions()
     ...\lib\distutils\command\build_ext.py", line 463, in build_extensions
        self.build_extension(ext)
     ...\lib\distutils\command\build_ext.py", line 518, in build_extension
        depends=ext.depends)
     ...\lib\distutils\msvc9compiler.py", line 460, in compile
        self.initialize()
     ...\lib\distutils\msvc9compiler.py", line 371, in initialize
        vc_env = query_vcvarsall(VERSION, plat_spec)
     ...\lib\distutils\msvc9compiler.py", line 287, in query_vcvarsall
        raise ValueError(str(list(result.keys())))
    ValueError: ['path']

    I agree with Éric that the test should be skipped if it cannot go on.
    Wrapping
    self.assertRaises((UnknownFileError, CompileError),
    cmd.run) # should raise an error
    and
    cmd.run()
    with
    try:...except ValueError: <skip> would fix this issue on Windows. So would skipping if sys.platform == 'win32' and '+' not in sys.version (repository builds have a '+' and the test currently work for them). However, neither solution would work on *nix, which seems to have a completely different failure path to the same end result.

    Are distutils and test_distutils maintained at all? msvc9compiler.py (2005) says it also works with vc10 (2008, used for 2.7) but makes no mention of vc11 (2010, used for 3.3,3.4).

    Nick, what do you think we should do with this? I think that when python is installed and working as well as we expect, the test suite should pass, so any failures indicate a problem with the particular installation. I think this is especially important on Windows, with newbies who do not know distutils from, say, itertools.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jun 27, 2014

    Can someone follow up on this please. See also bpo-12401.

    @terryjreedy
    Copy link
    Member

    test_disutils still fails on both 2.7 and 3.4 installs. Since PATH is defined, that is not the issue here. I am tempted to unconditionally skip the test and close this issue.

    @ncoghlan
    Copy link
    Contributor

    For Python 3, I suggest tweaking the code to use shutil.which and see if that improves matters.

    For Python 2, I'm inclined not to worry about it.

    @ncoghlan
    Copy link
    Contributor

    Skipping if the compiler isn't available is problematic, since that could reflect a real failure mode for the code.

    @terryjreedy
    Copy link
    Member

    I feel that conscientious users who test their installations should get a clean test. They cannot be expected to know that this is an 'expected failure' and therefore not really a failure.

    Test_tools has the following, which indeed works to skip on installed 3.4.1 but not on built 3.4.1+.

    if not sysconfig.is_python_build():
        # XXX some installers do contain the tools, should we detect that
        # and run the tests in that case too?
        raise unittest.SkipTest('test irrelevant for an installed Python')

    How about we decorate the two failing tests
    line 156, in test_optional_extension # or
    line 316, in test_get_outputs
    with
    @unittest.skipUnless(sysconfig.is_python_build(),
    'test irrelevant for an installed Python') # or modify message
    ?

    @terryjreedy
    Copy link
    Member

    A minor change: distutils has is own version of sysconfig*, which is already imported into disutils/tests/test_build_ext.py. It has '_python_build' instead of 'is_python_build'. With that change, the decorators work as expected to skip the tests on installed Windows 3.4.1 either when test_build_ext is run by itself as main or as part of test_distutils.

    # Seems like a violation of DRY. According to hg revision history, (and visually comparing the two, some patches have been applied to just one, some to both.

    @terryjreedy
    Copy link
    Member

    The following failures on new 3.5.0 seems related as they all end with the same message about vcvarsall.bat (slightly different from 3.4 failures).

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

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

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

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

    ======================================================================
    ERROR: test_compiler_options (distutils.tests.test_msvccompiler.msvccompilerTestCase)
    ----------------------------------------------------------------------

    ======================================================================
    ERROR: test_vcruntime_copy (distutils.tests.test_msvccompiler.msvccompilerTestCase)
    ----------------------------------------------------------------------

    ======================================================================
    ERROR: test_vcruntime_skip_copy (distutils.tests.test_msvccompiler.msvccompilerTestCase)
    ----------------------------------------------------------------------

    ======================================================================
    FAIL: test_customize_compiler_before_get_config_vars (distutils.tests.test_sysconfig.SysconfigTestCase)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "C:\Programs\Python 3.5\lib\distutils\tests\test_sysconfig.py", line 197, in test_customize_compiler_before_get_c
    onfig_vars
        self.assertEqual(0, p.returncode, "Subprocess failed: " + outs)
    AssertionError: 0 != 1 : Subprocess failed: Traceback (most recent call last):
      File "@test_5504_tmp", line 5, in <module>
        rc = config.try_compile('int x;')
      File "C:\Programs\Python 3.5\lib\distutils\command\config.py", line 227, in try_compile
        self._compile(body, headers, include_dirs, lang)
      File "C:\Programs\Python 3.5\lib\distutils\command\config.py", line 133, in _compile
        self.compiler.compile([src], include_dirs=include_dirs)
      File "C:\Programs\Python 3.5\lib\distutils\_msvccompiler.py", line 315, in compile
        self.initialize()
      File "C:\Programs\Python 3.5\lib\distutils\_msvccompiler.py", line 208, in initialize
        vc_env = _get_vc_env(plat_spec)
      File "C:\Programs\Python 3.5\lib\distutils\_msvccompiler.py", line 83, in _get_vc_env
        raise DistutilsPlatformError("Unable to find vcvarsall.bat")
    distutils.errors.DistutilsPlatformError: Unable to find vcvarsall.bat

    Ran 234 tests in 1.108s

    FAILED (failures=1, errors=7, skipped=28)
    Warning -- threading._dangling was modified by test_distutils
    Warning -- files was modified by test_distutils

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Sep 14, 2015

    "Unable to find vcvarsall.bat" is raised when Visual Studio isn't installed or it's the incorrect version, so I don't believe these are the same at all.

    @zware
    Copy link
    Member

    zware commented Sep 14, 2015

    Agreed with Mark; I don't see anywhere that lack of PATH would affect the failures you're seeing, Terry. Do you have VS2015 installed? If not, the failure of distutils to skip the tests that need it should be raised as a separate issue.

    @terryjreedy
    Copy link
    Member

    No VS2015 yet. New issue bpo-25100. Thanks.

    @terryjreedy
    Copy link
    Member

    test_distutils now passes on 3.6, so this is out-of-date for 3.x.

    On installed 2.7.13 (released Dec 2016)
    ERROR: test_get_outputs (distutils.tests.test_build_ext.BuildExtTestCase)
    FAIL: test_customize_compiler_before_get_config_vars (distutils.tests.test_sysconfig.SysconfigTestCase)
    the error is ValueError: [u'path'] instead of ValueError: ['path'].

    Nick suggested leaving 2.7 alone, I don't care about it either, and I believe Victor Stinner has worked on its tests since last December.

    @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