Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8173970: jar tool should have a way to extract to a directory #2752

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
@@ -362,10 +362,7 @@ public synchronized boolean run(String args[]) {
}
}
} else if (xflag) {
if (xdestDir == null) {
// default to current working directory
xdestDir = ".";
} else if (!xdestDir.equals(".")) {
if (xdestDir != null) {
final Path destPath = Paths.get(xdestDir);
try {
Files.createDirectories(destPath);
@@ -694,6 +691,10 @@ boolean parseArgs(String args[]) {
return false;
}
}
if (xflag && pflag && xdestDir != null) {
usageError(getMsg("error.extract.pflag.not.allowed"));
return false;
}
return true;
}

@@ -1345,7 +1346,8 @@ void updateLastModifiedTime(Set<ZipEntry> zes) throws IOException {
*/
boolean extract(InputStream in, String files[]) throws IOException {
if (vflag) {
output(formatMsg("out.extract.dir", Path.of(xdestDir).normalize().toAbsolutePath().toString()));
output(formatMsg("out.extract.dir", Path.of(xdestDir == null ? "." : xdestDir).normalize()
.toAbsolutePath().toString()));
}
ZipInputStream zis = new ZipInputStream(in);
ZipEntry e;
@@ -1382,7 +1384,8 @@ boolean extract(InputStream in, String files[]) throws IOException {
*/
void extract(String fname, String files[]) throws IOException {
if (vflag) {
output(formatMsg("out.extract.dir", Path.of(xdestDir).normalize().toAbsolutePath().toString()));
output(formatMsg("out.extract.dir", Path.of(xdestDir == null ? "." : xdestDir).normalize()
.toAbsolutePath().toString()));
}
ZipFile zf = new ZipFile(fname);
Set<ZipEntry> dirs = newDirSet();
@@ -60,6 +60,8 @@ error.create.tempfile=\
Could not create a temporary file
error.extract.multiple.dest.dir=\
You may not specify more than one directory for extracting the jar

This comment has been minimized.

@LanceAndersen

LanceAndersen Mar 31, 2021
Contributor

Perhaps something similar to:

You may not specify the '-C' or '--dir' option more than once with the '-x' option

This comment has been minimized.

@jaikiran

jaikiran Apr 1, 2021
Author Member

Done

error.extract.pflag.not.allowed=\
-P option cannot be used when extracting a jar to a specific location

This comment has been minimized.

@LanceAndersen

LanceAndersen Mar 31, 2021
Contributor

Perhaps something similar to

You may not specify '-Px' with the '-C' or '--dir' options

This comment has been minimized.

@jaikiran

jaikiran Apr 1, 2021
Author Member

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

error.hash.dep=\
Hashing module {0} dependences, unable to find module {1} on module path
error.module.options.without.info=\
@@ -242,9 +244,9 @@ main.help.opt.main.describe-module=\
main.help.opt.any=\
\ Operation modifiers valid in any mode:\n\
\n\
\ -C DIR When used in extract mode, extracts the jar into the\n\
\ specified directory. In any other mode, change to the\n\
\ specified directory and include the following file
\ -C DIR Change to the specified directory and include the\n\
\ following file. When used in extract mode, extracts\n\
\ the jar to the specified directory

This comment has been minimized.

@LanceAndersen

LanceAndersen Mar 31, 2021
Contributor

Should this be updated on line 187 as well in the compatibility mode section?

This comment has been minimized.

@jaikiran

jaikiran Apr 1, 2021
Author Member

Updated in latest version of the PR.

main.help.opt.any.file=\
\ -f, --file=FILE The archive file name. When omitted, either stdin or\n\
\ stdout is used based on the operation\n\
@@ -56,13 +56,17 @@
);

private static final byte[] FILE_CONTENT = "Hello world!!!".getBytes(StandardCharsets.UTF_8);
private static final String LEADING_SLASH_PRESERVED_ENTRY = "/tmp/8173970/f1.txt";
// the jar that will get extracted in the tests
private static Path testJarPath;
private static Collection<Path> filesToDelete = new ArrayList<>();

@BeforeClass
public static void createTestJar() throws Exception {
testJarPath = Paths.get("8173970-test.jar");
Files.deleteIfExists(Path.of(LEADING_SLASH_PRESERVED_ENTRY));

final String tmpDir = Files.createTempDirectory("8173970-").toString();
testJarPath = Paths.get(tmpDir,"8173970-test.jar");
final JarBuilder builder = new JarBuilder(testJarPath.toString());
// d1
// |--- d2
@@ -78,6 +82,8 @@ public static void createTestJar() throws Exception {
builder.addEntry("d1/d2/d3/f2.txt", FILE_CONTENT);
builder.addEntry("d1/d4/", new byte[0]);
builder.build();

filesToDelete.add(Path.of(LEADING_SLASH_PRESERVED_ENTRY));
}

@AfterClass
@@ -197,6 +203,58 @@ public void testExtractWithoutOutputDir() throws Exception {
verifyExtractedContent(".");
}

/**
* Tests that extracting a jar using {@code -P} flag and without any explicit destination
* directory works correctly if the jar contains entries with leading slashes and/or {@code ..}
* parts preserved.
*/
@Test
public void testExtractNoDestDirWithPFlag() throws Exception {
// create a jar which has leading slash (/) and dot-dot (..) preserved in entry names
final Path jarPath = createJarWithPFlagSemantics();
// extract with -P flag without any explicit destination directory (expect the extraction to work fine)
final String[] args = new String[]{"-xvfP", jarPath.toString()};

This comment has been minimized.

@LanceAndersen

LanceAndersen Mar 31, 2021
Contributor

Perhaps add a DataProvider so you can test --extract as well?

This comment has been minimized.

@jaikiran

jaikiran Apr 1, 2021
Author Member

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

printJarCommand(args);
final int exitCode = JAR_TOOL.run(System.out, System.err, args);
Assert.assertEquals(exitCode, 0, "Failed to extract " + jarPath);
final String dest = ".";
Assert.assertTrue(Files.isDirectory(Paths.get(dest)), dest + " is not a directory");
final Path d1 = Paths.get(dest, "d1");
Assert.assertTrue(Files.isDirectory(d1), d1 + " directory is missing or not a directory");
final Path d2 = Paths.get(dest, "d1", "d2");
Assert.assertTrue(Files.isDirectory(d2), d2 + " directory is missing or not a directory");
final Path f1 = Paths.get(LEADING_SLASH_PRESERVED_ENTRY);
Assert.assertTrue(Files.isRegularFile(f1), f1 + " is missing or not a file");
Assert.assertEquals(Files.readAllBytes(f1), FILE_CONTENT, "Unexpected content in file " + f1);
final Path f2 = Paths.get("d1/d2/../f2.txt");
Assert.assertTrue(Files.isRegularFile(f2), f2 + " is missing or not a file");
Assert.assertEquals(Files.readAllBytes(f2), FILE_CONTENT, "Unexpected content in file " + f2);
}

/**
* Tests that the {@code -P} option cannot be used during jar extraction when the {@code -C} and/or
* {@code --dir} option is used
*/
@Test
public void testExtractWithDirPFlagNotAllowed() throws Exception {

This comment has been minimized.

@LanceAndersen

LanceAndersen Mar 31, 2021
Contributor

I would include --extract in your testing options

This comment has been minimized.

@jaikiran

jaikiran Apr 1, 2021
Author Member

Done in latest version of the PR

final String expectedErrMsg = "-P option cannot be used when extracting a jar to a specific location";

This comment has been minimized.

@LanceAndersen

LanceAndersen Mar 31, 2021
Contributor

Probably need to add a comment that this must match the entry in jar.properties

This comment has been minimized.

@jaikiran

jaikiran Apr 1, 2021
Author Member

Added a comment mentioning where this message is sourced from.

final String tmpDir = Files.createTempDirectory(Path.of("."), "8173970-").toString();
final List<String[]> cmdArgs = new ArrayList<>();
cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "-P", "-C", tmpDir});
cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "-P", "--dir", tmpDir});
cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "-P", "-C", "."});
cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "-P", "--dir", "."});
cmdArgs.add(new String[]{"-xvfP", testJarPath.toString(), "-C", tmpDir});

This comment has been minimized.

@LanceAndersen

LanceAndersen Mar 31, 2021
Contributor

Perhaps add a DataProvider so you can test --extract as well?

This comment has been minimized.

@jaikiran

jaikiran Apr 1, 2021
Author Member

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

for (final String[] args : cmdArgs) {
final ByteArrayOutputStream err = new ByteArrayOutputStream();
printJarCommand(args);
int exitCode = JAR_TOOL.run(System.out, new PrintStream(err), args);
Assert.assertNotEquals(exitCode, 0, "jar extraction was expected to fail but didn't");
// verify it did indeed fail due to the right reason
Assert.assertTrue(err.toString(StandardCharsets.UTF_8).contains(expectedErrMsg));
}
}

/**
* Tests that {@code jar -xvf <jarname> -C <dir>} works fine too
*/
@@ -306,6 +364,18 @@ private void verifyExtractedContent(final String dest) throws IOException {
Assert.assertEquals(Files.readAllBytes(f2), FILE_CONTENT, "Unexpected content in file " + f2);
}

private static Path createJarWithPFlagSemantics() throws IOException {

This comment has been minimized.

@LanceAndersen

LanceAndersen Mar 31, 2021
Contributor

Perhaps add a comment as to what the method does

This comment has been minimized.

@jaikiran

jaikiran Apr 1, 2021
Author Member

Done

final String tmpDir = Files.createTempDirectory(Path.of("."), "8173970-").toString();
final Path jarPath = Paths.get(tmpDir,"8173970-test-withpflag.jar");
final JarBuilder builder = new JarBuilder(jarPath.toString());
builder.addEntry("d1/", new byte[0]);
builder.addEntry("d1/d2/", new byte[0]);
builder.addEntry(LEADING_SLASH_PRESERVED_ENTRY, FILE_CONTENT);
builder.addEntry("d1/d2/../f2.txt", FILE_CONTENT);
builder.build();
return jarPath;
}

private static void printJarCommand(final String[] cmdArgs) {
System.out.println("Running 'jar " + String.join(" ", cmdArgs) + "'");
}
ProTip! Use n and p to navigate between commits in a pull request.