Skip to content

Commit

Permalink
[7.1.0] Report unresolved symlinks as such in the execution log. (#21177
Browse files Browse the repository at this point in the history 
)

In addition, check that an output is of the expected type before
emitting it, and otherwise deem it nonexistent. Strictly speaking, this
is a breaking change for the old (expanded) log formats, but any such
spawn will later cause the respective action to fail, so it doesn't
materially affect the ability to analyze cache misses.

PiperOrigin-RevId: 602724835
Change-Id: Id3ccb2db8362e19d7e26bf8e6bc5cb7fb9478d52
  • Loading branch information
@tjgq
tjgq authored Feb 1, 2024
1 parent 3f11b34 commit ecd85e1
Show file tree
Hide file tree
Showing 5 changed files with 223 additions and 77 deletions.
46 changes: 35 additions & 11 deletions src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ private interface ExecLogEntrySupplier {
private final XattrProvider xattrProvider;

// Maps a key identifying an entry into its ID.
// Each key is either a NestedSet.Node or the String path of a file or directory.
// Each key is either a NestedSet.Node or the String path of a file, directory or symlink.
// Only entries that are likely to be referenced by future entries are stored.
// Use a specialized map for minimal memory footprint.
@GuardedBy("this")
Expand Down Expand Up @@ -143,17 +143,21 @@ public void logSpawn(

for (ActionInput output : spawn.getOutputFiles()) {
Path path = fileSystem.getPath(execRoot.getRelative(output.getExecPath()));
if (!path.exists()) {
if (!output.isDirectory() && !output.isSymlink() && path.isFile()) {
builder.addOutputs(
ExecLogEntry.Output.newBuilder().setMissingPath(output.getExecPathString()));
} else if (path.isDirectory()) {
ExecLogEntry.Output.newBuilder()
.setFileId(logFile(output, path, inputMetadataProvider)));
} else if (output.isDirectory() && path.isDirectory()) {
builder.addOutputs(
ExecLogEntry.Output.newBuilder()
.setDirectoryId(logDirectory(output, path, inputMetadataProvider)));
} else {
} else if (output.isSymlink() && path.isSymbolicLink()) {
builder.addOutputs(
ExecLogEntry.Output.newBuilder()
.setFileId(logFile(output, path, inputMetadataProvider)));
.setUnresolvedSymlinkId(logUnresolvedSymlink(output, path)));
} else {
builder.addOutputs(
ExecLogEntry.Output.newBuilder().setInvalidOutputPath(output.getExecPathString()));
}
}

Expand Down Expand Up @@ -335,12 +339,12 @@ private int logFile(ActionInput input, Path path, InputMetadataProvider inputMet
/**
* Logs a directory.
*
* <p>This may be either a source directory, a fileset or an output directory. An output directory
* is not guaranteed to be a tree artifact; we must support the case where a directory is produced
* when a file was expected. For runfiles, {@link #logRunfilesDirectory} must be used instead.
* <p>This may be either a source directory, a fileset or an output directory. For runfiles,
* {@link #logRunfilesDirectory} must be used instead.
*
* @param input the input representing the directory
* @param root the path to the directory
* @param input the input representing the directory.
* @param root the path to the directory, which must have already been verified to be of the
* correct type.
* @return the entry ID of the {@link ExecLogEntry.Directory} describing the directory.
*/
private int logDirectory(
Expand Down Expand Up @@ -455,6 +459,26 @@ private ImmutableList<ExecLogEntry.File> expandDirectory(
return builder.build();
}

/**
* Logs an unresolved symlink.
*
* @param input the input representing the unresolved symlink.
* @param path the path to the unresolved symlink, which must have already been verified to be of
* the correct type.
* @return the entry ID of the {@link ExecLogEntry.UnresolvedSymlink} describing the unresolved
* symlink.
*/
private int logUnresolvedSymlink(ActionInput input, Path path) throws IOException {
return logEntry(
input.getExecPathString(),
() ->
ExecLogEntry.newBuilder()
.setUnresolvedSymlink(
ExecLogEntry.UnresolvedSymlink.newBuilder()
.setPath(input.getExecPathString())
.setTargetPath(path.readSymbolicLink().getPathString())));
}

/**
* Ensures an entry is written to the log and returns its assigned ID.
*
Expand Down
62 changes: 31 additions & 31 deletions src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.FileArtifactValue.UnresolvedSymlinkArtifactValue;
import com.google.devtools.build.lib.actions.InputMetadataProvider;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnResult;
Expand Down Expand Up @@ -56,7 +57,6 @@
import java.util.List;
import java.util.Map;
import java.util.SortedMap;
import java.util.TreeMap;
import java.util.function.Consumer;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -144,7 +144,6 @@ public void logSpawn(
SpawnResult result)
throws IOException, ExecException {
try (SilentCloseable c = Profiler.instance().profile("logSpawn")) {
SortedMap<Path, ActionInput> existingOutputs = listExistingOutputs(spawn, fileSystem);
SpawnExec.Builder builder = SpawnExec.newBuilder();
builder.addAllCommandArgs(spawn.getArguments());
builder.addAllEnvironmentVariables(getEnvironmentVariables(spawn));
Expand Down Expand Up @@ -175,6 +174,16 @@ public void logSpawn(
continue;
}

if (input.isSymlink()) {
UnresolvedSymlinkArtifactValue metadata =
(UnresolvedSymlinkArtifactValue) inputMetadataProvider.getInputMetadata(input);
builder
.addInputsBuilder()
.setPath(displayPath.getPathString())
.setSymlinkTargetPath(metadata.getSymlinkTarget());
continue;
}

Digest digest =
computeDigest(
input,
Expand All @@ -201,29 +210,33 @@ public void logSpawn(
Collections.sort(outputPaths);
builder.addAllListedOutputs(outputPaths);
try {
for (Map.Entry<Path, ActionInput> e : existingOutputs.entrySet()) {
Path path = e.getKey();
ActionInput output = e.getValue();
if (path.isDirectory()) {
for (ActionInput output : spawn.getOutputFiles()) {
Path path = fileSystem.getPath(execRoot.getRelative(output.getExecPathString()));
if (!output.isDirectory() && !output.isSymlink() && path.isFile()) {
builder
.addActualOutputsBuilder()
.setPath(output.getExecPathString())
.setDigest(
computeDigest(
output,
path,
inputMetadataProvider,
xattrProvider,
digestHashFunction,
/* includeHashFunctionName= */ true));
} else if (output.isDirectory() && path.isDirectory()) {
listDirectoryContents(
output.getExecPath(),
path,
builder::addActualOutputs,
inputMetadataProvider,
/* isTool= */ false);
continue;
} else if (output.isSymlink() && path.isSymbolicLink()) {
builder
.addActualOutputsBuilder()
.setPath(output.getExecPathString())
.setSymlinkTargetPath(path.readSymbolicLink().getPathString());
}
builder
.addActualOutputsBuilder()
.setPath(output.getExecPathString())
.setDigest(
computeDigest(
output,
path,
inputMetadataProvider,
xattrProvider,
digestHashFunction,
/* includeHashFunctionName= */ true));
}
} catch (IOException ex) {
logger.atWarning().withCause(ex).log("Error computing spawn output properties");
Expand Down Expand Up @@ -292,18 +305,6 @@ public void close() throws IOException {
}
}

private SortedMap<Path, ActionInput> listExistingOutputs(Spawn spawn, FileSystem fileSystem) {
TreeMap<Path, ActionInput> result = new TreeMap<>();
for (ActionInput output : spawn.getOutputFiles()) {
Path outputPath = fileSystem.getPath(execRoot.getRelative(output.getExecPathString()));
// TODO(olaola): once symlink API proposal is implemented, report symlinks here.
if (outputPath.exists()) {
result.put(outputPath, output);
}
}
return result;
}

/**
* Expands a directory into its contents.
*
Expand All @@ -318,7 +319,6 @@ private void listDirectoryContents(
InputMetadataProvider inputMetadataProvider,
boolean isTool)
throws IOException {
// TODO(olaola): once symlink API proposal is implemented, report symlinks here.
List<Dirent> sortedDirent = new ArrayList<>(contentPath.readdir(Symlinks.NOFOLLOW));
sortedDirent.sort(Comparator.comparing(Dirent::getName));

Expand Down
33 changes: 26 additions & 7 deletions src/main/protobuf/spawn.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,12 @@ message File {
// Path to the file relative to the execution root.
string path = 1;

// Symlink target path.
// Only set for unresolved symlinks.
string symlink_target_path = 4;

// File digest.
// May be omitted for empty files.
// Always omitted for unresolved symlinks. May be omitted for empty files.
Digest digest = 2;

// Whether the file is a tool.
Expand Down Expand Up @@ -147,6 +151,8 @@ message SpawnExec {
string mnemonic = 10;

// The outputs generated by the execution.
// In order for one of the listed_outputs to appear here, it must have been
// produced and have the expected type (file, directory or symlink).
repeated File actual_outputs = 11;

// If the spawn did not hit a disk or remote cache, this will be the name of
Expand Down Expand Up @@ -225,13 +231,22 @@ message ExecLogEntry {
}

// An input or output directory.
// May be a source directory, a runfiles or fileset tree, or a tree artifact.
message Directory {
// The directory path.
string path = 1;
// The contained files, whose paths are relative to the directory.
repeated File files = 2;
}

// An unresolved symlink.
message UnresolvedSymlink {
// The symlink path.
string path = 1;
// The path the symlink points to.
string target_path = 2;
}

// A set of spawn inputs.
// The contents of the set are the directly referenced files and directories
// in addition to the contents of all transitively referenced sets. Sets are
Expand All @@ -249,12 +264,15 @@ message ExecLogEntry {
// A spawn output.
message Output {
oneof type {
// An output file.
// An output file, i.e., ctx.actions.declare_file.
int32 file_id = 1;
// An output directory.
// An output directory, i.e., ctx.actions.declare_directory.
int32 directory_id = 2;
// A declared output that was not produced.
string missing_path = 3;
// An output unresolved symlink, i.e., ctx.actions.declare_symlink.
int32 unresolved_symlink_id = 3;
// A declared output that is either missing or has the wrong type
// (e.g., a file where a directory was expected).
string invalid_output_path = 4;
}
}

Expand Down Expand Up @@ -325,7 +343,8 @@ message ExecLogEntry {
Invocation invocation = 2;
File file = 3;
Directory directory = 4;
InputSet input_set = 5;
Spawn spawn = 6;
UnresolvedSymlink unresolved_symlink = 5;
InputSet input_set = 6;
Spawn spawn = 7;
}
}
17 changes: 15 additions & 2 deletions src/test/java/com/google/devtools/build/lib/exec/CompactSpawnLogContextTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,14 @@ private SpawnExec reconstructSpawnExec(
builder.addActualOutputs(reconstructFile(dir, dirFile, hashFunctionName));
}
break;
case MISSING_PATH:
builder.addListedOutputs(output.getMissingPath());
case UNRESOLVED_SYMLINK_ID:
ExecLogEntry.UnresolvedSymlink symlink =
checkNotNull(entryMap.get(output.getUnresolvedSymlinkId())).getUnresolvedSymlink();
builder.addListedOutputs(symlink.getPath());
builder.addActualOutputs(reconstructSymlink(symlink));
break;
case INVALID_OUTPUT_PATH:
builder.addListedOutputs(output.getInvalidOutputPath());
break;
case TYPE_NOT_SET:
throw new AssertionError("malformed log entry");
Expand Down Expand Up @@ -252,4 +258,11 @@ private File reconstructFile(

return builder.build();
}

private File reconstructSymlink(ExecLogEntry.UnresolvedSymlink symlink) {
return File.newBuilder()
.setPath(symlink.getPath())
.setSymlinkTargetPath(symlink.getTargetPath())
.build();
}
}
Loading

0 comments on commit ecd85e1

Please sign in to comment.