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

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: 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 <[hidden email]> 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

Hi Jaikiran

Overall this looks good.  I have a few comments below and will look at the CSR shortly.

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'

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

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

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}'

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?

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

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

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

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

PR: https://git.openjdk.java.net/jdk/pull/2752
Reply | Threaded
Open this post in threaded view
|

Re: 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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Jaikiran Pai-3
On Wed, 31 Mar 2021 17:22:55 GMT, Lance Andersen <[hidden email]> 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