Skip to content

Commit

Permalink
[7.0.1] Fixes for Bazel's own integration tests fail locally on Linux (…
Browse files Browse the repository at this point in the history 
…#20821)

Includes 3 commits.

c48392c,
bc1d9d3,
b0db044

Progress towards #20753

---------

Co-authored-by: Googler <[email protected]>
  • Loading branch information
@iancha1992 @lberki
iancha1992 and lberki authored Jan 9, 2024
1 parent 47de7f0 commit bcdfffe
Show file tree
Hide file tree
Showing 8 changed files with 83 additions and 45 deletions.
17 changes: 15 additions & 2 deletions src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.TreeSet;

/** Helper utility to create ActionInput instances. */
public final class ActionInputHelper {
Expand Down Expand Up @@ -122,14 +125,24 @@ public static List<ActionInput> expandArtifacts(
ArtifactExpander artifactExpander,
boolean keepEmptyTreeArtifacts) {
List<ActionInput> result = new ArrayList<>();
Set<Artifact> emptyTreeArtifacts = new TreeSet<>();
Set<Artifact> treeFileArtifactParents = new HashSet<>();
for (ActionInput input : inputs.toList()) {
if (input instanceof Artifact) {
Artifact.addExpandedArtifact(
(Artifact) input, result, artifactExpander, keepEmptyTreeArtifacts);
Artifact inputArtifact = (Artifact) input;
Artifact.addExpandedArtifact(inputArtifact, result, artifactExpander, emptyTreeArtifacts);
if (inputArtifact.isChildOfDeclaredDirectory()) {
treeFileArtifactParents.add(inputArtifact.getParent());
}
} else {
result.add(input);
}
}

if (keepEmptyTreeArtifacts) {
emptyTreeArtifacts.removeAll(treeFileArtifactParents);
result.addAll(emptyTreeArtifacts);
}
return result;
}

Expand Down
12 changes: 6 additions & 6 deletions src/main/java/com/google/devtools/build/lib/actions/Artifact.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import java.util.Comparator;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.UnaryOperator;
import javax.annotation.Nullable;
import net.starlark.java.eval.EvalException;
Expand Down Expand Up @@ -1544,21 +1545,20 @@ public static String joinRootRelativePaths(String delimiter, Iterable<Artifact>
/**
* Adds an artifact to a collection, expanding it once if it's a middleman or tree artifact.
*
* <p>A middleman artifact is never added to the collection. If {@code keepEmptyTreeArtifacts} is
* true, a tree artifact will be added to the collection when it expands into zero file artifacts.
* Otherwise, only the file artifacts the tree artifact expands into will be added.
* <p>The middleman or tree artifact is never added to the output collection. If a tree artifact
* expands into zero file artifacts, it is added to emptyTreeArtifacts.
*/
static void addExpandedArtifact(
Artifact artifact,
Collection<? super Artifact> output,
ArtifactExpander artifactExpander,
boolean keepEmptyTreeArtifacts) {
Set<Artifact> emptyTreeArtifacts) {
if (artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()) {
List<Artifact> expandedArtifacts = new ArrayList<>();
artifactExpander.expand(artifact, expandedArtifacts);
output.addAll(expandedArtifacts);
if (keepEmptyTreeArtifacts && artifact.isTreeArtifact() && expandedArtifacts.isEmpty()) {
output.add(artifact);
if (artifact.isTreeArtifact() && expandedArtifacts.isEmpty()) {
emptyTreeArtifacts.add(artifact);
}
} else {
output.add(artifact);
Expand Down
8 changes: 7 additions & 1 deletion src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,12 @@ private TestAttemptResult runTestAttempt(
.getOutputMetadataStore()
.getTreeArtifactChildren(
(SpecialArtifact) testAction.getCoverageDirectoryTreeArtifact());
ImmutableSet<ActionInput> coverageSpawnMetadata =
ImmutableSet.<ActionInput>builder()
.addAll(expandedCoverageDir)
.add(testAction.getCoverageDirectoryTreeArtifact())
.build();

Spawn coveragePostProcessingSpawn =
createCoveragePostProcessingSpawn(
actionExecutionContext,
Expand All @@ -759,7 +765,7 @@ private TestAttemptResult runTestAttempt(
ActionExecutionContext coverageActionExecutionContext =
actionExecutionContext
.withFileOutErr(coverageOutErr)
.withOutputsAsInputs(expandedCoverageDir);
.withOutputsAsInputs(coverageSpawnMetadata);

writeOutFile(coverageOutErr.getErrorPath(), coverageOutErr.getOutputPath());
appendCoverageLog(coverageOutErr, fileOutErr);
Expand Down
23 changes: 21 additions & 2 deletions src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.devtools.build.lib.sandbox;

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.NetworkNamespace.NETNS_WITH_LOOPBACK;
import static com.google.devtools.build.lib.sandbox.LinuxSandboxCommandLineBuilder.NetworkNamespace.NO_NETNS;

Expand Down Expand Up @@ -60,8 +61,10 @@
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.SortedMap;
import java.util.TreeMap;
import java.util.TreeSet;
import java.util.concurrent.atomic.AtomicBoolean;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -392,7 +395,7 @@ public String getName() {
protected ImmutableSet<Path> getWritableDirs(
Path sandboxExecRoot, Path withinSandboxExecRoot, Map<String, String> env)
throws IOException {
ImmutableSet.Builder<Path> writableDirs = ImmutableSet.builder();
Set<Path> writableDirs = new TreeSet<>();
writableDirs.addAll(super.getWritableDirs(sandboxExecRoot, withinSandboxExecRoot, env));
if (getSandboxOptions().memoryLimitMb > 0) {
CgroupsInfo cgroupsInfo = CgroupsInfo.getInstance();
Expand All @@ -402,7 +405,23 @@ protected ImmutableSet<Path> getWritableDirs(
writableDirs.add(fs.getPath("/dev/shm").resolveSymbolicLinks());
writableDirs.add(fs.getPath("/tmp"));

return writableDirs.build();
if (sandboxExecRoot.equals(withinSandboxExecRoot)) {
return ImmutableSet.copyOf(writableDirs);
}

// If a writable directory is under the sandbox exec root, transform it so that its path will
// be the one that it will be available at after processing the bind mounts (this is how the
// sandbox interprets the corresponding arguments)
//
// Notably, this is usually the case for $TEST_TMPDIR because its default value is under the
// execroot.
return writableDirs.stream()
.map(
d ->
d.startsWith(sandboxExecRoot)
? withinSandboxExecRoot.getRelative(d.relativeTo(sandboxExecRoot))
: d)
.collect(toImmutableSet());
}

private ImmutableList<BindMount> prepareAndGetBindMounts(
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1246,8 +1246,7 @@ private <S extends ActionInputMapSink, R> R accumulateInputs(
topLevelFilesets,
input,
value,
env,
skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput());
env);
}
}

Expand Down
55 changes: 31 additions & 24 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.Map;
Expand All @@ -48,8 +49,7 @@ static void addToMap(
Map<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
Artifact key,
SkyValue value,
Environment env,
boolean requiresTreeMetadataWhenTreeFileIsInput)
Environment env)
throws InterruptedException {
addToMap(
inputMap,
Expand All @@ -60,8 +60,7 @@ static void addToMap(
key,
value,
env,
MetadataConsumerForMetrics.NO_OP,
requiresTreeMetadataWhenTreeFileIsInput);
MetadataConsumerForMetrics.NO_OP);
}

/**
Expand All @@ -77,8 +76,7 @@ static void addToMap(
Artifact key,
SkyValue value,
Environment env,
MetadataConsumerForMetrics consumer,
boolean requiresTreeMetadataWhenTreeFileIsInput)
MetadataConsumerForMetrics consumer)
throws InterruptedException {
if (value instanceof RunfilesArtifactValue) {
// Note: we don't expand the .runfiles/MANIFEST file into the inputs. The reason for that
Expand Down Expand Up @@ -125,15 +123,17 @@ static void addToMap(
/*depOwner=*/ key);
consumer.accumulate(treeArtifactValue);
} else if (value instanceof ActionExecutionValue) {
if (requiresTreeMetadataWhenTreeFileIsInput && key.isChildOfDeclaredDirectory()) {
// Actions resulting from the expansion of an ActionTemplate consume only one of the files
// in a tree artifact. However, the input prefetcher requires access to the tree metadata
// to determine the prefetch location of a tree artifact materialized as a symlink
// (cf. TreeArtifactValue#getMaterializationExecPath()).
// Actions resulting from the expansion of an ActionTemplate consume only one of the files
// in a tree artifact. However, the input prefetcher and the Linux sandbox require access to
// the tree metadata to determine the prefetch location of a tree artifact materialized as a
// symlink (cf. TreeArtifactValue#getMaterializationExecPath()).
if (key.isChildOfDeclaredDirectory()) {
SpecialArtifact treeArtifact = key.getParent();
TreeArtifactValue treeArtifactValue =
((ActionExecutionValue) value).getTreeArtifactValue(treeArtifact);
inputMap.putTreeArtifact(treeArtifact, treeArtifactValue, /* depOwner= */ treeArtifact);
addArchivedTreeArtifactMaybe(
treeArtifact, treeArtifactValue, archivedTreeArtifacts, inputMap, key);
consumer.accumulate(treeArtifactValue);
}
FileArtifactValue metadata = ((ActionExecutionValue) value).getExistingFileArtifactValue(key);
Expand Down Expand Up @@ -221,20 +221,27 @@ private static void expandTreeArtifactAndPopulateArtifactData(
return;
}

inputMap.putTreeArtifact((SpecialArtifact) treeArtifact, value, depOwner);
expandedArtifacts.put(treeArtifact, value.getChildren());
inputMap.putTreeArtifact((SpecialArtifact) treeArtifact, value, depOwner);
addArchivedTreeArtifactMaybe(
(SpecialArtifact) treeArtifact, value, archivedTreeArtifacts, inputMap, depOwner);
}

private static void addArchivedTreeArtifactMaybe(
SpecialArtifact treeArtifact,
TreeArtifactValue value,
Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts,
ActionInputMapSink inputMap,
Artifact depOwner) {
if (!value.getArchivedRepresentation().isPresent()) {
return;
}

value
.getArchivedRepresentation()
.ifPresent(
archivedRepresentation -> {
inputMap.put(
archivedRepresentation.archivedTreeFileArtifact(),
archivedRepresentation.archivedFileValue(),
depOwner);
archivedTreeArtifacts.put(
(SpecialArtifact) treeArtifact,
archivedRepresentation.archivedTreeFileArtifact());
});
ArchivedRepresentation archivedRepresentation = value.getArchivedRepresentation().get();
inputMap.put(
archivedRepresentation.archivedTreeFileArtifact(),
archivedRepresentation.archivedFileValue(),
depOwner);
archivedTreeArtifacts.put(treeArtifact, archivedRepresentation.archivedTreeFileArtifact());
}
}
6 changes: 2 additions & 4 deletions src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
input,
artifactValue,
env,
currentConsumer,
skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput());
currentConsumer);
if (!allArtifactsAreImportant && importantArtifactSet.contains(input)) {
// Calling #addToMap a second time with `input` and `artifactValue` will perform no-op
// updates to the secondary collections passed in (eg. expandedArtifacts,
Expand All @@ -220,8 +219,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
topLevelFilesets,
input,
artifactValue,
env,
skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput());
env);
}
}
}
Expand Down
4 changes: 0 additions & 4 deletions src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,10 +350,6 @@ boolean useArchivedTreeArtifacts(ActionAnalysisMetadata action) {
.test(action.getMnemonic());
}

boolean requiresTreeMetadataWhenTreeFileIsInput() {
return actionInputPrefetcher.requiresTreeMetadataWhenTreeFileIsInput();
}

boolean publishTargetSummaries() {
return options.getOptions(BuildEventProtocolOptions.class).publishTargetSummary;
}
Expand Down

0 comments on commit bcdfffe

Please sign in to comment.