regrtest: setup_tests() must not replace module.__path__ (_NamespacePath) with a simple list // importlib & abspath

[vstinner]

BPO	26538
Nosy	@brettcannon, @vstinner, @ericvsmith
Files	libregrtest_module_path.patch

^{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 2016-03-17.10:40:38.778>
created_at = <Date 2016-03-11.11:44:27.904>
labels = []
title = 'regrtest: setup_tests() must not replace module.__path__ (_NamespacePath) with a simple list // importlib & abspath'
updated_at = <Date 2016-03-17.10:40:38.777>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2016-03-17.10:40:38.777>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2016-03-17.10:40:38.778>
closer = 'vstinner'
components = []
creation = <Date 2016-03-11.11:44:27.904>
creator = 'vstinner'
dependencies = []
files = ['42130']
hgrepos = []
issue_num = 26538
keywords = ['patch']
message_count = 6.0
messages = ['261562', '261581', '261587', '261592', '261831', '261904']
nosy_count = 4.0
nosy_names = ['brett.cannon', 'vstinner', 'eric.smith', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue26538'
versions = ['Python 3.6']

[vstinner]

Extract of test.libregrtest.setup_tests():

    for module in sys.modules.values():
        if hasattr(module, '__path__'):
            module.__path__ = [os.path.abspath(path)
                               for path in module.__path__]
        if hasattr(module, '__file__'):
            module.__file__ = os.path.abspath(module.__file__)

Because of this code, it's not possible to store test files outside Lib/test/. For the issue bpo-26295 (test_regrtest), I would like to create a temporary directory and then a subdirectory test/ to create temporary test files.

Attached patch adds _NamespacePath.__setitem__() method and modify setup_tests() to keep the _NamespacePath type of module.__path__.

Maybe we should move this abspath() code somewhere in importlib. The site module already contains similar code, abs_paths() function:

    for m in set(sys.modules.values()):
        if (getattr(getattr(m, '__loader__', None), '__module__', None) not in
                ('_frozen_importlib', '_frozen_importlib_external')):
            continue   # don't mess with a PEP 302-supplied __file__
        try:
            m.__file__ = os.path.abspath(m.__file__)
        except (AttributeError, OSError):
            pass
        try:
            m.__cached__ = os.path.abspath(m.__cached__)
        except (AttributeError, OSError):
            pass

Since this code looks to depend on the implementation of importlib (the __loader__ test), IMHO it makes sense to move the code directly somewhere in importlib.

[brettcannon]

I think a more important question is why is test.libregrtest doing what it's doing? Is it simply to guarantee that the site code is run? If so it should just import site.

As for adding the __setitem__() method, I'm fine with it but maybe Eric has an opinion?

[vstinner]

Longer extract of setup_tests(), with the long comment:

    # Some times __path__ and __file__ are not absolute (e.g. while running from
    # Lib/) and, if we change the CWD to run the tests in a temporary dir, some
    # imports might fail.  This affects only the modules imported before os.chdir().
    # These modules are searched first in sys.path[0] (so '' -- the CWD) and if
    # they are found in the CWD their __file__ and __path__ will be relative (this
    # happens before the chdir).  All the modules imported after the chdir, are
    # not found in the CWD, and since the other paths in sys.path[1:] are absolute
    # (site.py absolutize them), the __file__ and __path__ will be absolute too.
    # Therefore it is necessary to absolutize manually the __file__ and __path__ of
    # the packages to prevent later imports to fail when the CWD is different.
    for module in sys.modules.values():
        if hasattr(module, '__path__'):
            module.__path__ = [os.path.abspath(path)
                               for path in module.__path__]
        if hasattr(module, '__file__'):
            module.__file__ = os.path.abspath(module.__file__)

[brettcannon]

Figures. If you do decide to centralize the code then put it in importlib.util and please don't make it public. It's something that should only be done once and not something most people should have to worry about.

[python-dev]

New changeset 96b73b649b15 by Victor Stinner in branch 'default':
regrtest: Fix module.__path__
https://hg.python.org/cpython/rev/96b73b649b15

[vstinner]

I pushed my minimum fix. I now understand that it is not needed to change importlib, it's ok to keep "abspath" code in site & libregrtest.