Use Java 7 APIs to unzip a file instead of shelling out to `unzip`.
Summary:
Users have been reporting intermittent errors when multiple threads
are unzipping artifacts pulled from cache. Previously, we were
shelling out to `unzip` to do this extraction, but now, we use Java 7
APIs that create the parent directories for each file before
writing the file. We'll have to live with this in the wild and see
whether this is an improvement.
If nothing else, this makes this part of the code work on Windows
instead of only Linux and OS X.
Note that this diff also removes the `filesToExtract` option from `UnzipStep`,
as no one was using it and it was not tested.
Test Plan:
I did a build purely from cache with 12 threads and did not get
any collisions.
diff --git a/src/com/facebook/buck/rules/AbstractCachingBuildRule.java b/src/com/facebook/buck/rules/AbstractCachingBuildRule.java
index 774dfec..296eaca 100644
--- a/src/com/facebook/buck/rules/AbstractCachingBuildRule.java
+++ b/src/com/facebook/buck/rules/AbstractCachingBuildRule.java
@@ -23,13 +23,14 @@
import com.facebook.buck.step.StepRunner;
import com.facebook.buck.util.BuckConstant;
import com.facebook.buck.util.MorePaths;
-import com.facebook.buck.util.ProcessExecutor;
import com.facebook.buck.util.concurrent.MoreFutures;
+import com.facebook.buck.zip.Unzip;
import com.google.common.annotations.Beta;
import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;
+import com.google.common.base.Throwables;
import com.google.common.collect.Lists;
import com.google.common.util.concurrent.FutureCallback;
import com.google.common.util.concurrent.Futures;
@@ -423,40 +424,20 @@
// Unfortunately, this does not appear to work, in practice, because MoreFiles fails when trying
// to resolve a Path for a zip entry against a file Path on disk.
- // TODO(user, simons): Make this work on Windows. A custom implementation of unzip in Java
- // needs to be heavily tested to ensure that it works on all platforms in a multithreaded
- // environment. In my testing of the unzip executable, this has not been an issue, so this level
- // of fidelity needs to be maintained.
- ProcessBuilder processBuilder = new ProcessBuilder(
- "unzip", "-o", "-qq", zipFile.getAbsolutePath());
- processBuilder.directory(projectRoot.toFile());
-
try {
- ProcessExecutor executor = buildContext.createProcessExecutorForUnzippingArtifact();
- Process process = processBuilder.start();
- ProcessExecutor.Result result = executor.execute(process,
- /* shouldPrintStdOut */ false,
- /* shouldPrintStdErr */ false,
- /* isSilent */ false);
- int exitCode = result.getExitCode();
- if (exitCode != 0) {
- // In the wild, we have seen some inexplicable failures during this step. For now, we try to
- // give the user as much information as we can to debug the issue, but return false so that
- // Buck will fall back on doing a local build.
- buildContext.getEventBus().post(LogEvent.warning(
- "Failed to unzip the artifact for %s at %s.\n" +
- "The rule will be built locally, but here is the output of the failed unzip call:\n" +
- "Exit code: %s\n" +
- "STDOUT:\n%s\n" +
- "STDERR:\n%s\n",
- getBuildTarget(),
- zipFile.getAbsolutePath(),
- exitCode,
- result.getStdout(),
- result.getStderr()));
- return CacheResult.MISS;
- }
+ Unzip.extractZipFile(zipFile.getAbsolutePath(),
+ projectRoot.toAbsolutePath().toString(),
+ /* overwriteExistingFiles */ true);
} catch (IOException e) {
+ // In the wild, we have seen some inexplicable failures during this step. For now, we try to
+ // give the user as much information as we can to debug the issue, but return CacheResult.MISS
+ // so that Buck will fall back on doing a local build.
+ buildContext.getEventBus().post(LogEvent.warning(
+ "Failed to unzip the artifact for %s at %s.\n" +
+ "The rule will be built locally, but here is the stacktrace of the failed unzip call:\n" +
+ getBuildTarget(),
+ zipFile.getAbsolutePath(),
+ Throwables.getStackTraceAsString(e)));
return CacheResult.MISS;
}
diff --git a/src/com/facebook/buck/rules/BUCK b/src/com/facebook/buck/rules/BUCK
index 2d2026d..5c70e41 100644
--- a/src/com/facebook/buck/rules/BUCK
+++ b/src/com/facebook/buck/rules/BUCK
@@ -153,6 +153,7 @@
'//src/com/facebook/buck/util:util',
'//src/com/facebook/buck/util/concurrent:concurrent',
'//src/com/facebook/buck/util/environment:environment',
+ '//src/com/facebook/buck/zip:steps',
'//third-party/java/astyanax:astyanax-cassandra',
'//third-party/java/astyanax:astyanax-core',
'//third-party/java/astyanax:astyanax-thrift',
diff --git a/src/com/facebook/buck/zip/BUCK b/src/com/facebook/buck/zip/BUCK
index fc71611..6c7bcf0 100644
--- a/src/com/facebook/buck/zip/BUCK
+++ b/src/com/facebook/buck/zip/BUCK
@@ -20,7 +20,7 @@
java_library(
name = 'steps',
- srcs = glob(['*Step.java']),
+ srcs = glob(['*Step.java']) + [ 'Unzip.java' ],
deps = [
':stream',
'//lib:guava',
@@ -33,6 +33,7 @@
],
visibility = [
'//src/com/facebook/buck/android/...',
+ '//src/com/facebook/buck/rules:rules',
'//test/com/facebook/buck/zip:zip',
],
)
diff --git a/src/com/facebook/buck/zip/Unzip.java b/src/com/facebook/buck/zip/Unzip.java
new file mode 100644
index 0000000..23c8740
--- /dev/null
+++ b/src/com/facebook/buck/zip/Unzip.java
@@ -0,0 +1,66 @@
+/*
+ * Copyright 2013-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License. You may obtain
+ * a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.facebook.buck.zip;
+
+import com.google.common.io.ByteStreams;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipInputStream;
+
+public class Unzip {
+
+ public static void extractZipFile(String zipFile,
+ String destination,
+ boolean overwriteExistingFiles) throws IOException {
+ // Create output directory if it does not exist
+ File folder = new File(destination);
+ // TODO UnzipStep could be a CompositeStep with a MakeCleanDirectoryStep for the output dir.
+ Files.createDirectories(folder.toPath());
+
+ try (ZipInputStream zip = new ZipInputStream(new FileInputStream(zipFile))) {
+ for (ZipEntry entry = zip.getNextEntry(); entry != null; entry = zip.getNextEntry()) {
+ String fileName = entry.getName();
+ File target = new File(folder, fileName);
+ if (target.exists() && !overwriteExistingFiles) {
+ continue;
+ }
+
+ // TODO(mbolin): Keep track of which directories have already been written to avoid
+ // making unnecessary Files.createDirectories() calls. In practice, a single zip file will
+ // have many entries in the same directory.
+
+ if (entry.isDirectory()) {
+ // Create the directory and all its parent directories
+ Files.createDirectories(target.toPath());
+ } else {
+ // Create parent folder
+ Files.createDirectories(target.toPath().getParent());
+ // Write file
+ try (FileOutputStream out = new FileOutputStream(target)) {
+ ByteStreams.copy(zip, out);
+ }
+ }
+ }
+ }
+ }
+
+}
diff --git a/src/com/facebook/buck/zip/UnzipStep.java b/src/com/facebook/buck/zip/UnzipStep.java
index 2f1c610..f12fad9 100644
--- a/src/com/facebook/buck/zip/UnzipStep.java
+++ b/src/com/facebook/buck/zip/UnzipStep.java
@@ -19,28 +19,17 @@
import com.facebook.buck.step.ExecutionContext;
import com.facebook.buck.step.Step;
import com.facebook.buck.util.Verbosity;
-import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.io.ByteStreams;
-import java.io.File;
-import java.io.FileInputStream;
-import java.io.FileOutputStream;
import java.io.IOException;
-import java.nio.file.Files;
-import java.util.Set;
-import java.util.zip.ZipEntry;
-import java.util.zip.ZipInputStream;
public class UnzipStep implements Step {
private final String pathToZipFile;
private final String pathToDestinationDirectory;
private final boolean overwriteExistingFiles;
- private final ImmutableSet<String> filesToExtract;
/**
* Creates an {@link UnzipStep} that extracts an entire .zip archive.
@@ -52,35 +41,9 @@
String pathToZipFile,
String pathToDestinationDirectory,
boolean overwriteExistingFiles) {
- this(
- pathToZipFile,
- pathToDestinationDirectory,
- overwriteExistingFiles,
- ImmutableSet.<String>of());
- }
-
- /**
- * Creates an {@link UnzipStep} that extracts only specific files from a .zip archive.
- * Example:
- * <pre>
- * new UnzipCommand("my.apk", "/my/dir",
- * true, ImmutableSet.of("resources.arsc"))
- * </pre>
- * @param pathToZipFile archive to unzip.
- * @param pathToDestinationDirectory extract into this folder.
- * @param overwriteExistingFiles if {@code true}, existing files on disk will be overwritten.
- * @param filesToExtract only these files will be extracted. If this is the empty set, the entire
- * archive will be extracted.
- */
- public UnzipStep(
- String pathToZipFile,
- String pathToDestinationDirectory,
- boolean overwriteExistingFiles,
- Set<String> filesToExtract) {
this.pathToZipFile = Preconditions.checkNotNull(pathToZipFile);
this.pathToDestinationDirectory = Preconditions.checkNotNull(pathToDestinationDirectory);
this.overwriteExistingFiles = overwriteExistingFiles;
- this.filesToExtract = ImmutableSet.copyOf(Preconditions.checkNotNull(filesToExtract));
}
@Override
@@ -91,9 +54,8 @@
@Override
public int execute(ExecutionContext context) {
try {
- extractZipFile(pathToZipFile,
+ Unzip.extractZipFile(pathToZipFile,
pathToDestinationDirectory,
- filesToExtract,
overwriteExistingFiles);
} catch (IOException e) {
e.printStackTrace(context.getStdErr());
@@ -127,45 +89,6 @@
// file to unzip
args.add(pathToZipFile);
- // specific files within the archive to unzip -- if empty, extract all
- args.addAll(filesToExtract);
-
return Joiner.on(" ").join(args.build());
}
-
- @VisibleForTesting
- static void extractZipFile(String zipFile,
- String destination,
- ImmutableSet<String> filesToExtract,
- boolean overwriteExistingFiles) throws IOException {
- // Create output directory if it does not exist
- File folder = new File(destination);
- // TODO UnzipStep could be a CompositeStep with a MakeCleanDirectoryStep for the output dir.
- Files.createDirectories(folder.toPath());
-
- try (ZipInputStream zip = new ZipInputStream(new FileInputStream(zipFile))) {
- for (ZipEntry entry = zip.getNextEntry(); entry != null; entry = zip.getNextEntry()) {
- String fileName = entry.getName();
- // If filesToExtract is not empty, check if current entry is contained.
- if (!filesToExtract.isEmpty() && !filesToExtract.contains(fileName)) {
- continue;
- }
- File target = new File(folder, fileName);
- if (target.exists() && !overwriteExistingFiles) {
- continue;
- }
- if (entry.isDirectory()) {
- // Create the directory and all its parent directories
- Files.createDirectories(target.toPath());
- } else {
- // Create parent folder
- Files.createDirectories(target.toPath().getParent());
- // Write file
- try (FileOutputStream out = new FileOutputStream(target)) {
- ByteStreams.copy(zip, out);
- }
- }
- }
- }
- }
}
diff --git a/test/com/facebook/buck/zip/UnzipStepTest.java b/test/com/facebook/buck/zip/UnzipStepTest.java
index 8694c4a..49e24d5 100644
--- a/test/com/facebook/buck/zip/UnzipStepTest.java
+++ b/test/com/facebook/buck/zip/UnzipStepTest.java
@@ -20,7 +20,6 @@
import static org.junit.Assert.assertTrue;
import com.facebook.buck.testutil.Zip;
-import com.google.common.collect.ImmutableSet;
import org.junit.Before;
import org.junit.Rule;
@@ -51,11 +50,9 @@
@Test
public void testExtractZipFile() throws IOException {
- ImmutableSet<String> filesToExtract = ImmutableSet.of();
File extractFolder = tmpFolder.newFolder();
- UnzipStep.extractZipFile(zipFile.getAbsolutePath(),
+ Unzip.extractZipFile(zipFile.getAbsolutePath(),
extractFolder.getAbsolutePath(),
- filesToExtract,
false);
assertTrue(new File(extractFolder.getAbsolutePath() + "/1.bin").exists());
File bin2 = new File(extractFolder.getAbsolutePath() + "/subdir/2.bin");
