Skip to content

bpo-43105: Importlib now resolves relative paths when creating module spec objects from file locations #25121

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

Merged
merged 5 commits into from
Apr 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 62 additions & 19 deletions Lib/importlib/_bootstrap_external.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
# Assumption made in _path_join()
assert all(len(sep) == 1 for sep in path_separators)
path_sep = path_separators[0]
path_sep_tuple = tuple(path_separators)
path_separators = ''.join(path_separators)
_pathseps_with_colon = {f':{s}' for s in path_separators}

Expand Down Expand Up @@ -91,22 +92,49 @@ def _unpack_uint16(data):
return int.from_bytes(data, 'little')


def _path_join(*path_parts):
"""Replacement for os.path.join()."""
return path_sep.join([part.rstrip(path_separators)
for part in path_parts if part])
if _MS_WINDOWS:
def _path_join(*path_parts):
"""Replacement for os.path.join()."""
if not path_parts:
return ""
if len(path_parts) == 1:
return path_parts[0]
root = ""
path = []
for new_root, tail in map(_os._path_splitroot, path_parts):
if new_root.startswith(path_sep_tuple) or new_root.endswith(path_sep_tuple):
root = new_root.rstrip(path_separators) or root
path = [path_sep + tail]
elif new_root.endswith(':'):
if root.casefold() != new_root.casefold():
# Drive relative paths have to be resolved by the OS, so we reset the
# tail but do not add a path_sep prefix.
root = new_root
path = [tail]
else:
path.append(tail)
else:
root = new_root or root
path.append(tail)
path = [p.rstrip(path_separators) for p in path if p]
if len(path) == 1 and not path[0]:
# Avoid losing the root's trailing separator when joining with nothing
return root + path_sep
return root + path_sep.join(path)

else:
def _path_join(*path_parts):
"""Replacement for os.path.join()."""
return path_sep.join([part.rstrip(path_separators)
for part in path_parts if part])


def _path_split(path):
"""Replacement for os.path.split()."""
if len(path_separators) == 1:
front, _, tail = path.rpartition(path_sep)
return front, tail
for x in reversed(path):
if x in path_separators:
front, tail = path.rsplit(x, maxsplit=1)
return front, tail
return '', path
i = max(path.rfind(p) for p in path_separators)
if i < 0:
return '', path
return path[:i], path[i + 1:]


def _path_stat(path):
Expand Down Expand Up @@ -140,13 +168,18 @@ def _path_isdir(path):
return _path_is_mode_type(path, 0o040000)


def _path_isabs(path):
"""Replacement for os.path.isabs.
if _MS_WINDOWS:
def _path_isabs(path):
"""Replacement for os.path.isabs."""
if not path:
return False
root = _os._path_splitroot(path)[0].replace('/', '\\')
return len(root) > 1 and (root.startswith('\\\\') or root.endswith('\\'))

Considers a Windows drive-relative path (no drive, but starts with slash) to
still be "absolute".
"""
return path.startswith(path_separators) or path[1:3] in _pathseps_with_colon
else:
def _path_isabs(path):
"""Replacement for os.path.isabs."""
return path.startswith(path_separators)


def _write_atomic(path, data, mode=0o666):
Expand Down Expand Up @@ -707,6 +740,11 @@ def spec_from_file_location(name, location=None, *, loader=None,
pass
else:
location = _os.fspath(location)
if not _path_isabs(location):
try:
location = _path_join(_os.getcwd(), location)
except OSError:
pass

# If the location is on the filesystem, but doesn't actually exist,
# we could return None here, indicating that the location is not
Expand Down Expand Up @@ -1451,6 +1489,8 @@ def __init__(self, path, *loader_details):
self._loaders = loaders
# Base (directory) path
self.path = path or '.'
if not _path_isabs(self.path):
self.path = _path_join(_os.getcwd(), self.path)
self._path_mtime = -1
self._path_cache = set()
self._relaxed_path_cache = set()
Expand Down Expand Up @@ -1516,7 +1556,10 @@ def find_spec(self, fullname, target=None):
is_namespace = _path_isdir(base_path)
# Check for a file w/ a proper suffix exists.
for suffix, loader_class in self._loaders:
full_path = _path_join(self.path, tail_module + suffix)
try:
full_path = _path_join(self.path, tail_module + suffix)
except ValueError:
return None
_bootstrap._verbose_message('trying {}', full_path, verbosity=2)
if cache_module + suffix in cache:
if _path_isfile(full_path):
Expand Down
14 changes: 7 additions & 7 deletions Lib/test/test_import/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -906,15 +906,15 @@ def test_missing_source_legacy(self):
m = __import__(TESTFN)
try:
self.assertEqual(m.__file__,
os.path.join(os.curdir, os.path.relpath(pyc_file)))
os.path.join(os.getcwd(), os.curdir, os.path.relpath(pyc_file)))
finally:
os.remove(pyc_file)

def test___cached__(self):
# Modules now also have an __cached__ that points to the pyc file.
m = __import__(TESTFN)
pyc_file = importlib.util.cache_from_source(TESTFN + '.py')
self.assertEqual(m.__cached__, os.path.join(os.curdir, pyc_file))
self.assertEqual(m.__cached__, os.path.join(os.getcwd(), os.curdir, pyc_file))

@skip_if_dont_write_bytecode
def test___cached___legacy_pyc(self):
Expand All @@ -930,7 +930,7 @@ def test___cached___legacy_pyc(self):
importlib.invalidate_caches()
m = __import__(TESTFN)
self.assertEqual(m.__cached__,
os.path.join(os.curdir, os.path.relpath(pyc_file)))
os.path.join(os.getcwd(), os.curdir, os.path.relpath(pyc_file)))

@skip_if_dont_write_bytecode
def test_package___cached__(self):
Expand All @@ -950,10 +950,10 @@ def cleanup():
m = __import__('pep3147.foo')
init_pyc = importlib.util.cache_from_source(
os.path.join('pep3147', '__init__.py'))
self.assertEqual(m.__cached__, os.path.join(os.curdir, init_pyc))
self.assertEqual(m.__cached__, os.path.join(os.getcwd(), os.curdir, init_pyc))
foo_pyc = importlib.util.cache_from_source(os.path.join('pep3147', 'foo.py'))
self.assertEqual(sys.modules['pep3147.foo'].__cached__,
os.path.join(os.curdir, foo_pyc))
os.path.join(os.getcwd(), os.curdir, foo_pyc))

def test_package___cached___from_pyc(self):
# Like test___cached__ but ensuring __cached__ when imported from a
Expand All @@ -977,10 +977,10 @@ def cleanup():
m = __import__('pep3147.foo')
init_pyc = importlib.util.cache_from_source(
os.path.join('pep3147', '__init__.py'))
self.assertEqual(m.__cached__, os.path.join(os.curdir, init_pyc))
self.assertEqual(m.__cached__, os.path.join(os.getcwd(), os.curdir, init_pyc))
foo_pyc = importlib.util.cache_from_source(os.path.join('pep3147', 'foo.py'))
self.assertEqual(sys.modules['pep3147.foo'].__cached__,
os.path.join(os.curdir, foo_pyc))
os.path.join(os.getcwd(), os.curdir, foo_pyc))

def test_recompute_pyc_same_second(self):
# Even when the source file doesn't change timestamp, a change in
Expand Down
19 changes: 15 additions & 4 deletions Lib/test/test_importlib/test_spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ class FactoryTests:

def setUp(self):
self.name = 'spam'
self.path = 'spam.py'
self.path = os.path.abspath('spam.py')
self.cached = self.util.cache_from_source(self.path)
self.loader = TestLoader()
self.fileloader = TestLoader(self.path)
Expand Down Expand Up @@ -645,7 +645,7 @@ def test_spec_from_loader_is_package_true_with_fileloader(self):
self.assertEqual(spec.loader, self.fileloader)
self.assertEqual(spec.origin, self.path)
self.assertIs(spec.loader_state, None)
self.assertEqual(spec.submodule_search_locations, [''])
self.assertEqual(spec.submodule_search_locations, [os.getcwd()])
self.assertEqual(spec.cached, self.cached)
self.assertTrue(spec.has_location)

Expand Down Expand Up @@ -744,7 +744,7 @@ def test_spec_from_file_location_smsl_empty(self):
self.assertEqual(spec.loader, self.fileloader)
self.assertEqual(spec.origin, self.path)
self.assertIs(spec.loader_state, None)
self.assertEqual(spec.submodule_search_locations, [''])
self.assertEqual(spec.submodule_search_locations, [os.getcwd()])
self.assertEqual(spec.cached, self.cached)
self.assertTrue(spec.has_location)

Expand All @@ -769,7 +769,7 @@ def test_spec_from_file_location_smsl_default(self):
self.assertEqual(spec.loader, self.pkgloader)
self.assertEqual(spec.origin, self.path)
self.assertIs(spec.loader_state, None)
self.assertEqual(spec.submodule_search_locations, [''])
self.assertEqual(spec.submodule_search_locations, [os.getcwd()])
self.assertEqual(spec.cached, self.cached)
self.assertTrue(spec.has_location)

Expand Down Expand Up @@ -817,6 +817,17 @@ def is_package(self, name):
self.assertEqual(spec.cached, self.cached)
self.assertTrue(spec.has_location)

def test_spec_from_file_location_relative_path(self):
spec = self.util.spec_from_file_location(self.name,
os.path.basename(self.path), loader=self.fileloader)

self.assertEqual(spec.name, self.name)
self.assertEqual(spec.loader, self.fileloader)
self.assertEqual(spec.origin, self.path)
self.assertIs(spec.loader_state, None)
self.assertIs(spec.submodule_search_locations, None)
self.assertEqual(spec.cached, self.cached)
self.assertTrue(spec.has_location)

(Frozen_FactoryTests,
Source_FactoryTests
Expand Down
45 changes: 45 additions & 0 deletions Lib/test/test_importlib/test_windows.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,3 +126,48 @@ def test_tagged_suffix(self):
(Frozen_WindowsExtensionSuffixTests,
Source_WindowsExtensionSuffixTests
) = test_util.test_both(WindowsExtensionSuffixTests, machinery=machinery)


@unittest.skipUnless(sys.platform.startswith('win'), 'requires Windows')
class WindowsBootstrapPathTests(unittest.TestCase):
def check_join(self, expected, *inputs):
from importlib._bootstrap_external import _path_join
actual = _path_join(*inputs)
if expected.casefold() == actual.casefold():
return
self.assertEqual(expected, actual)

def test_path_join(self):
self.check_join(r"C:\A\B", "C:\\", "A", "B")
self.check_join(r"C:\A\B", "D:\\", "D", "C:\\", "A", "B")
self.check_join(r"C:\A\B", "C:\\", "A", "C:B")
self.check_join(r"C:\A\B", "C:\\", "A\\B")
self.check_join(r"C:\A\B", r"C:\A\B")

self.check_join("D:A", r"D:", "A")
self.check_join("D:A", r"C:\B\C", "D:", "A")
self.check_join("D:A", r"C:\B\C", r"D:A")

self.check_join(r"A\B\C", "A", "B", "C")
self.check_join(r"A\B\C", "A", r"B\C")
self.check_join(r"A\B/C", "A", "B/C")
self.check_join(r"A\B\C", "A/", "B\\", "C")

# Dots are not normalised by this function
self.check_join(r"A\../C", "A", "../C")
self.check_join(r"A.\.\B", "A.", ".", "B")

self.check_join(r"\\Server\Share\A\B\C", r"\\Server\Share", "A", "B", "C")
self.check_join(r"\\Server\Share\A\B\C", r"\\Server\Share", "D", r"\A", "B", "C")
self.check_join(r"\\Server\Share\A\B\C", r"\\Server2\Share2", "D",
r"\\Server\Share", "A", "B", "C")
self.check_join(r"\\Server\Share\A\B\C", r"\\Server", r"\Share", "A", "B", "C")
self.check_join(r"\\Server\Share", r"\\Server\Share")
self.check_join(r"\\Server\Share\\", r"\\Server\Share\\")

# Handle edge cases with empty segments
self.check_join("C:\\A", "C:/A", "")
self.check_join("C:\\", "C:/", "")
self.check_join("C:", "C:", "")
self.check_join("//Server/Share\\", "//Server/Share/", "")
self.check_join("//Server/Share\\", "//Server/Share", "")
50 changes: 1 addition & 49 deletions Lib/test/test_site.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ def test_addpackage_import_bad_pth_file(self):
pth_dir, pth_fn = self.make_pth("abc\x00def\n")
with captured_stderr() as err_out:
self.assertFalse(site.addpackage(pth_dir, pth_fn, set()))
self.maxDiff = None
self.assertEqual(err_out.getvalue(), "")
for path in sys.path:
if isinstance(path, str):
Expand Down Expand Up @@ -408,55 +409,6 @@ def tearDown(self):
"""Restore sys.path"""
sys.path[:] = self.sys_path

def test_abs_paths(self):
# Make sure all imported modules have their __file__ and __cached__
# attributes as absolute paths. Arranging to put the Lib directory on
# PYTHONPATH would cause the os module to have a relative path for
# __file__ if abs_paths() does not get run. sys and builtins (the
# only other modules imported before site.py runs) do not have
# __file__ or __cached__ because they are built-in.
try:
parent = os.path.relpath(os.path.dirname(os.__file__))
cwd = os.getcwd()
except ValueError:
# Failure to get relpath probably means we need to chdir
# to the same drive.
cwd, parent = os.path.split(os.path.dirname(os.__file__))
with change_cwd(cwd):
env = os.environ.copy()
env['PYTHONPATH'] = parent
code = ('import os, sys',
# use ASCII to avoid locale issues with non-ASCII directories
'os_file = os.__file__.encode("ascii", "backslashreplace")',
r'sys.stdout.buffer.write(os_file + b"\n")',
'os_cached = os.__cached__.encode("ascii", "backslashreplace")',
r'sys.stdout.buffer.write(os_cached + b"\n")')
command = '\n'.join(code)
# First, prove that with -S (no 'import site'), the paths are
# relative.
proc = subprocess.Popen([sys.executable, '-S', '-c', command],
env=env,
stdout=subprocess.PIPE)
stdout, stderr = proc.communicate()

self.assertEqual(proc.returncode, 0)
os__file__, os__cached__ = stdout.splitlines()[:2]
self.assertFalse(os.path.isabs(os__file__))
self.assertFalse(os.path.isabs(os__cached__))
# Now, with 'import site', it works.
proc = subprocess.Popen([sys.executable, '-c', command],
env=env,
stdout=subprocess.PIPE)
stdout, stderr = proc.communicate()
self.assertEqual(proc.returncode, 0)
os__file__, os__cached__ = stdout.splitlines()[:2]
self.assertTrue(os.path.isabs(os__file__),
"expected absolute path, got {}"
.format(os__file__.decode('ascii')))
self.assertTrue(os.path.isabs(os__cached__),
"expected absolute path, got {}"
.format(os__cached__.decode('ascii')))

def test_abs_paths_cached_None(self):
"""Test for __cached__ is None.

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Importlib now resolves relative paths when creating module spec objects from
file locations.
Loading