Refactor DecompressorDescriptor

- Now uses AutoValue
- Removed the targetKind() and targetName() fields, which nobody was using, and replaced them with context(), which is more general (since this will be used by module_ctx too)
- Renamed repositoryPath() to destinationPath() (since this will be used by module_ctx too)
- Removed setDecompressor(), which was only used in tests, and was actually a no-op. The decompressor() field is only ever read in DecompressorValue#decompress, which test code never calls anyway.
- Also removed executable(), which wasn't being set or read anywhere.

Work towards bazelbuild#16144

PiperOrigin-RevId: 469700938
Change-Id: I038a6228a7583e22a0e497c74cb790235903a4ee

src/main/java/com/google/devtools/build/lib/bazel/repository/ArFunction.java

@@ -58,7 +58,7 @@ public Path decompress(DecompressorDescriptor descriptor)
       while ((entry = arStream.getNextArEntry()) != null) {
         String entryName = entry.getName();
         entryName = renameFiles.getOrDefault(entryName, entryName);
-        Path filePath = descriptor.repositoryPath().getRelative(entryName);
+        Path filePath = descriptor.destinationPath().getRelative(entryName);
         filePath.getParentDirectory().createDirectoryAndParents();
         if (entry.isDirectory()) {
           // ar archives don't contain any directory information, so this should never
@@ -81,6 +81,6 @@ public Path decompress(DecompressorDescriptor descriptor)
       }
     }
 
-    return descriptor.repositoryPath();
+    return descriptor.destinationPath();
   }
 }

src/main/java/com/google/devtools/build/lib/bazel/repository/BUILD

@@ -51,6 +51,7 @@ java_library(
         "//src/main/java/com/google/devtools/common/options",
         "//src/main/java/net/starlark/java/eval",
         "//third_party:apache_commons_compress",
+        "//third_party:auto_value",
         "//third_party:flogger",
         "//third_party:guava",
         "//third_party:java-diff-utils",

src/main/java/com/google/devtools/build/lib/bazel/repository/CompressedTarFunction.java

@@ -74,18 +74,18 @@ public Path decompress(DecompressorDescriptor descriptor)
           continue;
         }
 
-        Path filePath = descriptor.repositoryPath().getRelative(entryPath.getPathFragment());
+        Path filePath = descriptor.destinationPath().getRelative(entryPath.getPathFragment());
         filePath.getParentDirectory().createDirectoryAndParents();
         if (entry.isDirectory()) {
           filePath.createDirectoryAndParents();
         } else {
           if (entry.isSymbolicLink() || entry.isLink()) {
             PathFragment targetName = PathFragment.create(entry.getLinkName());
-            targetName = maybeDeprefixSymlink(targetName, prefix, descriptor.repositoryPath());
+            targetName = maybeDeprefixSymlink(targetName, prefix, descriptor.destinationPath());
             if (entry.isSymbolicLink()) {
               symlinks.put(filePath, targetName);
             } else {
-              Path targetPath = descriptor.repositoryPath().getRelative(targetName);
+              Path targetPath = descriptor.destinationPath().getRelative(targetName);
               if (filePath.equals(targetPath)) {
                 // The behavior here is semantically different, depending on whether the underlying
                 // filesystem is case-sensitive or case-insensitive. However, it is effectively the
@@ -133,6 +133,6 @@ public Path decompress(DecompressorDescriptor descriptor)
       }
     }
 
-    return descriptor.repositoryPath();
+    return descriptor.destinationPath();
   }
 }

src/main/java/com/google/devtools/build/lib/bazel/repository/DecompressorDescriptor.java

@@ -14,193 +14,47 @@
 
 package com.google.devtools.build.lib.bazel.repository;
 
+import com.google.auto.value.AutoValue;
 import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.bazel.repository.DecompressorValue.Decompressor;
-import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException;
 import com.google.devtools.build.lib.vfs.Path;
-import com.google.errorprone.annotations.CanIgnoreReturnValue;
-import java.util.Collections;
 import java.util.Map;
-import java.util.Objects;
-import javax.annotation.Nullable;
-
-/**
- * Description of an archive to be decompressed.
- * TODO(bazel-team): this should be an autovalue class.
- */
-public class DecompressorDescriptor {
-  private final String targetKind;
-  private final String targetName;
-  private final Path archivePath;
-  private final Path repositoryPath;
-  private final Optional<String> prefix;
-  private final boolean executable;
-  private final Map<String, String> renameFiles;
-  private final Decompressor decompressor;
-
-  private DecompressorDescriptor(
-      String targetKind,
-      String targetName,
-      Path archivePath,
-      Path repositoryPath,
-      @Nullable String prefix,
-      boolean executable,
-      Map<String, String> renameFiles,
-      Decompressor decompressor) {
-    this.targetKind = targetKind;
-    this.targetName = targetName;
-    this.archivePath = archivePath;
-    this.repositoryPath = repositoryPath;
-    this.prefix = Optional.fromNullable(prefix);
-    this.executable = executable;
-    this.renameFiles = renameFiles;
-    this.decompressor = decompressor;
-  }
 
-  public String targetKind() {
-    return targetKind;
-  }
+/** Description of an archive to be decompressed. */
+@AutoValue
+public abstract class DecompressorDescriptor {
 
-  public String targetName() {
-    return targetName;
-  }
+  /** The context in which this decompression is happening. Should only be used for reporting. */
+  public abstract String context();
 
-  public Path archivePath() {
-    return archivePath;
-  }
+  public abstract Path archivePath();
 
-  public Path repositoryPath() {
-    return repositoryPath;
-  }
+  public abstract Path destinationPath();
 
-  public Optional<String> prefix() {
-    return prefix;
-  }
+  public abstract Optional<String> prefix();
 
-  public boolean executable() {
-    return executable;
-  }
+  public abstract ImmutableMap<String, String> renameFiles();
 
-  public Map<String, String> renameFiles() {
-    return renameFiles;
+  public static Builder builder() {
+    return new AutoValue_DecompressorDescriptor.Builder()
+        .setContext("")
+        .setRenameFiles(ImmutableMap.of());
   }
 
-  public Decompressor getDecompressor() {
-    return decompressor;
-  }
+  /** Builder for describing the file to be decompressed. */
+  @AutoValue.Builder
+  public abstract static class Builder {
 
-  @Override
-  public boolean equals(Object other) {
-    if (this == other) {
-      return true;
-    }
-
-    if (!(other instanceof DecompressorDescriptor)) {
-      return false;
-    }
-
-    DecompressorDescriptor descriptor = (DecompressorDescriptor) other;
-    return Objects.equals(targetKind, descriptor.targetKind)
-        && Objects.equals(targetName, descriptor.targetName)
-        && Objects.equals(archivePath, descriptor.archivePath)
-        && Objects.equals(repositoryPath, descriptor.repositoryPath)
-        && Objects.equals(prefix, descriptor.prefix)
-        && Objects.equals(executable, descriptor.executable)
-        && Objects.equals(renameFiles, descriptor.renameFiles)
-        && decompressor == descriptor.decompressor;
-  }
+    public abstract Builder setContext(String context);
 
-  @Override
-  public int hashCode() {
-    return Objects.hash(targetKind, targetName, archivePath, repositoryPath, prefix, renameFiles);
-  }
+    public abstract Builder setArchivePath(Path archivePath);
 
-  public static Builder builder() {
-    return new Builder();
-  }
+    public abstract Builder setDestinationPath(Path destinationPath);
+
+    public abstract Builder setPrefix(String prefix);
+
+    public abstract Builder setRenameFiles(Map<String, String> renameFiles);
 
-  /**
-   * Builder for describing the file to be decompressed.  The fields set will depend on the type
-   * of file.
-   */
-  public static class Builder {
-    private String targetKind;
-    private String targetName;
-    private Path archivePath;
-    private Path repositoryPath;
-    private String prefix;
-    private boolean executable;
-    private Map<String, String> renameFiles;
-    private Decompressor decompressor;
-
-    private Builder() {
-    }
-
-    public DecompressorDescriptor build() throws RepositoryFunctionException {
-      if (decompressor == null) {
-        decompressor = DecompressorValue.getDecompressor(archivePath);
-      }
-      if (renameFiles == null) {
-        renameFiles = Collections.emptyMap();
-      }
-      return new DecompressorDescriptor(
-          targetKind,
-          targetName,
-          archivePath,
-          repositoryPath,
-          prefix,
-          executable,
-          renameFiles,
-          decompressor);
-    }
-
-    @CanIgnoreReturnValue
-    public Builder setTargetKind(String targetKind) {
-      this.targetKind = targetKind;
-      return this;
-    }
-
-    @CanIgnoreReturnValue
-    public Builder setTargetName(String targetName) {
-      this.targetName = targetName;
-      return this;
-    }
-
-    @CanIgnoreReturnValue
-    public Builder setArchivePath(Path archivePath) {
-      this.archivePath = archivePath;
-      return this;
-    }
-
-    @CanIgnoreReturnValue
-    public Builder setRepositoryPath(Path repositoryPath) {
-      this.repositoryPath = repositoryPath;
-      return this;
-    }
-
-    @CanIgnoreReturnValue
-    public Builder setPrefix(String prefix) {
-      this.prefix = prefix;
-      return this;
-    }
-
-    @CanIgnoreReturnValue
-    public Builder setExecutable(boolean executable) {
-      this.executable = executable;
-      return this;
-    }
-
-    @CanIgnoreReturnValue
-    public Builder setRenameFiles(Map<String, String> renameFiles) {
-      this.renameFiles = ImmutableMap.copyOf(renameFiles);
-      return this;
-    }
-
-    @CanIgnoreReturnValue
-    public Builder setDecompressor(Decompressor decompressor) {
-      this.decompressor = decompressor;
-      return this;
-    }
+    public abstract DecompressorDescriptor build();
   }
 }

src/main/java/com/google/devtools/build/lib/bazel/repository/DecompressorValue.java

@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.bazel.repository;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Optional;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException;
 import com.google.devtools.build.lib.vfs.Path;
@@ -89,8 +90,8 @@ public int hashCode() {
     return directory.hashCode();
   }
 
-  static Decompressor getDecompressor(Path archivePath)
-      throws RepositoryFunctionException {
+  @VisibleForTesting
+  static Decompressor getDecompressor(Path archivePath) throws RepositoryFunctionException {
     String baseName = archivePath.getBaseName();
     if (baseName.endsWith(".zip")
         || baseName.endsWith(".jar")
@@ -122,7 +123,7 @@ static Decompressor getDecompressor(Path archivePath)
   public static Path decompress(DecompressorDescriptor descriptor)
       throws RepositoryFunctionException, InterruptedException {
     try {
-      return descriptor.getDecompressor().decompress(descriptor);
+      return getDecompressor(descriptor.archivePath()).decompress(descriptor);
     } catch (IOException e) {
       Path destinationDirectory = descriptor.archivePath().getParentDirectory();
       throw new RepositoryFunctionException(

src/main/java/com/google/devtools/build/lib/bazel/repository/ZipDecompressor.java

@@ -58,8 +58,8 @@ private ZipDecompressor() {
   @VisibleForTesting static final int WINDOWS_FILE_ATTRIBUTE_NORMAL = 0x80;
 
   /**
-   * This unzips the zip file to directory {@link DecompressorDescriptor#repositoryPath()}, which by
-   * default is empty relative [to the calling external repository rule] path. The zip file is
+   * This unzips the zip file to directory {@link DecompressorDescriptor#destinationPath()}, which
+   * by default is empty relative [to the calling external repository rule] path. The zip file is
    * expected to have the WORKSPACE file at the top level, e.g.:
    *
    * <pre>
@@ -77,7 +77,7 @@ private ZipDecompressor() {
   @Nullable
   public Path decompress(DecompressorDescriptor descriptor)
       throws IOException, InterruptedException {
-    Path destinationDirectory = descriptor.repositoryPath();
+    Path destinationDirectory = descriptor.destinationPath();
     Optional<String> prefix = descriptor.prefix();
     Map<String, String> renameFiles = descriptor.renameFiles();
     boolean foundPrefix = false;

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java

@@ -797,10 +797,9 @@ public void extract(
                 outputPath.getPath().toString(), "Extracting " + archivePath.getPath()));
     DecompressorValue.decompress(
         DecompressorDescriptor.builder()
-            .setTargetKind(rule.getTargetKind())
-            .setTargetName(rule.getName())
+            .setContext(getIdentifyingStringForLogging())
             .setArchivePath(archivePath.getPath())
-            .setRepositoryPath(outputPath.getPath())
+            .setDestinationPath(outputPath.getPath())
             .setPrefix(stripPrefix)
             .setRenameFiles(renameFilesMap)
             .build());
@@ -1009,10 +1008,9 @@ public StructImpl downloadAndExtract(
               new ExtractProgress(outputPath.getPath().toString(), "Extracting " + downloadedPath));
       DecompressorValue.decompress(
           DecompressorDescriptor.builder()
-              .setTargetKind(rule.getTargetKind())
-              .setTargetName(rule.getName())
+              .setContext(getIdentifyingStringForLogging())
               .setArchivePath(downloadedPath)
-              .setRepositoryPath(outputPath.getPath())
+              .setDestinationPath(outputPath.getPath())
               .setPrefix(stripPrefix)
               .setRenameFiles(renameFilesMap)
               .build());

src/test/java/com/google/devtools/build/lib/bazel/repository/ArFunctionTest.java

@@ -47,7 +47,7 @@ public class ArFunctionTest {
 
   @Test
   public void testDecompress() throws Exception {
-    Path outputDir = decompress(createDescriptorBuilder());
+    Path outputDir = decompress(createDescriptorBuilder().build());
 
     assertThat(outputDir.exists()).isTrue();
     Path firstFile = outputDir.getRelative(FIRST_FILE_NAME);
@@ -72,16 +72,15 @@ public void testDecompressWithRenamedFiles() throws Exception {
     renameFiles.put("archived_first.txt", "renamed_file.txt");
     DecompressorDescriptor.Builder descriptorBuilder =
         createDescriptorBuilder().setRenameFiles(renameFiles);
-    Path outputDir = decompress(descriptorBuilder);
+    Path outputDir = decompress(descriptorBuilder.build());
 
     assertThat(outputDir.exists()).isTrue();
     Path renamedFile = outputDir.getRelative("renamed_file.txt");
     assertThat(renamedFile.exists()).isTrue();
   }
 
-  private Path decompress(DecompressorDescriptor.Builder descriptorBuilder) throws Exception {
-    descriptorBuilder.setDecompressor(ArFunction.INSTANCE);
-    return new ArFunction().decompress(descriptorBuilder.build());
+  private Path decompress(DecompressorDescriptor descriptor) throws Exception {
+    return new ArFunction().decompress(descriptor);
   }
 
   private DecompressorDescriptor.Builder createDescriptorBuilder() throws IOException {
@@ -100,6 +99,6 @@ private DecompressorDescriptor.Builder createDescriptorBuilder() throws IOExcept
     Path workingDir = testFS.getPath(new File(TestUtils.tmpDir()).getCanonicalPath());
     Path outDir = workingDir.getRelative("out");
 
-    return DecompressorDescriptor.builder().setRepositoryPath(outDir).setArchivePath(tarballPath);
+    return DecompressorDescriptor.builder().setDestinationPath(outDir).setArchivePath(tarballPath);
   }
 }