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

repository_ctx: Allow renaming archive entries during extraction. #16052

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -110,17 +110,25 @@ public static WorkspaceRuleEvent newDownloadEvent(

/** Creates a new WorkspaceRuleEvent for an extract event. */
public static WorkspaceRuleEvent newExtractEvent(
String archive, String output, String stripPrefix, String ruleLabel, Location location) {
ExtractEvent e =
String archive,
String output,
String stripPrefix,
Map<String, String> renameFiles,
String ruleLabel,
Location location) {

WorkspaceLogProtos.ExtractEvent.Builder e =
WorkspaceLogProtos.ExtractEvent.newBuilder()
.setArchive(archive)
.setOutput(output)
.setStripPrefix(stripPrefix)
.build();
.setStripPrefix(stripPrefix);
if (renameFiles != null) {
e = e.putAllRenameFiles(renameFiles);
}

WorkspaceLogProtos.WorkspaceEvent.Builder result =
WorkspaceLogProtos.WorkspaceEvent.newBuilder();
result = result.setExtractEvent(e);
result = result.setExtractEvent(e.build());
if (location != null) {
result = result.setLocation(location.toString());
}
Expand All @@ -138,6 +146,7 @@ public static WorkspaceRuleEvent newDownloadAndExtractEvent(
String integrity,
String type,
String stripPrefix,
Map<String, String> renameFiles,
String ruleLabel,
Location location) {
WorkspaceLogProtos.DownloadAndExtractEvent.Builder e =
Expand All @@ -150,6 +159,9 @@ public static WorkspaceRuleEvent newDownloadAndExtractEvent(
for (URL u : urls) {
e.addUrl(u.toString());
}
if (renameFiles != null) {
e = e.putAllRenameFiles(renameFiles);
}

WorkspaceLogProtos.WorkspaceEvent.Builder result =
WorkspaceLogProtos.WorkspaceEvent.newBuilder();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ message ExtractEvent {
string output = 2;
// A directory prefix to strip from extracted files.
string strip_prefix = 3;
// Files to rename during extraction.
map<string, string> rename_files = 4;
}

message DownloadAndExtractEvent {
Expand All @@ -74,6 +76,8 @@ message DownloadAndExtractEvent {
string strip_prefix = 5;
// checksum in Subresource Integrity format, if specified
string integrity = 6;
// Files to rename during extraction.
map<string, string> rename_files = 7;
}

// Information on "file" event in repository_ctx.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.util.Map;
import org.apache.commons.compress.archivers.ar.ArArchiveEntry;
import org.apache.commons.compress.archivers.ar.ArArchiveInputStream;

Expand Down Expand Up @@ -49,11 +50,17 @@ public Path decompress(DecompressorDescriptor descriptor)
throw new InterruptedException();
}

Map<String, String> renameFiles = descriptor.renameFiles();

try (InputStream decompressorStream = getDecompressorStream(descriptor)) {
ArArchiveInputStream arStream = new ArArchiveInputStream(decompressorStream);
ArArchiveEntry entry;
while ((entry = arStream.getNextArEntry()) != null) {
Path filePath = descriptor.repositoryPath().getRelative(entry.getName());
String entryName = entry.getName();
if (renameFiles != null) {
entryName = renameFiles.getOrDefault(entryName, entryName);
}
Path filePath = descriptor.repositoryPath().getRelative(entryName);
filePath.getParentDirectory().createDirectoryAndParents();
if (entry.isDirectory()) {
// ar archives don't contain any directory information, so this should never
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public Path decompress(DecompressorDescriptor descriptor)
throw new InterruptedException();
}
Optional<String> prefix = descriptor.prefix();
Map<String, String> renameFiles = descriptor.renameFiles();
boolean foundPrefix = false;
Set<String> availablePrefixes = new HashSet<>();
// Store link, target info of symlinks, we create them after regular files are extracted.
Expand All @@ -56,7 +57,11 @@ public Path decompress(DecompressorDescriptor descriptor)
TarArchiveInputStream tarStream = new TarArchiveInputStream(decompressorStream);
TarArchiveEntry entry;
while ((entry = tarStream.getNextTarEntry()) != null) {
StripPrefixedPath entryPath = StripPrefixedPath.maybeDeprefix(entry.getName(), prefix);
String entryName = entry.getName();
if (renameFiles != null) {
entryName = renameFiles.getOrDefault(entryName, entryName);
}
StripPrefixedPath entryPath = StripPrefixedPath.maybeDeprefix(entryName, prefix);
foundPrefix = foundPrefix || entryPath.foundPrefix();

if (prefix.isPresent() && !foundPrefix) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
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.Map;
import java.util.Objects;
import javax.annotation.Nullable;

Expand All @@ -33,17 +34,21 @@ public class DecompressorDescriptor {
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, Decompressor decompressor) {
@Nullable String prefix, boolean executable,
@Nullable Map<String, String> renameFiles,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't need to be nullable (it's never null anyway, and null has the same semantics as empty). also remove all the null checks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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;
}

Expand Down Expand Up @@ -71,6 +76,10 @@ public boolean executable() {
return executable;
}

public Map<String, String> renameFiles() {
return renameFiles;
}

public Decompressor getDecompressor() {
return decompressor;
}
Expand All @@ -92,12 +101,13 @@ public boolean equals(Object other) {
&& Objects.equals(repositoryPath, descriptor.repositoryPath)
&& Objects.equals(prefix, descriptor.prefix)
&& Objects.equals(executable, descriptor.executable)
&& Objects.equals(renameFiles, descriptor.renameFiles)
&& decompressor == descriptor.decompressor;
}

@Override
public int hashCode() {
return Objects.hash(targetKind, targetName, archivePath, repositoryPath, prefix);
return Objects.hash(targetKind, targetName, archivePath, repositoryPath, prefix, renameFiles);
}

public static Builder builder() {
Expand All @@ -115,6 +125,7 @@ public static class Builder {
private Path repositoryPath;
private String prefix;
private boolean executable;
private Map<String, String> renameFiles;
private Decompressor decompressor;

private Builder() {
Expand All @@ -125,7 +136,7 @@ public DecompressorDescriptor build() throws RepositoryFunctionException {
decompressor = DecompressorValue.getDecompressor(archivePath);
}
return new DecompressorDescriptor(
targetKind, targetName, archivePath, repositoryPath, prefix, executable, decompressor);
targetKind, targetName, archivePath, repositoryPath, prefix, executable, renameFiles, decompressor);
}

@CanIgnoreReturnValue
Expand Down Expand Up @@ -164,6 +175,12 @@ public Builder setExecutable(boolean executable) {
return this;
}

@CanIgnoreReturnValue
public Builder setRenameFiles(Map<String, String> renameFiles) {
this.renameFiles = renameFiles;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defensively copy here (ImmutableMap.copyOf)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return this;
}

@CanIgnoreReturnValue
public Builder setDecompressor(Decompressor decompressor) {
this.decompressor = decompressor;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,19 @@ public Path decompress(DecompressorDescriptor descriptor)
throws IOException, InterruptedException {
Path destinationDirectory = descriptor.repositoryPath();
Optional<String> prefix = descriptor.prefix();
Map<String, String> renameFiles = descriptor.renameFiles();
boolean foundPrefix = false;
// Store link, target info of symlinks, we create them after regular files are extracted.
Map<Path, PathFragment> symlinks = new HashMap<>();

try (ZipReader reader = new ZipReader(descriptor.archivePath().getPathFile())) {
Collection<ZipFileEntry> entries = reader.entries();
for (ZipFileEntry entry : entries) {
StripPrefixedPath entryPath = StripPrefixedPath.maybeDeprefix(entry.getName(), prefix);
String entryName = entry.getName();
if (renameFiles != null) {
entryName = renameFiles.getOrDefault(entryName, entryName);
}
StripPrefixedPath entryPath = StripPrefixedPath.maybeDeprefix(entryName, prefix);
foundPrefix = foundPrefix || entryPath.foundPrefix();
if (entryPath.skip()) {
continue;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -749,8 +749,22 @@ public StructImpl download(
+ " archive. Instead of needing to specify this prefix over and over in the"
+ " <code>build_file</code>, this field can be used to strip it from extracted"
+ " files."),
@Param(
name = "renameFiles",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first time I actually noticed that stripPrefix has a camel case name - that looks like a bug as Starlark parameters usually use snake case. Using snake case for renameFiles at least wouldn't complicate future efforts to clean up this inconsistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

defaultValue = "{}",
named = true,
positional = false,
doc =
"An optional dict specifying files to rename during the extraction. Archive entries"
+ " with names exactly matching a key will be renamed to the value, prior to"
+ " any directory prefix adjustment."),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add more information on how this parameter can be used, like you did in the PR comments? or maybe even link to this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

})
public void extract(Object archive, Object output, String stripPrefix, StarlarkThread thread)
public void extract(
Object archive,
Object output,
String stripPrefix,
Dict<?, ?> renameFiles, // <String, String> expected
StarlarkThread thread)
throws RepositoryFunctionException, InterruptedException, EvalException {
StarlarkPath archivePath = getPath("extract()", archive);

Expand All @@ -762,11 +776,15 @@ public void extract(Object archive, Object output, String stripPrefix, StarlarkT
StarlarkPath outputPath = getPath("extract()", output);
checkInOutputDirectory("write", outputPath);

Map<String, String> renameFilesMap =
Dict.cast(renameFiles, String.class, String.class, "renameFiles");

WorkspaceRuleEvent w =
WorkspaceRuleEvent.newExtractEvent(
archive.toString(),
output.toString(),
stripPrefix,
renameFilesMap,
rule.getLabel().toString(),
thread.getCallerLocation());
env.getListener().post(w);
Expand All @@ -782,6 +800,7 @@ public void extract(Object archive, Object output, String stripPrefix, StarlarkT
.setArchivePath(archivePath.getPath())
.setRepositoryPath(outputPath.getPath())
.setPrefix(stripPrefix)
.setRenameFiles(renameFilesMap)
.build());
env.getListener().post(new ExtractProgress(outputPath.getPath().toString()));
}
Expand Down Expand Up @@ -881,6 +900,15 @@ public void extract(Object archive, Object output, String stripPrefix, StarlarkT
+ " risk to omit the checksum as remote files can change. At best omitting this"
+ " field will make your build non-hermetic. It is optional to make development"
+ " easier but should be set before shipping."),
@Param(
name = "renameFiles",
defaultValue = "{}",
named = true,
positional = false,
doc =
"An optional dict specifying files to rename during the extraction. Archive entries"
+ " with names exactly matching a key will be renamed to the value, prior to"
+ " any directory prefix adjustment."),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

})
public StructImpl downloadAndExtract(
Object url,
Expand All @@ -892,6 +920,7 @@ public StructImpl downloadAndExtract(
String canonicalId,
Dict<?, ?> auth, // <String, Dict> expected
String integrity,
Dict<?, ?> renameFiles, // <String, String> expected
StarlarkThread thread)
throws RepositoryFunctionException, InterruptedException, EvalException {
Map<URI, Map<String, String>> authHeaders = getAuthHeaders(getAuthContents(auth, "auth"));
Expand All @@ -911,6 +940,9 @@ public StructImpl downloadAndExtract(
checksumValidation = e;
}

Map<String, String> renameFilesMap =
Dict.cast(renameFiles, String.class, String.class, "renameFiles");

WorkspaceRuleEvent w =
WorkspaceRuleEvent.newDownloadAndExtractEvent(
urls,
Expand All @@ -919,6 +951,7 @@ public StructImpl downloadAndExtract(
integrity,
type,
stripPrefix,
renameFilesMap,
rule.getLabel().toString(),
thread.getCallerLocation());

Expand Down Expand Up @@ -977,6 +1010,7 @@ public StructImpl downloadAndExtract(
.setArchivePath(downloadedPath)
.setRepositoryPath(outputPath.getPath())
.setPrefix(stripPrefix)
.setRenameFiles(renameFilesMap)
.build());
env.getListener().post(new ExtractProgress(outputPath.getPath().toString()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.google.devtools.build.runfiles.Runfiles;
import java.io.File;
import java.io.IOException;
import java.util.HashMap;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -62,6 +63,23 @@ public void testDecompress() throws Exception {
assertThat(secondFile.isSymbolicLink()).isFalse();
}

/**
* Test decompressing an ar file, with some entries being renamed during the
* extraction process.
*/
@Test
public void testDecompressWithRenamedFiles() throws Exception {
HashMap<String, String> renameFiles = new HashMap<>();
renameFiles.put("archived_first.txt", "renamed_file.txt");
DecompressorDescriptor.Builder descriptorBuilder =
createDescriptorBuilder().setRenameFiles(renameFiles);
Path outputDir = decompress(descriptorBuilder);

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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@

package com.google.devtools.build.lib.bazel.repository;

import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.lib.bazel.repository.TestArchiveDescriptor.INNER_FOLDER_NAME;
import static com.google.devtools.build.lib.bazel.repository.TestArchiveDescriptor.ROOT_FOLDER_NAME;

import com.google.devtools.build.lib.vfs.Path;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.HashMap;
import java.util.zip.GZIPInputStream;
import org.junit.Before;
import org.junit.Test;
Expand Down Expand Up @@ -64,6 +66,26 @@ public void testDecompressWithPrefix() throws Exception {
archiveDescriptor.assertOutputFiles(outputDir, INNER_FOLDER_NAME);
}

/**
* Test decompressing a tar.gz file, with some entries being renamed during the
* extraction process.
*/
@Test
public void testDecompressWithRenamedFiles() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a test case where strip_prefix is also present, to test that rename_files happens before strip_prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

String innerDirName = ROOT_FOLDER_NAME + "/" + INNER_FOLDER_NAME;

HashMap<String, String> renameFiles = new HashMap<>();
renameFiles.put(
innerDirName + "/hardLinkFile",
innerDirName + "/renamedFile");
DecompressorDescriptor.Builder descriptorBuilder =
archiveDescriptor.createDescriptorBuilder().setRenameFiles(renameFiles);
Path outputDir = decompress(descriptorBuilder);

Path innerDir = outputDir.getRelative(ROOT_FOLDER_NAME).getRelative(INNER_FOLDER_NAME);
assertThat(innerDir.getRelative("renamedFile").exists()).isTrue();
}

private Path decompress(DecompressorDescriptor.Builder descriptorBuilder) throws Exception {
descriptorBuilder.setDecompressor(TarGzFunction.INSTANCE);
return new CompressedTarFunction() {
Expand Down
Loading