RFR: 8173970: jar tool should have a way to extract to a directory [v3]

[Jaikiran Pai]

On Wed, 31 Mar 2021 17:22:55 GMT, Lance Andersen <lancea at openjdk.org> wrote:

>> Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Alan's review feedback for -C help text
>>  - Keep -xfP backward compatible but don't allow -C/--dir with -xfP
>
> Some additional comments basically suggesting that we test --extract in addition to -x

Thank you for the reviews, Lance. The latest version of this PR has taken into account most of these comments. There's one review comment which hasn't resulted in a change and I've added a reply to that review comment explaining my thoughts.

> src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1427:
> 
>> 1425:             return rc;    // leading '/' or 'dot-dot' only path
>> 1426:         }
>> 1427:         File f = new File(xdestDir, name.replace('/', File.separatorChar));
> 
> Could you please add a comment regarding xdestDir and also correct the typo on line 1418 'requres' -> 'requires'

Fixed the typo and also added code comment about the `xdestDir` usage and semantics. If the new comment needs clarification/changes, do let me know.

> src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 62:
> 
>> 60:         Could not create a temporary file
>> 61: error.extract.multiple.dest.dir=\
>> 62:         You may not specify more than one directory for extracting the jar
> 
> Perhaps something similar to:
> 
> You may not specify  the  '-C' or '--dir' option more than once with the '-x' option

Done

> src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 64:
> 
>> 62:         You may not specify more than one directory for extracting the jar
>> 63: error.extract.pflag.not.allowed=\
>> 64:         -P option cannot be used when extracting a jar to a specific location
> 
> Perhaps something similar to
> 
> You may not specify '-Px'  with the '-C' or '--dir' options

Makes sense. I've updated the PR with this change.

> src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 167:
> 
>> 165:         (in = {0}) (out= {1})
>> 166: out.extract.dir=\
>> 167:         extracting to {0}
> 
> Perhaps 'Extracting to directory: {0}'

Updated the message to say `extracting to directory: {0}`. I decided to use lower case for `extracting` to keep it consistent with other similar messages that get logged here.

> src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 249:
> 
>> 247: \  -C DIR                     Change to the specified directory and include the\n\
>> 248: \                             following file. When used in extract mode, extracts\n\
>> 249: \                             the jar to the specified directory
> 
> Should this be updated on line 187 as well in the compatibility mode section?

Updated in latest version of the PR.

> test/jdk/tools/jar/JarExtractTest.java line 152:
> 
>> 150:         return abs;
>> 151:     }
>> 152: 
> 
> Please add a comment to each test giving a high level overview of what it does as it will help future maintainers

Done in latest update to this PR

> test/jdk/tools/jar/JarExtractTest.java line 175:
> 
>> 173:         final String dest = "foo-bar";
>> 174:         System.out.println("Extracting " + testJarPath + " to " + dest);
>> 175:         final int exitCode = JAR_TOOL.run(System.out, System.err, "-x", "-f", testJarPath.toString(),
> 
> Perhaps add a DataProvider so you can test --extract as well?

Not a data provider but the latest version of PR tests --extract as well

> test/jdk/tools/jar/JarExtractTest.java line 216:
> 
>> 214:         final Path jarPath = createJarWithPFlagSemantics();
>> 215:         // extract with -P flag without any explicit destination directory (expect the extraction to work fine)
>> 216:         final String[] args = new String[]{"-xvfP", jarPath.toString()};
> 
> Perhaps add a DataProvider so you can test --extract as well?

Not a data provider but the latest version of PR tests --extract as well

> test/jdk/tools/jar/JarExtractTest.java line 239:
> 
>> 237:      */
>> 238:     @Test
>> 239:     public void testExtractWithDirPFlagNotAllowed() throws Exception {
> 
> I would include --extract in your testing options

Done in latest version of the PR

> test/jdk/tools/jar/JarExtractTest.java line 240:
> 
>> 238:     @Test
>> 239:     public void testExtractWithDirPFlagNotAllowed() throws Exception {
>> 240:         final String expectedErrMsg = "-P option cannot be used when extracting a jar to a specific location";
> 
> Probably need to add a comment that this must match the entry in jar.properties

Added a comment mentioning where this message is sourced from.

> test/jdk/tools/jar/JarExtractTest.java line 247:
> 
>> 245:         cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "-P", "-C", "."});
>> 246:         cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "-P", "--dir", "."});
>> 247:         cmdArgs.add(new String[]{"-xvfP", testJarPath.toString(), "-C", tmpDir});
> 
> Perhaps add a DataProvider so you can test --extract as well?

Not a data provider but the latest version of PR tests `--extract` as well.

> test/jdk/tools/jar/JarExtractTest.java line 262:
> 
>> 260:      */
>> 261:     @Test
>> 262:     public void testLegacyCompatibilityMode() throws Exception {
> 
> Perhaps add a DataProvider so you can test --extract as well?

So from what I understand of the code in the jar tool the "legacy compatibility" mode stands for using the single hypen option and clubbing multiple options into one. Something like `-xvfP` instead of `-x -v -f -P` and this legacy compatibility mode isn't applicable for the long form options like `--extract`. So IMO using `--extract` here won't work. Let me know if I have misunderstood this review comment or the semantics of the legacy compatibility mode.

> test/jdk/tools/jar/JarExtractTest.java line 282:
> 
>> 280:         cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "-C", tmpDir, "-C", tmpDir});
>> 281:         cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "--dir", tmpDir, "--dir", tmpDir});
>> 282:         cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "--dir", tmpDir, "-C", tmpDir});
> 
> Perhaps use a DataProvider so you can test --extract as well?

Done

> test/jdk/tools/jar/JarExtractTest.java line 300:
> 
>> 298:     public void testExtractPartialContent() throws Exception {
>> 299:         final String tmpDir = Files.createTempDirectory(Path.of("."), "8173970-").toString();
>> 300:         final String[] cmdArgs = new String[]{"-x", "-f", testJarPath.toString(), "--dir", tmpDir,
> 
> Perhaps add a DataProvider so you can test --extract as well?

Didn't use a data provider, but did add tests for `--extract` as well, in the latest version of this PR.

> test/jdk/tools/jar/JarExtractTest.java line 307:
> 
>> 305:         // make sure only the specific files were extracted
>> 306:         final Stream<Path> paths = Files.walk(Path.of(tmpDir));
>> 307:         Assert.assertEquals(paths.count(), 6, "Unexpected number of files/dirs in " + tmpDir);
> 
> Should '6' be in a local variable to make it clearer or at a minimum a comment

The latest update to the PR includes a comment explaining what this number represents.

> test/jdk/tools/jar/JarExtractTest.java line 332:
> 
>> 330:      */
>> 331:     private void testExtract(final String dest) throws Exception {
>> 332:         final String[] args = new String[]{"-x", "-f", testJarPath.toString(), "-C", dest};
> 
> Perhaps add a DataProvider so you can test --extract as well?

The latest version of the PR tests the `--extract` as well.

> test/jdk/tools/jar/JarExtractTest.java line 367:
> 
>> 365:     }
>> 366: 
>> 367:     private static Path createJarWithPFlagSemantics() throws IOException {
> 
> Perhaps add a comment as to what the method does

Done

-------------

PR: https://git.openjdk.java.net/jdk/pull/2752