Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character [v3]
On Tue, 1 Mar 2022 08:13:57 GMT, Chris Hegarty wrote: >> The module finder implementation incorrectly uses the path-separator >> character from the default file system, when mapping the relative path >> of an entry in an exploded module to a package name. This causes >> problems on Windows [*] when using a module finder with a custom file >> system that has a path-separator character that differs from that of the >> default file system, e.g. the zip file system (which uses '/', >> rather than '\' ). >> >> Rather than adding a new test for this, I deciced to just modify an >> existing one. The existing test exercises the module finder with a >> custom file system, but does so with modules have trivial single-level >> packages names. I just expanded the module to have a subpackage. This >> is sufficient to reproduce the bug, and verify the fix. >> >> [*] >> >> java.lang.module.FindException: Error reading module: /m2 >> at >> java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) >> at >> java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) >> at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) >> at >> java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) >> at >> java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) >> at >> ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) >> at >> ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) >> at >> ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) >> at ... >> Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r >> not found in module >> at >> java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) >> at >> java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) >> at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) >> at >> java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) >> at >> java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) >> ... 36 more > > Chris Hegarty has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Merge branch 'openjdk:master' into modulefinder_separator_win > - update copyright year > - add tag with OrigBugId 8178380, and bugFixId 8282444 > - fix file separator in module finder with custom fs Not an expert of the area but the proposed changes look good to me. - PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character [v3]
> The module finder implementation incorrectly uses the path-separator > character from the default file system, when mapping the relative path > of an entry in an exploded module to a package name. This causes > problems on Windows [*] when using a module finder with a custom file > system that has a path-separator character that differs from that of the > default file system, e.g. the zip file system (which uses '/', > rather than '\' ). > > Rather than adding a new test for this, I deciced to just modify an > existing one. The existing test exercises the module finder with a > custom file system, but does so with modules have trivial single-level > packages names. I just expanded the module to have a subpackage. This > is sufficient to reproduce the bug, and verify the fix. > > [*] > > java.lang.module.FindException: Error reading module: /m2 > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) > at > java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) > at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) > at > java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) > at > java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) > at > ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) > at > ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) > at > ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) > at ... > Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r not > found in module > at > java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) > at > java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) > at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) > at > java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) > ... 36 more Chris Hegarty has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'openjdk:master' into modulefinder_separator_win - update copyright year - add tag with OrigBugId 8178380, and bugFixId 8282444 - fix file separator in module finder with custom fs - Changes: - all: https://git.openjdk.java.net/jdk/pull/7632/files - new: https://git.openjdk.java.net/jdk/pull/7632/files/220c43c2..a7cbfd0c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7632&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7632&range=01-02 Stats: 3764 lines in 134 files changed: 3088 ins; 449 del; 227 mod Patch: https://git.openjdk.java.net/jdk/pull/7632.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7632/head:pull/7632 PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character [v2]
On Mon, 28 Feb 2022 16:38:27 GMT, Chris Hegarty wrote: >> The module finder implementation incorrectly uses the path-separator >> character from the default file system, when mapping the relative path >> of an entry in an exploded module to a package name. This causes >> problems on Windows [*] when using a module finder with a custom file >> system that has a path-separator character that differs from that of the >> default file system, e.g. the zip file system (which uses '/', >> rather than '\' ). >> >> Rather than adding a new test for this, I deciced to just modify an >> existing one. The existing test exercises the module finder with a >> custom file system, but does so with modules have trivial single-level >> packages names. I just expanded the module to have a subpackage. This >> is sufficient to reproduce the bug, and verify the fix. >> >> [*] >> >> java.lang.module.FindException: Error reading module: /m2 >> at >> java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) >> at >> java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) >> at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) >> at >> java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) >> at >> java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) >> at >> ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) >> at >> ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) >> at >> ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) >> at ... >> Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r >> not found in module >> at >> java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) >> at >> java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) >> at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) >> at >> java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) >> at >> java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) >> ... 36 more > > Chris Hegarty has updated the pull request incrementally with two additional > commits since the last revision: > > - update copyright year > - add tag with OrigBugId 8178380, and bugFixId 8282444 Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character
On Mon, 28 Feb 2022 14:34:43 GMT, Alan Bateman wrote: > we should create another issue to create more tests for custom file systems. Yeah, that's a good point. I'll take a closer look at this and see how best to expand the coverage (separately). > I assume you'll update the date in the copyright header of the changed files. Done. >> Hi Chris, maybe you should add @bug 8282444 to >> ModulesInCustomFileSystem.java too? > The downside is that it would make it look like the test was added fro this > issue. I think it only works if the original issue for the module system > implementation is there too. I added a tag with both the original bugId that introduced the test, 8178380, and bug Fix Id for this issue, 8282444. - PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character [v2]
> The module finder implementation incorrectly uses the path-separator > character from the default file system, when mapping the relative path > of an entry in an exploded module to a package name. This causes > problems on Windows [*] when using a module finder with a custom file > system that has a path-separator character that differs from that of the > default file system, e.g. the zip file system (which uses '/', > rather than '\' ). > > Rather than adding a new test for this, I deciced to just modify an > existing one. The existing test exercises the module finder with a > custom file system, but does so with modules have trivial single-level > packages names. I just expanded the module to have a subpackage. This > is sufficient to reproduce the bug, and verify the fix. > > [*] > > java.lang.module.FindException: Error reading module: /m2 > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) > at > java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) > at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) > at > java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) > at > java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) > at > ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) > at > ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) > at > ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) > at ... > Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r not > found in module > at > java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) > at > java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) > at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) > at > java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) > ... 36 more Chris Hegarty has updated the pull request incrementally with two additional commits since the last revision: - update copyright year - add tag with OrigBugId 8178380, and bugFixId 8282444 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7632/files - new: https://git.openjdk.java.net/jdk/pull/7632/files/ca6cdf4c..220c43c2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7632&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7632&range=00-01 Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7632.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7632/head:pull/7632 PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character
On Mon, 28 Feb 2022 16:16:31 GMT, Daniel Fuchs wrote: > Hi Chris, maybe you should add `@bug 8282444` to > `ModulesInCustomFileSystem.java` too? The downside is that it would make it look like the test was added fro this issue. I think it only works if the original issue for the module system implementation is there too. - PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character
On Mon, 28 Feb 2022 11:12:17 GMT, Chris Hegarty wrote: > The module finder implementation incorrectly uses the path-separator > character from the default file system, when mapping the relative path > of an entry in an exploded module to a package name. This causes > problems on Windows [*] when using a module finder with a custom file > system that has a path-separator character that differs from that of the > default file system, e.g. the zip file system (which uses '/', > rather than '\' ). > > Rather than adding a new test for this, I deciced to just modify an > existing one. The existing test exercises the module finder with a > custom file system, but does so with modules have trivial single-level > packages names. I just expanded the module to have a subpackage. This > is sufficient to reproduce the bug, and verify the fix. > > [*] > > java.lang.module.FindException: Error reading module: /m2 > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) > at > java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) > at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) > at > java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) > at > java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) > at > ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) > at > ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) > at > ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) > at ... > Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r not > found in module > at > java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) > at > java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) > at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) > at > java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) > ... 36 more Hi Chris, maybe you should add `@bug 8282444` to `ModulesInCustomFileSystem.java` too? - PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character
On Mon, 28 Feb 2022 11:12:17 GMT, Chris Hegarty wrote: > The module finder implementation incorrectly uses the path-separator > character from the default file system, when mapping the relative path > of an entry in an exploded module to a package name. This causes > problems on Windows [*] when using a module finder with a custom file > system that has a path-separator character that differs from that of the > default file system, e.g. the zip file system (which uses '/', > rather than '\' ). > > Rather than adding a new test for this, I deciced to just modify an > existing one. The existing test exercises the module finder with a > custom file system, but does so with modules have trivial single-level > packages names. I just expanded the module to have a subpackage. This > is sufficient to reproduce the bug, and verify the fix. > > [*] > > java.lang.module.FindException: Error reading module: /m2 > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) > at > java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) > at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) > at > java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) > at > java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) > at > ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) > at > ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) > at > ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) > at ... > Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r not > found in module > at > java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) > at > java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) > at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) > at > java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) > ... 36 more The update to ModulePath looks okay. The tests for ModulePath with paths to locate files in custom file systems are somewhat minimal and probably should be expanded to ensure that there aren't any other issues. Updating the existing ModulesInCustomFileSystem test to use a deeper package structure is subtle but okay for now but we should create another issue to create more tests for custom file systems. I assume you'll update the date in the copyright header of the changed files. - PR: https://git.openjdk.java.net/jdk/pull/7632
RFR: 8282444: Module finder incorrectly assumes default file system path-separator character
The module finder implementation incorrectly uses the path-separator character from the default file system, when mapping the relative path of an entry in an exploded module to a package name. This causes problems on Windows [*] when using a module finder with a custom file system that has a path-separator character that differs from that of the default file system, e.g. the zip file system (which uses '/', rather than '\' ). Rather than adding a new test for this, I deciced to just modify an existing one. The existing test exercises the module finder with a custom file system, but does so with modules have trivial single-level packages names. I just expanded the module to have a subpackage. This is sufficient to reproduce the bug, and verify the fix. [*] java.lang.module.FindException: Error reading module: /m2 at java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) at java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) at java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) at java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) at ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) at ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) at ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) at ... Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r not found in module at java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) at java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) at java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) at java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) ... 36 more - Commit messages: - fix file separator in module finder with custom fs Changes: https://git.openjdk.java.net/jdk/pull/7632/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7632&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282444 Stats: 7 lines in 4 files changed: 1 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7632.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7632/head:pull/7632 PR: https://git.openjdk.java.net/jdk/pull/7632
