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
@@ -1415,15 +1415,23 @@ void extract(String fname, String files[]) throws IOException {
*/
ZipEntry extractFile(InputStream is, ZipEntry e) throws IOException {
ZipEntry rc = null;
// The spec requres all slashes MUST be forward '/', it is possible
// The spec requires all slashes MUST be forward '/', it is possible
// an offending zip/jar entry may uses the backwards slash in its
// name. It might cause problem on Windows platform as it skips
// our "safe" check for leading slahs and dot-dot. So replace them
// our "safe" check for leading slash and dot-dot. So replace them
// with '/'.
String name = safeName(e.getName().replace(File.separatorChar, '/'));
if (name.length() == 0) {
return rc; // leading '/' or 'dot-dot' only path
}
// the xdestDir points to the user specified location where the jar needs to
// be extracted. By default xdestDir is null and represents current working
// directory.
// jar extraction using -P option is only allowed when the destination
// directory isn't specified (and hence defaults to current working directory).
// In such cases using this java.io.File constructor which accepts a null parent path
// allows us to extract entries that may have leading slashes and hence may need
// to be extracted outside of the current directory.
File f = new File(xdestDir, name.replace('/', File.separatorChar));

This comment has been minimized.

@LanceAndersen

LanceAndersen Mar 31, 2021
Contributor

Could you please add a comment regarding xdestDir and also correct the typo on line 1418 'requres' -> 'requires'

This comment has been minimized.

@jaikiran

jaikiran Apr 1, 2021
Author Member

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.

if (e.isDirectory()) {
if (f.exists()) {
@@ -59,9 +59,9 @@ error.incorrect.length=\
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
You may not specify the '-C' or '--dir' option more than once with the '-x' option
error.extract.pflag.not.allowed=\
-P option cannot be used when extracting a jar to a specific location
You may not specify '-Px' with the '-C' or '--dir' options
error.hash.dep=\
Hashing module {0} dependences, unable to find module {1} on module path
error.module.options.without.info=\
@@ -164,7 +164,7 @@ out.inflated=\
out.size=\
(in = {0}) (out= {1})
out.extract.dir=\
extracting to {0}
extracting to directory: {0}

usage.compat=\
\Compatibility Interface:\
@@ -186,6 +186,7 @@ Options:\n\
\ \ -i generate index information for the specified jar files\n\
\ \ -C change to the specified directory and include the following file\n\
If any file is a directory then it is processed recursively.\n\
When used in extract mode, extracts the jar to the specified directory\n\
The manifest file name, the archive file name and the entry point name are\n\
specified in the same order as the 'm', 'f' and 'e' flags.\n\n\
Example 1: to archive two class files into an archive called classes.jar: \n\
@@ -23,8 +23,8 @@

import jdk.test.lib.util.JarBuilder;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.AfterTest;
import org.testng.annotations.BeforeTest;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

@@ -61,12 +61,12 @@
private static Path testJarPath;
private static Collection<Path> filesToDelete = new ArrayList<>();

@BeforeClass
@BeforeTest
public static void createTestJar() throws Exception {
Files.deleteIfExists(Path.of(LEADING_SLASH_PRESERVED_ENTRY));

final String tmpDir = Files.createTempDirectory("8173970-").toString();
testJarPath = Paths.get(tmpDir,"8173970-test.jar");
testJarPath = Paths.get(tmpDir, "8173970-test.jar");
final JarBuilder builder = new JarBuilder(testJarPath.toString());
// d1
// |--- d2
@@ -86,7 +86,7 @@ public static void createTestJar() throws Exception {
filesToDelete.add(Path.of(LEADING_SLASH_PRESERVED_ENTRY));
}

@AfterClass
@AfterTest
public void cleanup() {
for (final Path p : filesToDelete) {
try {
@@ -98,6 +98,9 @@ public void cleanup() {
}
}

/**
* Creates and returns various relative paths, to which the jar will be extracted in the tests
*/
@DataProvider(name = "relExtractLocations")
private Object[][] provideRelativeExtractLocations() throws Exception {
// create some dirs so that they already exist when the jar is being extracted
@@ -128,6 +131,9 @@ public void cleanup() {
};
}

/**
* Creates and returns various absolute paths, to which the jar will be extracted in the tests
*/
@DataProvider(name = "absExtractLocations")
private Object[][] provideAbsoluteExtractLocations() throws Exception {
final Object[][] relative = provideRelativeExtractLocations();
@@ -139,6 +145,9 @@ public void cleanup() {
return abs;
}

/**
* Creates and returns various normalized paths, to which the jar will be extracted in the tests
*/
@DataProvider(name = "absNormalizedExtractLocations")
private Object[][] provideAbsoluteNormalizedExtractLocations() throws Exception {
final Object[][] relative = provideAbsoluteExtractLocations();
@@ -150,26 +159,44 @@ public void cleanup() {
return abs;
}

This comment has been minimized.

@LanceAndersen

LanceAndersen Mar 31, 2021
Contributor

Please add a comment to each test giving a high level overview of what it does as it will help future maintainers

This comment has been minimized.

@jaikiran

jaikiran Apr 1, 2021
Author Member

Done in latest update to this PR

/**
* Extracts a jar to various relative paths, using the -C/--dir option and then
* verifies that the extracted content is at the expected locations with the correct
* content
*/
@Test(dataProvider = "relExtractLocations")
public void testExtractToRelativeDir(final String dest) throws Exception {
testLongFormExtract(dest);
testExtract(dest);
}

/**
* Extracts a jar to various absolute paths, using the -C/--dir option and then
* verifies that the extracted content is at the expected locations with the correct
* content
*/
@Test(dataProvider = "absExtractLocations")
public void testExtractToAbsoluteDir(final String dest) throws Exception {
testExtract(dest);
testLongFormExtract(dest);
}

/**
* Extracts a jar to various normalized paths (i.e. no {@code .} or @{code ..} in the path components),
* using the -C/--dir option and then verifies that the extracted content is at the expected locations
* with the correct content
*/
@Test(dataProvider = "absNormalizedExtractLocations")
public void testExtractToAbsoluteNormalizedDir(final String dest) throws Exception {
testExtract(dest);
testLongFormExtract(dest);
}

/**
* Test that {@code jar -x -f --dir} works as expected
* Test that extracting a jar with {@code jar -x -f --dir} works as expected
*/
@Test
public void testExtractLongForm() throws Exception {
public void testExtractLongFormDir() throws Exception {
final String dest = "foo-bar";
System.out.println("Extracting " + testJarPath + " to " + dest);
final int exitCode = JAR_TOOL.run(System.out, System.err, "-x", "-f", testJarPath.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

@@ -187,6 +214,8 @@ public void testHelpOutput() {
final int exitCode = JAR_TOOL.run(new PrintStream(outStream), System.err, "--help");
Assert.assertEquals(exitCode, 0, "jar --help command failed");
final String output = outStream.toString();
// this message is expected to be the one from the jar --help output which is sourced from
// jar.properties
final String expectedMsg = "--dir Directory into which the jar will be extracted";
Assert.assertTrue(output.contains(expectedMsg), "jar --help didn't contain --dir option");
}
@@ -203,6 +232,18 @@ public void testExtractWithoutOutputDir() throws Exception {
verifyExtractedContent(".");
}

/**
* Tests that {@code jar --extract -f} command works fine even when the -C or --dir option
* isn't specified
*/
@Test
public void testLongFormExtractWithoutOutputDir() throws Exception {
final int exitCode = JAR_TOOL.run(System.out, System.err, "--extract", "-f", testJarPath.toString());
Assert.assertEquals(exitCode, 0, "Failed to extract " + testJarPath);
// the content would have been extracted to current dir
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 ..}
@@ -212,23 +253,26 @@ public void testExtractWithoutOutputDir() throws Exception {
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()};
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);
final List<String[]> cmdArgs = new ArrayList<>();
cmdArgs.add(new String[]{"-xvfP", jarPath.toString()});
cmdArgs.add(new String[]{"--extract", "-v", "-P", "-f", jarPath.toString()});
for (final String[] args : cmdArgs) {
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);
}
}

/**
@@ -237,14 +281,20 @@ public void testExtractNoDestDirWithPFlag() throws Exception {
*/
@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 error message is expected to be the one from the jar --help output which is sourced from
// jar.properties
final String expectedErrMsg = "You may not specify '-Px' with the '-C' or '--dir' options";
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.

cmdArgs.add(new String[]{"--extract", "-f", testJarPath.toString(), "-P", "-C", tmpDir});
cmdArgs.add(new String[]{"--extract", "-f", testJarPath.toString(), "-P", "--dir", tmpDir});
cmdArgs.add(new String[]{"--extract", "-f", testJarPath.toString(), "-P", "-C", "."});
cmdArgs.add(new String[]{"--extract", "-f", testJarPath.toString(), "-P", "--dir", "."});
for (final String[] args : cmdArgs) {
final ByteArrayOutputStream err = new ByteArrayOutputStream();
printJarCommand(args);
@@ -274,12 +324,17 @@ public void testLegacyCompatibilityMode() throws Exception {
*/
@Test
public void testExtractFailWithMultipleDir() throws Exception {
final String expectedErrMsg = "You may not specify more than one directory for extracting the jar";
// this error message is expected to be the one from the jar --help output which is sourced from
// jar.properties
final String expectedErrMsg = "You may not specify the '-C' or '--dir' option more than once with the '-x' option";
final String tmpDir = Files.createTempDirectory(Path.of("."), "8173970-").toString();
final List<String[]> cmdArgs = new ArrayList<>();
cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "-C", tmpDir, "-C", tmpDir});
cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "--dir", tmpDir, "--dir", tmpDir});
cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(), "--dir", tmpDir, "-C", tmpDir});

This comment has been minimized.

@LanceAndersen

LanceAndersen Mar 31, 2021
Contributor

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

This comment has been minimized.

@jaikiran

jaikiran Apr 1, 2021
Author Member

Done

cmdArgs.add(new String[]{"--extract", "-f", testJarPath.toString(), "-C", tmpDir, "-C", tmpDir});
cmdArgs.add(new String[]{"--extract", "-f", testJarPath.toString(), "--dir", tmpDir, "--dir", tmpDir});
cmdArgs.add(new String[]{"--extract", "-f", testJarPath.toString(), "--dir", tmpDir, "-C", tmpDir});
for (final String[] args : cmdArgs) {
final ByteArrayOutputStream err = new ByteArrayOutputStream();
printJarCommand(args);
@@ -296,19 +351,37 @@ public void testExtractFailWithMultipleDir() throws Exception {
*/
@Test
public void testExtractPartialContent() throws Exception {
final String tmpDir = Files.createTempDirectory(Path.of("."), "8173970-").toString();
final String[] cmdArgs = new String[]{"-x", "-f", testJarPath.toString(), "--dir", tmpDir,
String tmpDir = Files.createTempDirectory(Path.of("."), "8173970-").toString();
String[] cmdArgs = new String[]{"-x", "-f", testJarPath.toString(), "--dir", tmpDir,
"f1.txt", "d1/d2/d3/f2.txt"};
printJarCommand(cmdArgs);
final int exitCode = JAR_TOOL.run(System.out, System.err, cmdArgs);
testExtractPartialContent(tmpDir, cmdArgs);

tmpDir = Files.createTempDirectory(Path.of("."), "8173970-").toString();
cmdArgs = new String[]{"--extract", "-f", testJarPath.toString(), "--dir", tmpDir,
"f1.txt", "d1/d2/d3/f2.txt"};
testExtractPartialContent(tmpDir, cmdArgs);

}

/**
* Extract to destDir using the passed command arguments and verify the extracted content
*/
private void testExtractPartialContent(final String destDir, final String[] extractArgs) throws Exception {
printJarCommand(extractArgs);
final int exitCode = JAR_TOOL.run(System.out, System.err, extractArgs);
Assert.assertEquals(exitCode, 0, "Failed to extract " + testJarPath);
// make sure only the specific files were extracted
final Stream<Path> paths = Files.walk(Path.of(tmpDir));
Assert.assertEquals(paths.count(), 6, "Unexpected number of files/dirs in " + tmpDir);
final Path f1 = Paths.get(tmpDir, "f1.txt");
final Stream<Path> paths = Files.walk(Path.of(destDir));
// files/dirs count expected to be found when the location to which the jar was extracted
// is walked.
// 1) The top level dir being walked 2) f1.txt file 3) d1 dir 4) d1/d2 dir
// 5) d1/d2/d3 dir 6) d1/d2/d3/f2.txt file
final int numExpectedFiles = 6;
Assert.assertEquals(paths.count(), numExpectedFiles, "Unexpected number of files/dirs in " + destDir);
final Path f1 = Paths.get(destDir, "f1.txt");
Assert.assertTrue(Files.isRegularFile(f1), f1.toString() + " wasn't extracted from " + testJarPath);
Assert.assertEquals(Files.readAllBytes(f1), FILE_CONTENT, "Unexpected content in file " + f1);
final Path d1 = Paths.get(tmpDir, "d1");
final Path d1 = Paths.get(destDir, "d1");
Assert.assertTrue(Files.isDirectory(d1), d1.toString() + " wasn't extracted from " + testJarPath);
Assert.assertEquals(Files.walk(d1, 1).count(), 2, "Unexpected number " +
"of files/dirs in " + d1);
@@ -326,7 +399,7 @@ public void testExtractPartialContent() throws Exception {
}

/**
* Extracts the jar file using {@code jar -x -f <jarfile> -C <dest>}
* Extracts the jar file using {@code jar -x -f <jarfile> -C <dest>} and verifies the extracted content
*/
private void testExtract(final String dest) throws Exception {
final String[] args = new String[]{"-x", "-f", testJarPath.toString(), "-C", dest};

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

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

@@ -336,6 +409,18 @@ private void testExtract(final String dest) throws Exception {
verifyExtractedContent(dest);
}

/**
* Extracts the jar file using {@code jar --extract -f <jarfile> -C <dest>} and verifies the
* extracted content
*/
private void testLongFormExtract(final String dest) throws Exception {
final String[] args = new String[]{"--extract", "-f", testJarPath.toString(), "-C", dest};
printJarCommand(args);
final int exitCode = JAR_TOOL.run(System.out, System.err, args);
Assert.assertEquals(exitCode, 0, "Failed to extract " + testJarPath + " to " + dest);
verifyExtractedContent(dest);
}

/**
* Verifies that the extracted jar content matches what was present in the original jar
*/
@@ -364,9 +449,13 @@ private void verifyExtractedContent(final String dest) throws IOException {
Assert.assertEquals(Files.readAllBytes(f2), FILE_CONTENT, "Unexpected content in file " + f2);
}

/**
* Creates a jar whose entries have a leading slash and the dot-dot character preserved.
* This is the same as creating a jar using {@code jar -cfP somejar.jar <file1> <file2> ...}
*/
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 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]);
ProTip! Use n and p to navigate between commits in a pull request.