Correctly match regex with tree artifact

coeuvre · copybara-github · commit 86dee6d2ecb2 · 2022-12-08T02:34:27.000-08:00
It doesn't work when setting `--experimental_remote_download_regex` to match files inside tree artifact after bazelbuild@e01e7f5. This change fixes that. Fixes bazelbuild#16922. Closes bazelbuild#16949. PiperOrigin-RevId: 493838296 Change-Id: I6eceffbffce30949173d10120a9120c6c608a983
diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
@@ -548,11 +548,15 @@ public void finalizeAction(Action action, MetadataHandler metadataHandler) {
         inputsToDownload.add(output);
       }
 
-      for (Pattern pattern : patternsToDownload) {
-        if (pattern.matcher(output.getExecPathString()).matches()) {
-          outputsToDownload.add(output);
-          break;
+      if (output.isTreeArtifact()) {
+        var children = metadataHandler.getTreeArtifactChildren((SpecialArtifact) output);
+        for (var file : children) {
+          if (outputMatchesPattern(file)) {
+            outputsToDownload.add(file);
+          }
         }
+      } else if (outputMatchesPattern(output)) {
+        outputsToDownload.add(output);
       }
     }
 
@@ -565,6 +569,15 @@ public void finalizeAction(Action action, MetadataHandler metadataHandler) {
     }
   }
 
+  private boolean outputMatchesPattern(Artifact output) {
+    for (var pattern : patternsToDownload) {
+      if (pattern.matcher(output.getExecPathString()).matches()) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   public void flushOutputTree() throws InterruptedException {
     downloadCache.awaitInProgressTasks();
   }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
@@ -101,6 +101,87 @@ public void downloadOutputsWithRegex() throws Exception {
     assertOutputsDoNotExist("//:foobar");
   }
 
+  @Test
+  public void downloadOutputsWithRegex_treeOutput_regexMatchesTreeFile() throws Exception {
+    // Disable on Windows since it fails for unknown reasons.
+    // TODO(chiwang): Enable it on windows.
+    if (OS.getCurrent() == OS.WINDOWS) {
+      return;
+    }
+
+    writeOutputDirRule();
+    write(
+        "BUILD",
+        "load(':output_dir.bzl', 'output_dir')",
+        "output_dir(",
+        "  name = 'foo',",
+        "  manifest = ':manifest',",
+        ")");
+    write("manifest", "file-1", "file-2", "file-3");
+    addOptions("--experimental_remote_download_regex=.*foo/file-2$");
+
+    buildTarget("//:foo");
+    waitDownloads();
+
+    assertValidOutputFile("foo/file-2", "file-2\n");
+    assertOutputDoesNotExist("foo/file-1");
+    assertOutputDoesNotExist("foo/file-3");
+  }
+
+  @Test
+  public void downloadOutputsWithRegex_treeOutput_regexMatchesTreeRoot() throws Exception {
+    writeOutputDirRule();
+    write(
+        "BUILD",
+        "load(':output_dir.bzl', 'output_dir')",
+        "output_dir(",
+        "  name = 'foo',",
+        "  manifest = ':manifest',",
+        ")");
+    write("manifest", "file-1", "file-2", "file-3");
+    addOptions("--experimental_remote_download_regex=.*foo$");
+
+    buildTarget("//:foo");
+    waitDownloads();
+
+    assertThat(getOutputPath("foo").exists()).isTrue();
+    assertOutputDoesNotExist("foo/file-1");
+    assertOutputDoesNotExist("foo/file-2");
+    assertOutputDoesNotExist("foo/file-3");
+  }
+
+  @Test
+  public void downloadOutputsWithRegex_regexMatchParentPath_filesNotDownloaded() throws Exception {
+    write(
+        "BUILD",
+        "genrule(",
+        "  name = 'file-1',",
+        "  srcs = [],",
+        "  outs = ['foo/file-1'],",
+        "  cmd = 'echo file-1 > $@',",
+        ")",
+        "genrule(",
+        "  name = 'file-2',",
+        "  srcs = [],",
+        "  outs = ['foo/file-2'],",
+        "  cmd = 'echo file-2 > $@',",
+        ")",
+        "genrule(",
+        "  name = 'file-3',",
+        "  srcs = [],",
+        "  outs = ['foo/file-3'],",
+        "  cmd = 'echo file-3 > $@',",
+        ")");
+    addOptions("--experimental_remote_download_regex=.*foo$");
+
+    buildTarget("//:file-1", "//:file-2", "//:file-3");
+    waitDownloads();
+
+    assertOutputDoesNotExist("foo/file-1");
+    assertOutputDoesNotExist("foo/file-2");
+    assertOutputDoesNotExist("foo/file-3");
+  }
+
   @Test
   public void intermediateOutputsAreInputForLocalActions_prefetchIntermediateOutputs()
       throws Exception {

Original file line number	Diff line number	Diff line change
`@@ -548,11 +548,15 @@ public void finalizeAction(Action action, MetadataHandler metadataHandler) {`
`548`	`548`	`inputsToDownload.add(output);`
`549`	`549`	`}`
`550`	`550`
`551`		`- for (Pattern pattern : patternsToDownload) {`
`552`		`- if (pattern.matcher(output.getExecPathString()).matches()) {`
`553`		`- outputsToDownload.add(output);`
`554`		`- break;`
	`551`	`+ if (output.isTreeArtifact()) {`
	`552`	`+ var children = metadataHandler.getTreeArtifactChildren((SpecialArtifact) output);`
	`553`	`+ for (var file : children) {`
	`554`	`+ if (outputMatchesPattern(file)) {`
	`555`	`+ outputsToDownload.add(file);`
	`556`	`+ }`
`555`	`557`	`}`
	`558`	`+ } else if (outputMatchesPattern(output)) {`
	`559`	`+ outputsToDownload.add(output);`
`556`	`560`	`}`
`557`	`561`	`}`
`558`	`562`
`@@ -565,6 +569,15 @@ public void finalizeAction(Action action, MetadataHandler metadataHandler) {`
`565`	`569`	`}`
`566`	`570`	`}`
`567`	`571`
	`572`	`+ private boolean outputMatchesPattern(Artifact output) {`
	`573`	`+ for (var pattern : patternsToDownload) {`
	`574`	`+ if (pattern.matcher(output.getExecPathString()).matches()) {`
	`575`	`+ return true;`
	`576`	`+ }`
	`577`	`+ }`
	`578`	`+ return false;`
	`579`	`+ }`
	`580`	`+`
`568`	`581`	`public void flushOutputTree() throws InterruptedException {`
`569`	`582`	`downloadCache.awaitInProgressTasks();`
`570`	`583`	`}`