Skip to content

Commit 20552d8

Browse files
coeuvrecopybara-github
coeuvre
authored and
copybara-github
committed
Make --experimental_remote_download_regex work in incremental builds.
When building without the bytes, we can set `--experimental_remote_download_regex` to download additionaly outputs. However, the flag doesn't work with incremental builds, since it depends on action executions. That is, in an incremental build, if you just: 1. deleted previous downloaded outputs 2. or changed the regex outputs that are expected to be downloaded will not be downloaded since the generating actions are not executed, because of cache. This change fixes that by checking whether Bazel should download an output during the dirtiness check. If so, we mark generating actions dirty so they will be re-executed (similar to the case in normal build when local outputs are missing). PiperOrigin-RevId: 527242696 Change-Id: If985d02d1dffd89780885f5b94c171a29986c30f
1 parent 1fff1a1 commit 20552d8

9 files changed

Lines changed: 156 additions & 35 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java‎

Lines changed: 5 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424

2525
import com.google.auto.value.AutoValue;
2626
import com.google.common.annotations.VisibleForTesting;
27-
import com.google.common.collect.ImmutableList;
2827
import com.google.common.collect.ImmutableSet;
2928
import com.google.common.collect.Sets;
3029
import com.google.common.flogger.GoogleLogger;
@@ -64,7 +63,6 @@
6463
import java.util.concurrent.ConcurrentHashMap;
6564
import java.util.concurrent.atomic.AtomicBoolean;
6665
import java.util.concurrent.atomic.AtomicReference;
67-
import java.util.regex.Pattern;
6866
import javax.annotation.Nullable;
6967

7068
/**
@@ -81,7 +79,7 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet
8179
protected final Set<Artifact> outputsAreInputs = Sets.newConcurrentHashSet();
8280

8381
protected final Path execRoot;
84-
protected final ImmutableList<Pattern> patternsToDownload;
82+
protected final RemoteOutputChecker remoteOutputChecker;
8583

8684
private final Set<ActionInput> missingActionInputs = Sets.newConcurrentHashSet();
8785

@@ -231,12 +229,12 @@ protected AbstractActionInputPrefetcher(
231229
Reporter reporter,
232230
Path execRoot,
233231
TempPathGenerator tempPathGenerator,
234-
ImmutableList<Pattern> patternsToDownload,
232+
RemoteOutputChecker remoteOutputChecker,
235233
OutputPermissions outputPermissions) {
236234
this.reporter = reporter;
237235
this.execRoot = execRoot;
238236
this.tempPathGenerator = tempPathGenerator;
239-
this.patternsToDownload = patternsToDownload;
237+
this.remoteOutputChecker = remoteOutputChecker;
240238
this.outputPermissions = outputPermissions;
241239
}
242240

@@ -605,11 +603,11 @@ public void finalizeAction(Action action, OutputMetadataStore outputMetadataStor
605603
} else if (output.isTreeArtifact()) {
606604
var children = outputMetadataStore.getTreeArtifactChildren((SpecialArtifact) output);
607605
for (var file : children) {
608-
if (outputMatchesPattern(file)) {
606+
if (remoteOutputChecker.shouldDownloadFile(file)) {
609607
outputsToDownload.add(file);
610608
}
611609
}
612-
} else if (outputMatchesPattern(output)) {
610+
} else if (remoteOutputChecker.shouldDownloadFile(output)) {
613611
outputsToDownload.add(output);
614612
}
615613
}
@@ -658,15 +656,6 @@ public void onFailure(Throwable throwable) {
658656
}
659657
}
660658

661-
private boolean outputMatchesPattern(Artifact output) {
662-
for (var pattern : patternsToDownload) {
663-
if (pattern.matcher(output.getExecPathString()).matches()) {
664-
return true;
665-
}
666-
}
667-
return false;
668-
}
669-
670659
public void flushOutputTree() throws InterruptedException {
671660
downloadCache.awaitInProgressTasks();
672661
}

‎src/main/java/com/google/devtools/build/lib/remote/BUILD‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,13 +190,15 @@ java_library(
190190
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
191191
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
192192
"//src/main/java/com/google/devtools/build/lib/clock",
193+
"//third_party:guava",
193194
],
194195
)
195196

196197
java_library(
197198
name = "abstract_action_input_prefetcher",
198199
srcs = ["AbstractActionInputPrefetcher.java"],
199200
deps = [
201+
":remote_output_checker",
200202
"//src/main/java/com/google/devtools/build/lib/actions",
201203
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
202204
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",

‎src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import build.bazel.remote.execution.v2.Digest;
2020
import build.bazel.remote.execution.v2.RequestMetadata;
2121
import com.google.common.base.Preconditions;
22-
import com.google.common.collect.ImmutableList;
2322
import com.google.common.util.concurrent.ListenableFuture;
2423
import com.google.devtools.build.lib.actions.FileArtifactValue;
2524
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
@@ -35,7 +34,6 @@
3534
import com.google.devtools.build.lib.vfs.Path;
3635
import com.google.devtools.build.lib.vfs.PathFragment;
3736
import java.io.IOException;
38-
import java.util.regex.Pattern;
3937

4038
/**
4139
* Stages output files that are stored remotely to the local filesystem.
@@ -56,9 +54,9 @@ class RemoteActionInputFetcher extends AbstractActionInputPrefetcher {
5654
RemoteCache remoteCache,
5755
Path execRoot,
5856
TempPathGenerator tempPathGenerator,
59-
ImmutableList<Pattern> patternsToDownload,
57+
RemoteOutputChecker remoteOutputChecker,
6058
OutputPermissions outputPermissions) {
61-
super(reporter, execRoot, tempPathGenerator, patternsToDownload, outputPermissions);
59+
super(reporter, execRoot, tempPathGenerator, remoteOutputChecker, outputPermissions);
6260
this.buildRequestId = Preconditions.checkNotNull(buildRequestId);
6361
this.commandId = Preconditions.checkNotNull(commandId);
6462
this.remoteCache = Preconditions.checkNotNull(remoteCache);

‎src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -965,7 +965,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
965965
if (!remoteOutputsMode.downloadAllOutputs() && actionContextProvider.getRemoteCache() != null) {
966966
Preconditions.checkNotNull(patternsToDownload, "patternsToDownload must not be null");
967967

968-
var remoteOutputChecker = new RemoteOutputChecker(new JavaClock());
968+
var remoteOutputChecker = new RemoteOutputChecker(new JavaClock(), patternsToDownload);
969969

970970
actionInputFetcher =
971971
new RemoteActionInputFetcher(
@@ -975,7 +975,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
975975
actionContextProvider.getRemoteCache(),
976976
env.getExecRoot(),
977977
tempPathGenerator,
978-
patternsToDownload,
978+
remoteOutputChecker,
979979
outputPermissions);
980980
env.getEventBus().register(actionInputFetcher);
981981
builder.setActionInputPrefetcher(actionInputFetcher);

‎src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,21 +13,53 @@
1313
// limitations under the License.
1414
package com.google.devtools.build.lib.remote;
1515

16+
import static com.google.common.base.Preconditions.checkArgument;
17+
18+
import com.google.common.collect.ImmutableList;
1619
import com.google.devtools.build.lib.actions.Artifact;
1720
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
1821
import com.google.devtools.build.lib.actions.RemoteArtifactChecker;
1922
import com.google.devtools.build.lib.clock.Clock;
23+
import java.util.regex.Pattern;
2024

2125
/** A {@link RemoteArtifactChecker} that checks the TTL of remote metadata. */
2226
public class RemoteOutputChecker implements RemoteArtifactChecker {
2327
private final Clock clock;
28+
private final ImmutableList<Pattern> patternsToDownload;
2429

25-
public RemoteOutputChecker(Clock clock) {
30+
public RemoteOutputChecker(Clock clock, ImmutableList<Pattern> patternsToDownload) {
2631
this.clock = clock;
32+
this.patternsToDownload = patternsToDownload;
33+
}
34+
35+
public boolean shouldDownloadFile(Artifact file) {
36+
checkArgument(!file.isTreeArtifact(), "file must not be a tree.");
37+
38+
if (outputMatchesPattern(file)) {
39+
return true;
40+
}
41+
42+
return false;
43+
}
44+
45+
private boolean outputMatchesPattern(Artifact output) {
46+
for (var pattern : patternsToDownload) {
47+
if (pattern.matcher(output.getExecPathString()).matches()) {
48+
return true;
49+
}
50+
}
51+
return false;
2752
}
2853

2954
@Override
3055
public boolean shouldTrustRemoteArtifact(Artifact file, RemoteFileArtifactValue metadata) {
56+
if (shouldDownloadFile(file)) {
57+
// If Bazel should download this file, but it does not exist locally, returns false to rerun
58+
// the generating action to trigger the download (just like in the normal build, when local
59+
// outputs are missing).
60+
return false;
61+
}
62+
3163
return metadata.isAlive(clock.now());
3264
}
3365
}

‎src/test/java/com/google/devtools/build/lib/remote/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ java_test(
7474
"//src/main/java/com/google/devtools/build/lib/pkgcache",
7575
"//src/main/java/com/google/devtools/build/lib/remote",
7676
"//src/main/java/com/google/devtools/build/lib/remote:abstract_action_input_prefetcher",
77+
"//src/main/java/com/google/devtools/build/lib/remote:remote_output_checker",
7778
"//src/main/java/com/google/devtools/build/lib/remote/common",
7879
"//src/main/java/com/google/devtools/build/lib/remote/common:bulk_transfer_exception",
7980
"//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception",

‎src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
import static com.google.common.collect.Iterables.getOnlyElement;
1717
import static com.google.common.truth.Truth.assertThat;
1818
import static com.google.devtools.build.lib.remote.util.IntegrationTestUtils.startWorker;
19+
import static com.google.devtools.build.lib.vfs.FileSystemUtils.readContent;
20+
import static java.nio.charset.StandardCharsets.UTF_8;
1921
import static org.junit.Assert.assertThrows;
2022
import static org.junit.Assume.assumeFalse;
2123

@@ -89,9 +91,9 @@ protected ImmutableList<BlazeModule> getSpawnModules() {
8991
}
9092

9193
@Override
92-
protected void assertOutputEquals(String realContent, String expectedContent, boolean isLocal)
94+
protected void assertOutputEquals(Path path, String expectedContent, boolean isLocal)
9395
throws Exception {
94-
assertThat(realContent).isEqualTo(expectedContent);
96+
assertThat(readContent(path, UTF_8)).isEqualTo(expectedContent);
9597
}
9698

9799
@Override
@@ -517,7 +519,7 @@ public void remoteCacheEvictBlobs_whenPrefetchingInput_exitWithCode39() throws E
517519

518520
// Populate remote cache
519521
buildTarget("//a:bar");
520-
var bytes = FileSystemUtils.readContent(getOutputPath("a/foo.out"));
522+
var bytes = readContent(getOutputPath("a/foo.out"));
521523
var hashCode = getDigestHashFunction().getHashFunction().hashBytes(bytes);
522524
getOutputPath("a/foo.out").delete();
523525
getOutputPath("a/bar.out").delete();
@@ -565,7 +567,7 @@ public void remoteCacheEvictBlobs_whenUploadingInput_exitWithCode39() throws Exc
565567
setDownloadAll();
566568
buildTarget("//a:bar");
567569
waitDownloads();
568-
var bytes = FileSystemUtils.readContent(getOutputPath("a/foo.out"));
570+
var bytes = readContent(getOutputPath("a/foo.out"));
569571
var hashCode = getDigestHashFunction().getHashFunction().hashBytes(bytes);
570572
getOutputPath("a/foo.out").delete();
571573
getOutputPath("a/bar.out").delete();

‎src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java‎

Lines changed: 99 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
import static com.google.common.collect.Iterables.getOnlyElement;
1717
import static com.google.common.truth.Truth.assertThat;
1818
import static com.google.common.truth.Truth.assertWithMessage;
19-
import static com.google.devtools.build.lib.vfs.FileSystemUtils.readContent;
19+
import static com.google.devtools.build.lib.vfs.FileSystemUtils.writeContent;
2020
import static java.nio.charset.StandardCharsets.UTF_8;
2121
import static org.junit.Assert.assertThrows;
2222

@@ -46,8 +46,8 @@ public abstract class BuildWithoutTheBytesIntegrationTestBase extends BuildInteg
4646

4747
protected abstract void setDownloadAll();
4848

49-
protected abstract void assertOutputEquals(
50-
String realContent, String expectedContent, boolean isLocal) throws Exception;
49+
protected abstract void assertOutputEquals(Path path, String expectedContent, boolean isLocal)
50+
throws Exception;
5151

5252
protected abstract void assertOutputContains(String content, String contains) throws Exception;
5353

@@ -133,6 +133,100 @@ public void downloadOutputsWithRegex() throws Exception {
133133

134134
assertValidOutputFile("out/foo.txt", "foo\n");
135135
assertOutputsDoNotExist("//:foobar");
136+
137+
// Assert that no actions have been executed for the next incremental build since nothing
138+
// changed
139+
ActionEventCollector actionEventCollector = new ActionEventCollector();
140+
getRuntimeWrapper().registerSubscriber(actionEventCollector);
141+
// Override out/foo.txt with the same content
142+
{
143+
var path = getOutputPath("out/foo.txt");
144+
var isWritable = path.isWritable();
145+
if (!isWritable) {
146+
path.setWritable(true);
147+
}
148+
writeContent(path, UTF_8, "foo\n");
149+
if (!isWritable) {
150+
path.setWritable(false);
151+
}
152+
}
153+
buildTarget("//:foobar");
154+
assertThat(actionEventCollector.getActionExecutedEvents()).isEmpty();
155+
}
156+
157+
@Test
158+
public void downloadOutputsWithRegex_deleteOutput_reDownload() throws Exception {
159+
// Arrange: Do a clean build and download out/foo.txt
160+
write(
161+
"BUILD",
162+
"genrule(",
163+
" name = 'foo',",
164+
" srcs = [],",
165+
" outs = ['out/foo.txt'],",
166+
" cmd = 'echo foo > $@',",
167+
")",
168+
"genrule(",
169+
" name = 'foobar',",
170+
" srcs = [':foo'],",
171+
" outs = ['out/foobar.txt'],",
172+
" cmd = 'cat $(location :foo) > $@ && echo bar >> $@',",
173+
")");
174+
addOptions("--experimental_remote_download_regex=.*foo\\.txt$");
175+
176+
buildTarget("//:foobar");
177+
waitDownloads();
178+
179+
assertValidOutputFile("out/foo.txt", "foo\n");
180+
assertOutputsDoNotExist("//:foobar");
181+
182+
// Arrange: Delete out/foo.txt and do an incremental build
183+
getOutputPath("out/foo.txt").delete();
184+
ActionEventCollector actionEventCollector = new ActionEventCollector();
185+
getRuntimeWrapper().registerSubscriber(actionEventCollector);
186+
buildTarget("//:foobar");
187+
waitDownloads();
188+
189+
// Assert: out/foo.txt is re-downloaded
190+
assertThat(actionEventCollector.getActionExecutedEvents()).hasSize(1);
191+
assertValidOutputFile("out/foo.txt", "foo\n");
192+
}
193+
194+
@Test
195+
public void downloadOutputsWithRegex_changeRegex_downloadNewMatches() throws Exception {
196+
// Arrange: Do a clean build
197+
write(
198+
"BUILD",
199+
"genrule(",
200+
" name = 'foo',",
201+
" srcs = [],",
202+
" outs = ['out/foo.txt'],",
203+
" cmd = 'echo foo > $@',",
204+
")",
205+
"genrule(",
206+
" name = 'foobar',",
207+
" srcs = [':foo'],",
208+
" outs = ['out/foobar.txt'],",
209+
" cmd = 'cat $(location :foo) > $@ && echo bar >> $@',",
210+
")");
211+
212+
buildTarget("//:foobar");
213+
// Add the new option here because waitDownloads below will internally create a new command
214+
// which will parse the new option.
215+
addOptions("--experimental_remote_download_regex=.*foobar\\.txt$");
216+
waitDownloads();
217+
218+
assertOutputsDoNotExist("//:foo");
219+
assertOutputsDoNotExist("//:foobar");
220+
221+
// Arrange: Change regex
222+
ActionEventCollector actionEventCollector = new ActionEventCollector();
223+
getRuntimeWrapper().registerSubscriber(actionEventCollector);
224+
buildTarget("//:foobar");
225+
waitDownloads();
226+
227+
// Assert: out/foobar.txt is downloaded
228+
assertThat(actionEventCollector.getActionExecutedEvents()).hasSize(1);
229+
assertValidOutputFile("out/foobar.txt", "foo\nbar\n");
136230
}
137231

138232
@Test
@@ -1125,7 +1219,7 @@ protected void assertOnlyOutputContent(String target, String filename, String co
11251219
Artifact output = getOnlyElement(getArtifacts(target));
11261220
assertThat(output.getFilename()).isEqualTo(filename);
11271221
assertThat(output.getPath().exists()).isTrue();
1128-
assertOutputEquals(readContent(output.getPath(), UTF_8), content, /* isLocal= */ false);
1222+
assertOutputEquals(output.getPath(), content, /* isLocal= */ false);
11291223
}
11301224

11311225
protected void assertValidOutputFile(String binRelativePath, String content) throws Exception {
@@ -1135,7 +1229,7 @@ protected void assertValidOutputFile(String binRelativePath, String content) thr
11351229
protected void assertValidOutputFile(String binRelativePath, String content, boolean isLocal)
11361230
throws Exception {
11371231
Path output = getOutputPath(binRelativePath);
1138-
assertOutputEquals(readContent(output, UTF_8), content, isLocal);
1232+
assertOutputEquals(getOutputPath(binRelativePath), content, isLocal);
11391233
assertThat(output.isReadable()).isTrue();
11401234
assertThat(output.isWritable()).isFalse();
11411235
assertThat(output.isExecutable()).isTrue();

0 commit comments

Comments
 (0)