From 773daaf5617b5615c4b663e62ee824cd8600fa60 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 7 Dec 2023 08:30:26 -0800 Subject: [PATCH] Correctly log paths for runfiles and filesets. The input mapping keys are canonical wrt the filesystem location of an input file as observed by the spawn. However, in the case of a runfiles or fileset tree, the actual contents are found at the filesystem location of the input mapping values. PiperOrigin-RevId: 588801774 Change-Id: Iea111802b006f5750655eb4fa228abdd52567812 --- .../lib/exec/ExpandedSpawnLogContext.java | 106 ++++++++++-------- .../lib/exec/ExpandedSpawnLogContextTest.java | 45 +++++++- 2 files changed, 101 insertions(+), 50 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java index 2299c63bfc10c3..960ab154d9d552 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java +++ b/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java @@ -157,30 +157,32 @@ public void logSpawn( try (SilentCloseable c = Profiler.instance().profile("logSpawn/inputs")) { for (Map.Entry e : inputMap.entrySet()) { + PathFragment displayPath = e.getKey(); ActionInput input = e.getValue(); if (input instanceof VirtualActionInput.EmptyActionInput) { continue; } - Path inputPath = fileSystem.getPath(execRoot.getRelative(input.getExecPathString())); - if (inputPath.isDirectory()) { - listDirectoryContents(inputPath, builder::addInputs, inputMetadataProvider); + Path contentPath = fileSystem.getPath(execRoot.getRelative(input.getExecPathString())); + if (contentPath.isDirectory()) { + listDirectoryContents( + displayPath, contentPath, builder::addInputs, inputMetadataProvider); continue; } Digest digest = computeDigest( - input, inputPath, inputMetadataProvider, xattrProvider, digestHashFunction); + input, contentPath, inputMetadataProvider, xattrProvider, digestHashFunction); boolean isTool = toolFiles.contains(input) || (input instanceof TreeFileArtifact && toolFiles.contains(((TreeFileArtifact) input).getParent())); builder .addInputsBuilder() - .setPath(input.getExecPathString()) + .setPath(displayPath.getPathString()) .setDigest(digest) .setIsTool(isTool); } } catch (IOException e) { - logger.atWarning().withCause(e).log("Error computing spawn inputs"); + logger.atWarning().withCause(e).log("Error computing spawn input properties"); } try (SilentCloseable c = Profiler.instance().profile("logSpawn/outputs")) { ArrayList outputPaths = new ArrayList<>(); @@ -189,21 +191,24 @@ public void logSpawn( } Collections.sort(outputPaths); builder.addAllListedOutputs(outputPaths); - for (Map.Entry e : existingOutputs.entrySet()) { - Path path = e.getKey(); - if (path.isDirectory()) { - listDirectoryContents(path, builder::addActualOutputs, inputMetadataProvider); - } else { - File.Builder outputBuilder = builder.addActualOutputsBuilder(); - outputBuilder.setPath(path.relativeTo(fileSystem.getPath(execRoot)).toString()); - try { - outputBuilder.setDigest( - computeDigest( - e.getValue(), path, inputMetadataProvider, xattrProvider, digestHashFunction)); - } catch (IOException ex) { - logger.atWarning().withCause(ex).log("Error computing spawn event output properties"); + try { + for (Map.Entry e : existingOutputs.entrySet()) { + Path path = e.getKey(); + ActionInput output = e.getValue(); + if (path.isDirectory()) { + listDirectoryContents( + output.getExecPath(), path, builder::addActualOutputs, inputMetadataProvider); + continue; } + builder + .addActualOutputsBuilder() + .setPath(output.getExecPathString()) + .setDigest( + computeDigest( + output, path, inputMetadataProvider, xattrProvider, digestHashFunction)); } + } catch (IOException ex) { + logger.atWarning().withCause(ex).log("Error computing spawn output properties"); } } builder.setRemotable(Spawns.mayBeExecutedRemotely(spawn)); @@ -288,35 +293,44 @@ private SortedMap listExistingOutputs(Spawn spawn, FileSystem return result; } + /** + * Expands a directory into its contents. + * + *

Note the difference between {@code displayPath} and {@code contentPath}: the first is where + * the spawn can find the directory, while the second is where Bazel can find it. They're not the + * same for a directory appearing in a runfiles or fileset tree. + */ private void listDirectoryContents( - Path path, Consumer addFile, InputMetadataProvider inputMetadataProvider) { - try { - // TODO(olaola): once symlink API proposal is implemented, report symlinks here. - List sortedDirent = new ArrayList<>(path.readdir(Symlinks.NOFOLLOW)); - sortedDirent.sort(Comparator.comparing(Dirent::getName)); - for (Dirent dirent : sortedDirent) { - String name = dirent.getName(); - Path child = path.getRelative(name); - if (dirent.getType() == Dirent.Type.DIRECTORY) { - listDirectoryContents(child, addFile, inputMetadataProvider); - } else { - String pathString; - if (child.startsWith(execRoot)) { - pathString = child.asFragment().relativeTo(execRoot).getPathString(); - } else { - pathString = child.getPathString(); - } - addFile.accept( - File.newBuilder() - .setPath(pathString) - .setDigest( - computeDigest( - null, child, inputMetadataProvider, xattrProvider, digestHashFunction)) - .build()); - } + PathFragment displayPath, + Path contentPath, + Consumer addFile, + InputMetadataProvider inputMetadataProvider) + throws IOException { + // TODO(olaola): once symlink API proposal is implemented, report symlinks here. + List sortedDirent = new ArrayList<>(contentPath.readdir(Symlinks.NOFOLLOW)); + sortedDirent.sort(Comparator.comparing(Dirent::getName)); + + for (Dirent dirent : sortedDirent) { + String name = dirent.getName(); + PathFragment childDisplayPath = displayPath.getChild(name); + Path childContentPath = contentPath.getChild(name); + + if (dirent.getType() == Dirent.Type.DIRECTORY) { + listDirectoryContents(childDisplayPath, childContentPath, addFile, inputMetadataProvider); + continue; } - } catch (IOException e) { - logger.atWarning().withCause(e).log("Error computing spawn event file properties"); + + addFile.accept( + File.newBuilder() + .setPath(childDisplayPath.getPathString()) + .setDigest( + computeDigest( + null, + childContentPath, + inputMetadataProvider, + xattrProvider, + digestHashFunction)) + .build()); } } } diff --git a/src/test/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContextTest.java b/src/test/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContextTest.java index 73a8f82a903b48..673f88c9e84d00 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContextTest.java @@ -245,7 +245,7 @@ public void testTreeInput( } @Test - public void testRunfilesInput() throws Exception { + public void testRunfilesFileInput() throws Exception { Artifact runfilesInput = ActionsTestUtil.createArtifact(rootDir, "data.txt"); writeFile(runfilesInput, "abc"); @@ -253,7 +253,8 @@ public void testRunfilesInput() throws Exception { SpawnLogContext context = createSpawnLogContext(); context.logSpawn( - // In reality, the spawn would have a RunfilesSupplier and a runfiles middleman input. + // In reality, the spawn would have a RunfilesSupplier and a runfiles middleman input, but + // these don't affect the operation of the spawn logger. defaultSpawn(), createInputMetadataProvider(runfilesInput), /* inputMap= */ ImmutableSortedMap.of( @@ -262,11 +263,47 @@ public void testRunfilesInput() throws Exception { defaultTimeout(), defaultSpawnResult()); - // TODO(tjgq): The path should be foo.runfiles/data.txt. closeAndAssertLog( context, defaultSpawnExecBuilder() - .addInputs(File.newBuilder().setPath("data.txt").setDigest(getDigest("abc"))) + .addInputs( + File.newBuilder().setPath("out/foo.runfiles/data.txt").setDigest(getDigest("abc"))) + .build()); + } + + @Test + public void testRunfilesDirectoryInput(@TestParameter DirContents dirContents) throws Exception { + Artifact runfilesInput = ActionsTestUtil.createArtifact(rootDir, "dir"); + + runfilesInput.getPath().createDirectoryAndParents(); + if (!dirContents.isEmpty()) { + writeFile(runfilesInput.getPath().getChild("data.txt"), "abc"); + } + + SpawnLogContext context = createSpawnLogContext(); + + context.logSpawn( + // In reality, the spawn would have a RunfilesSupplier and a runfiles middleman input, but + // these don't affect the operation of the spawn logger. + defaultSpawn(), + createInputMetadataProvider(runfilesInput), + /* inputMap= */ ImmutableSortedMap.of( + outputDir.getExecPath().getRelative("foo.runfiles/dir"), runfilesInput), + fs, + defaultTimeout(), + defaultSpawnResult()); + + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .addAllInputs( + dirContents.isEmpty() + ? ImmutableList.of() + : ImmutableList.of( + File.newBuilder() + .setPath("out/foo.runfiles/dir/data.txt") + .setDigest(getDigest("abc")) + .build())) .build()); }