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

[Lance Andersen]

On Mon, 29 Mar 2021 14:04:10 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review for this patch which proposes to implement the enhancement request noted in https://bugs.openjdk.java.net/browse/JDK-8173970?
>> 
>> The commit in this PR introduces the `-o` and `--output-dir` option to the `jar` command. The option takes a path to a destination directory as a value and extracts the contents of the jar into that directory. This is an optional option and the changes in the commit continue to maintain backward compatibility where the jar is extracted into current directory, if no `-o` or `--output-dir` option has been specified.
>> 
>> As far as I know, there hasn't been any discussion on what the name of this new option should be. I was initially thinking of using `-d` but that is currently used by the `jar` command for a different purpose. So I decided to use `-o` and `--output-dir`. This is of course open for change depending on any suggestions in this PR.
>> 
>> The commit in this PR also updates the `jar.properties` file which contains the English version of the jar command's `--help` output. However, no changes have been done to the internationalization files corresponding to this one (for example: `jar_de.properties`), because I don't know what process needs to be followed to have those files updated (if at all they need to be updated).
>> 
>> The commit also includes a jtreg testcase which verifies the usage of this new option.
>
> 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

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?

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?

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

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

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?

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?

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?

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?

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?

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

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