gh-59110: zipimport: support namespace packages when no directory entry exists #121233
Conversation
serhiy-storchaka
changed the title
serhiy-storchaka
changed the title
Lib/test/test_zipimport.py
Outdated
| self.assertEqual(spec.submodule_search_locations, | ||
| [(TEMP_ZIP + "/a/b").replace("/", zipimport.path_sep)]) | ||
| self.assertRaises(zipimport.ZipImportError, importer.get_code, "a.b") | ||
| self.assertEqual(importer.get_data("a/b/"), b"") |
There was a problem hiding this comment.
I don't know if this is a necessary test case for get_data, maybe directory path will be never passed to it.
Interesting, this was not a bug before, someone who wrote the test knows how it is working. |
Lib/test/test_zipimport.py
Outdated
| try: | ||
| import a.b.c | ||
| self.assertIn('namespace', str(a.b).lower()) | ||
| self.assertEqual(a.b.c.foo(), "foo") | ||
| finally: | ||
| del sys.path[0] | ||
| sys.modules.pop('a.b.c', None) | ||
| sys.modules.pop('a.b', None) | ||
| sys.modules.pop('a', None) | ||
|
|
There was a problem hiding this comment.
I see the ImportHooksBaseTestCase has setUp and tearDown to restore the environ, so the finally section could be removed.
serhiy-storchaka
requested review from
brettcannon,
ericsnowcurrently,
ncoghlan and
warsaw
as code owners
|
I did a little test and it seems that this change also fixed my use case as well. |
serhiy-storchaka
changed the title
|
Changed the reference to the older issue #59110. |
| for name in list(files): | ||
| while True: | ||
| i = name.rstrip(path_sep).rfind(path_sep) | ||
| if i < 0: | ||
| break | ||
| name = name[:i + 1] | ||
| if name in files: | ||
| break | ||
| files[name] = None | ||
| count += 1 |
There was a problem hiding this comment.
I probably would have written this as something like:
files.update(dict.fromkeys(zipfile._path.CompleteDirs._implicit_dirs(files)))
(or some variation that ports the CompleteDirs._implicit_dirs without itertools). The advantages of using that implementation are:
- it's a proven implementation and has already encountered real-world edge cases
- it uses lazy evaluation to ensure compact memory usage
- it applies functional principles, avoiding mutating structures in place and simplifying the logic
If you want the count of files added:
updates = dict.fromkeys(zipfile._path.CompleteDirs._implicit_dirs(files))
files.update(updates)
count = len(updates)
Of course, CompleteDirs operates on posixpath.sep and files here uses pathsep for separators, so maybe it's not worth porting CompleteDirs. Still, I think it would be nice if this new behavior were factored out so as to limit making this already enormous function much larger.
I set out to apply this approach in #121378.
|
This change feels like a bugfix. The bug is affecting users of Setuptools in pypa/setuptools#4641. Could the change be backported to bugfix Pythons? |
Test written by @SeaHOH.