classification
Title: regrtest: setup_tests() must not replace module.__path__ (_NamespacePath) with a simple list // importlib & abspath
Type: Stage:
Components: Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, eric.smith, python-dev, vstinner
Priority: normal Keywords: patch

Created on 2016-03-11 11:44 by vstinner, last changed 2016-03-17 10:40 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
libregrtest_module_path.patch vstinner, 2016-03-11 11:44
Messages (6)
msg261562 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-03-11 11:44
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 #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.
msg261581 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-03-11 16:56
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?
msg261587 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-03-11 17:55
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__)
msg261592 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-03-11 19:21
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.
msg261831 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-03-15 22:47
New changeset 96b73b649b15 by Victor Stinner in branch 'default':
regrtest: Fix module.__path__
https://hg.python.org/cpython/rev/96b73b649b15
msg261904 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-03-17 10:40
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.
History
Date User Action Args
2016-03-17 10:40:38vstinnersetstatus: open -> closed
resolution: fixed
2016-03-17 10:40:28vstinnersetmessages: + msg261904
2016-03-15 22:47:30python-devsetnosy: + python-dev
messages: + msg261831
2016-03-11 19:21:07brett.cannonsetmessages: + msg261592
2016-03-11 17:55:49vstinnersetmessages: + msg261587
2016-03-11 16:56:12brett.cannonsetnosy: + eric.smith
messages: + msg261581
2016-03-11 11:44:27vstinnercreate