gh-121111: Fix zipimport incorrectly dealing with namespace packages

[ZeroIntensity]

• Issue: zipimport cannot do a namespace import when a directory has no python files, but it contains nested directories with python files inside of a zip file. #121111

[SeaHOH]

If here test the namespace module in zip, mkdir() must be removed, this bug is triggered by no directory entry in zip.

And don't do test via zipimport, that always fail/succeed, because find functons depend the loader's path prefix. Direct import namespace module can cover all cases.

[ZeroIntensity]

I'm a bit confused on what you want here - the test passes, why should mkdir() be removed?

[SeaHOH]

Again, this bug is triggered by no directory entry in zip. If the directory entry is already exists, everything is OK.

[SeaHOH]

As https://github.com/python/cpython/pull/121161/files#r1660062444 described, this patch has no effect, shoud patch _is_dir or _read_directory.

[ZeroIntensity]

As https://github.com/python/cpython/pull/121161/files#r1660062444 described, this patch has no effect, shoud patch _is_dir or _read_directory.

From what I found, there's no problem with _read_directory, and _is_dir is only relevant to find_spec (the problem still occurs in get_code, for example). This patch seems to fix the issue for both. Have you tried this fix on this issue?

[SeaHOH]

For namespace package's dealing and operation, there has already clearly explained in the source code comment. You can see them in find_loader (<=py311), find_spec and _is_dir.

And, why your test with get_code (and get_filename, get_source, is_package also) was fail? Because find functons depend the loader's path prefix, different level modules have different level loaders, skip levels will be failed. Yes, this is _get_module_path which you patched, then it broken, return the full path at once instead of stepwise lookup.

If you like to test into zipimport functions, it should be get_data:

    def testImportSubmodulesInZip(self):
        with ZipFile(TEMP_ZIP, "w") as z:
            z.writestr("a/__init__.py", b'')
            z.writestr("a/b/c/__init__.py", b'def foo(): return "foo"')

        importer = zipimport.zipimporter(TEMP_ZIP + "/a/")
        spec = importer.find_spec("a.b")
        self.assertEqual(spec.loader, None)
        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"")

        sys.path.insert(0, TEMP_ZIP)
        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)

[serhiy-storchaka]

I agree with @SeaHOH that the problem is rather in _read_directory(). I created #121233 for this approach.

[AraHaan]

This fix worked, Thank you so much.

[SeaHOH]

This fix worked, Thank you so much.

@AraHaan It just work, did not be fixed. Run the test to confirm.

[AraHaan]

This fix worked, Thank you so much.

@AraHaan It just work, did not be fixed. Run the test to confirm.

It seems to work when backported to 3.12 for me.

[SeaHOH]

@serhiy-storchaka Could you take look at the testcase and patch?

[ZeroIntensity]

Can confirm that the test written by @SeaHOH continues to fail on the new patch. There were two bugs at play here, I only patched one of them (my original test case fails on the main branch, as of now).

The underlying problem is not directly with _is_dir, but it's that _read_directory doesn't have a way to represent empty directories. AFAIK, find_spec is only affected by this now.

[SeaHOH]

Sorry for my English is so bad. Seems #121233 fixed the bug.