Skip to content

Commit 25ba76c

Browse files
tjgqcopybara-github
tjgq
authored and
copybara-github
committed
Include full tree artifact in inputs when prefetcher doesn't support partial tree artifacts.
The actions generated by SpawnActionTemplate and CppCompileActionTemplate contain individual TreeFileArtifacts, but not the respective TreeArtifact, in their inputs. This causes the input prefetcher to crash when building without the bytes, as it is only able to fetch entire tree artifacts and expects their input metadata to be available. This PR introduces an ActionInputPrefetcher#supportsPartialTreeArtifactInputs method signaling whether prefetching partial tree artifacts is supported; if not, the entire tree artifact is included in the inputs of any such actions. This is a temporary workaround to unblock the 6.0.0 release. The long-term plan is to fix bazelbuild#16333 to enable prefetching partial tree artifacts, and remove this workaround. Fixes bazelbuild#16987. PiperOrigin-RevId: 495050207 Change-Id: I7d29713085d2cf84ce5302394fc18ff2a96ec4be
1 parent 72e0887 commit 25ba76c

File tree

8 files changed

+96
-24
lines changed

8 files changed

+96
-24
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@ public ListenableFuture<Void> prefetchFiles(
2828
// Do nothing.
2929
return immediateVoidFuture();
3030
}
31+
32+
@Override
33+
public boolean supportsPartialTreeArtifactInputs() {
34+
return true;
35+
}
3136
};
3237

3338
/**
@@ -39,4 +44,10 @@ public ListenableFuture<Void> prefetchFiles(
3944
*/
4045
ListenableFuture<Void> prefetchFiles(
4146
Iterable<? extends ActionInput> inputs, MetadataProvider metadataProvider);
47+
48+
/**
49+
* Whether the prefetcher is able to fetch individual files in a tree artifact without fetching
50+
* the entire tree artifact.
51+
*/
52+
boolean supportsPartialTreeArtifactInputs();
4253
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,12 @@ class RemoteActionInputFetcher extends AbstractActionInputPrefetcher {
6161
this.remoteCache = Preconditions.checkNotNull(remoteCache);
6262
}
6363

64+
@Override
65+
public boolean supportsPartialTreeArtifactInputs() {
66+
// This prefetcher is unable to fetch only individual files inside a tree artifact.
67+
return false;
68+
}
69+
6470
@Override
6571
protected void prefetchVirtualActionInput(VirtualActionInput input) throws IOException {
6672
if (!(input instanceof EmptyActionInput)) {

‎src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1253,7 +1253,8 @@ private <S extends ActionInputMapSink, R> R accumulateInputs(
12531253
topLevelFilesets,
12541254
input,
12551255
value,
1256-
env);
1256+
env,
1257+
skyframeActionExecutor.supportsPartialTreeArtifactInputs());
12571258
}
12581259

12591260
if (actionExecutionFunctionExceptionHandler != null) {

‎src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java‎

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact;
2626
import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
2727
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
28+
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
2829
import com.google.devtools.build.lib.actions.FileArtifactValue;
2930
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
3031
import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
@@ -47,7 +48,8 @@ static void addToMap(
4748
Map<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
4849
Artifact key,
4950
SkyValue value,
50-
Environment env)
51+
Environment env,
52+
boolean prefetcherSupportsPartialTreeArtifacts)
5153
throws InterruptedException {
5254
addToMap(
5355
inputMap,
@@ -58,7 +60,8 @@ static void addToMap(
5860
key,
5961
value,
6062
env,
61-
MetadataConsumerForMetrics.NO_OP);
63+
MetadataConsumerForMetrics.NO_OP,
64+
prefetcherSupportsPartialTreeArtifacts);
6265
}
6366

6467
/**
@@ -74,7 +77,8 @@ static void addToMap(
7477
Artifact key,
7578
SkyValue value,
7679
Environment env,
77-
MetadataConsumerForMetrics consumer)
80+
MetadataConsumerForMetrics consumer,
81+
boolean prefetcherSupportsPartialTreeArtifacts)
7882
throws InterruptedException {
7983
if (value instanceof RunfilesArtifactValue) {
8084
// Note: we don't expand the .runfiles/MANIFEST file into the inputs. The reason for that
@@ -120,16 +124,35 @@ static void addToMap(
120124
/*depOwner=*/ key);
121125
consumer.accumulate(treeArtifactValue);
122126
} else if (value instanceof ActionExecutionValue) {
123-
FileArtifactValue metadata = ((ActionExecutionValue) value).getExistingFileArtifactValue(key);
124-
inputMap.put(key, metadata, key);
125-
if (key.isFileset()) {
126-
ImmutableList<FilesetOutputSymlink> filesets = getFilesets(env, (SpecialArtifact) key);
127-
if (filesets != null) {
128-
topLevelFilesets.put(key, filesets);
129-
consumer.accumulate(filesets);
130-
}
127+
if (!prefetcherSupportsPartialTreeArtifacts && key instanceof TreeFileArtifact) {
128+
// If we're unable to prefetch individual files in a tree artifact, include the full tree
129+
// artifact in the action inputs. This makes actions that consume partial tree artifacts
130+
// (such as the ones generated by SpawnActionTemplate or CppCompileActionTemplate) less
131+
// efficient, but is needed until https://github.com/bazelbuild/bazel/issues/16333 is fixed.
132+
SpecialArtifact treeArtifact = key.getParent();
133+
TreeArtifactValue treeArtifactValue =
134+
((ActionExecutionValue) value).getTreeArtifactValue(treeArtifact);
135+
expandTreeArtifactAndPopulateArtifactData(
136+
treeArtifact,
137+
treeArtifactValue,
138+
expandedArtifacts,
139+
archivedTreeArtifacts,
140+
inputMap,
141+
/* depOwner= */ treeArtifact);
142+
consumer.accumulate(treeArtifactValue);
131143
} else {
132-
consumer.accumulate(metadata);
144+
FileArtifactValue metadata =
145+
((ActionExecutionValue) value).getExistingFileArtifactValue(key);
146+
inputMap.put(key, metadata, key);
147+
if (key.isFileset()) {
148+
ImmutableList<FilesetOutputSymlink> filesets = getFilesets(env, (SpecialArtifact) key);
149+
if (filesets != null) {
150+
topLevelFilesets.put(key, filesets);
151+
consumer.accumulate(filesets);
152+
}
153+
} else {
154+
consumer.accumulate(metadata);
155+
}
133156
}
134157
} else {
135158
Preconditions.checkArgument(value instanceof FileArtifactValue, "Unexpected value %s", value);

‎src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
205205
input,
206206
artifactValue,
207207
env,
208-
currentConsumer);
208+
currentConsumer,
209+
skyframeActionExecutor.supportsPartialTreeArtifactInputs());
209210
if (!allArtifactsAreImportant && importantArtifactSet.contains(input)) {
210211
// Calling #addToMap a second time with `input` and `artifactValue` will perform no-op
211212
// updates to the secondary collections passed in (eg. expandedArtifacts,
@@ -220,7 +221,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
220221
input,
221222
artifactValue,
222223
env,
223-
MetadataConsumerForMetrics.NO_OP);
224+
MetadataConsumerForMetrics.NO_OP,
225+
skyframeActionExecutor.supportsPartialTreeArtifactInputs());
224226
}
225227
}
226228
}

‎src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,10 @@ boolean useArchivedTreeArtifacts(ActionAnalysisMetadata action) {
314314
.test(action.getMnemonic());
315315
}
316316

317+
boolean supportsPartialTreeArtifactInputs() {
318+
return actionInputPrefetcher.supportsPartialTreeArtifactInputs();
319+
}
320+
317321
boolean publishTargetSummaries() {
318322
return options.getOptions(BuildEventProtocolOptions.class).publishTargetSummary;
319323
}

‎src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -879,6 +879,7 @@ public void testSharedActionsRacing() throws Exception {
879879
// is running. This way, both actions will check the action cache beforehand and try to update
880880
// the action cache post-build.
881881
final CountDownLatch inputsRequested = new CountDownLatch(2);
882+
skyframeExecutor.configureActionExecutor(/* fileCache= */ null, ActionInputPrefetcher.NONE);
882883
skyframeExecutor
883884
.getEvaluator()
884885
.injectGraphTransformerForTesting(
@@ -1231,6 +1232,7 @@ public void sharedActionTemplate() throws Exception {
12311232
ActionTemplate<DummyAction> template2 =
12321233
new DummyActionTemplate(baseOutput, sharedOutput2, ActionOwner.SYSTEM_ACTION_OWNER);
12331234
ActionLookupValue shared2Ct = createActionLookupValue(template2, shared2);
1235+
skyframeExecutor.configureActionExecutor(/* fileCache= */ null, ActionInputPrefetcher.NONE);
12341236
// Inject the "configured targets" into the graph.
12351237
skyframeExecutor
12361238
.getDifferencerForTesting()
@@ -1645,6 +1647,7 @@ public void analysisEventsNotStoredInExecution() throws Exception {
16451647
createActionLookupValue(action2, lc2),
16461648
null,
16471649
NestedSetBuilder.create(Order.STABLE_ORDER, Event.warn("analysis warning 2")));
1650+
skyframeExecutor.configureActionExecutor(/* fileCache= */ null, ActionInputPrefetcher.NONE);
16481651
skyframeExecutor
16491652
.getDifferencerForTesting()
16501653
.inject(ImmutableMap.of(lc1, ctValue1, lc2, ctValue2));

‎src/test/shell/bazel/remote/build_without_the_bytes_test.sh‎

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -70,14 +70,7 @@ else
7070
declare -r EXE_EXT=""
7171
fi
7272

73-
function test_cc_tree() {
74-
if [[ "$PLATFORM" == "darwin" ]]; then
75-
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
76-
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
77-
# action executors in order to select the appropriate Xcode toolchain.
78-
return 0
79-
fi
80-
73+
function setup_cc_tree() {
8174
mkdir -p a
8275
cat > a/BUILD <<EOF
8376
load(":tree.bzl", "mytree")
@@ -93,11 +86,40 @@ def _tree_impl(ctx):
9386
9487
mytree = rule(implementation = _tree_impl)
9588
EOF
89+
}
90+
91+
function test_cc_tree_remote_executor() {
92+
if [[ "$PLATFORM" == "darwin" ]]; then
93+
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
94+
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
95+
# action executors in order to select the appropriate Xcode toolchain.
96+
return 0
97+
fi
98+
99+
setup_cc_tree
100+
96101
bazel build \
97102
--remote_executor=grpc://localhost:${worker_port} \
98103
--remote_download_minimal \
99104
//a:tree_cc >& "$TEST_log" \
100-
|| fail "Failed to build //a:tree_cc with minimal downloads"
105+
|| fail "Failed to build //a:tree_cc with remote executor and minimal downloads"
106+
}
107+
108+
function test_cc_tree_remote_cache() {
109+
if [[ "$PLATFORM" == "darwin" ]]; then
110+
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
111+
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
112+
# action executors in order to select the appropriate Xcode toolchain.
113+
return 0
114+
fi
115+
116+
setup_cc_tree
117+
118+
bazel build \
119+
--remote_cache=grpc://localhost:${worker_port} \
120+
--remote_download_minimal \
121+
//a:tree_cc >& "$TEST_log" \
122+
|| fail "Failed to build //a:tree_cc with remote cache and minimal downloads"
101123
}
102124

103125
function test_cc_include_scanning_and_minimal_downloads() {

0 commit comments

Comments
 (0)