Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.1.0] Fixes for Bazel's own integration tests fail locally on Linux #20822

Merged
merged 3 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.sandbox;

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 @@ -61,6 +62,8 @@
import java.util.Map;
import java.util.Optional;
import java.util.SortedMap;
import java.util.Set;
import java.util.TreeSet;
import java.util.concurrent.atomic.AtomicBoolean;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -390,7 +393,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 @@ -400,7 +403,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> getBindMounts(
Expand Down
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
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());
}
}
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
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
Loading