On Wed, 24 Nov 2021 14:06:29 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 13 commits: > > - Merge latest from master branch > - use proper FileTime centric APIs > - review suggestion - use FileTime instead of Long to prevent any potential > NPEs during unboxing > - review suggestion - split into multiple statements to make it easily > readable > - review suggestion - Use Path.of instead of Paths.get in testcase > - review suggestion - store e.getValue() and reuse the stored value > - Merge latest from master branch > - use testng asserts > - Remove "final" usage from test > - convert test to testng > - ... and 3 more: > https://git.openjdk.java.net/jdk/compare/cf7adae6...6f1c1b62 A few minor cleanups but otherwise I think this is good. src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1057: > 1055: ZipEntry e = new ZipEntry(name); > 1056: FileTime lastModified = mie.getLastModifiedTime(); > 1057: e.setLastModifiedTime(lastModified != null ? lastModified : > FileTime.fromMillis(System.currentTimeMillis())); I think this would be a bit more readable if it were if-then-else. src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1768: > 1766: return is.readAllBytes(); > 1767: } > 1768: } I think the comment would be clear if it says "the last modified time of the module-info.class". Also would you mind changing ModuleInfoEntry to use 4-space rather than 2-space indentation? ------------- Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5486
