On Wed, 31 Mar 2021 17:22:55 GMT, Lance Andersen <lan...@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
