From bcdfffe2342ab67deceb2c82a30ad5b70392bada Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Tue, 9 Jan 2024 15:47:35 -0800 Subject: [PATCH] [7.0.1] Fixes for Bazel's own integration tests fail locally on Linux (#20821) Includes 3 commits. https://github.com/bazelbuild/bazel/commit/c48392c54a92b7bc5b04883bd6499c7a5677205d, https://github.com/bazelbuild/bazel/commit/bc1d9d38bea907beea8fd82c362c9882ddf48cfd, https://github.com/bazelbuild/bazel/commit/b0db044227d62178a7e578b8e03c452d8c17af33 Progress towards https://github.com/bazelbuild/bazel/issues/20753 --------- Co-authored-by: Googler --- .../build/lib/actions/ActionInputHelper.java | 17 +++++- .../devtools/build/lib/actions/Artifact.java | 12 ++-- .../lib/exec/StandaloneTestStrategy.java | 8 ++- .../sandbox/LinuxSandboxedSpawnRunner.java | 23 +++++++- .../lib/skyframe/ActionExecutionFunction.java | 3 +- .../lib/skyframe/ActionInputMapHelper.java | 55 +++++++++++-------- .../lib/skyframe/CompletionFunction.java | 6 +- .../lib/skyframe/SkyframeActionExecutor.java | 4 -- 8 files changed, 83 insertions(+), 45 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java index 9c57f73edf3ade..5bcb26aad5d2f3 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java @@ -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 { @@ -122,14 +125,24 @@ public static List expandArtifacts( ArtifactExpander artifactExpander, boolean keepEmptyTreeArtifacts) { List result = new ArrayList<>(); + Set emptyTreeArtifacts = new TreeSet<>(); + Set 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; } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index 1775144cfd400a..1a0b1ba55483cb 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java @@ -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; @@ -1544,21 +1545,20 @@ public static String joinRootRelativePaths(String delimiter, Iterable /** * Adds an artifact to a collection, expanding it once if it's a middleman or tree artifact. * - *

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. + *

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 output, ArtifactExpander artifactExpander, - boolean keepEmptyTreeArtifacts) { + Set emptyTreeArtifacts) { if (artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()) { List 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); diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index a7000ebbc233b7..3e8ca341c80a2d 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -740,6 +740,12 @@ private TestAttemptResult runTestAttempt( .getOutputMetadataStore() .getTreeArtifactChildren( (SpecialArtifact) testAction.getCoverageDirectoryTreeArtifact()); + ImmutableSet coverageSpawnMetadata = + ImmutableSet.builder() + .addAll(expandedCoverageDir) + .add(testAction.getCoverageDirectoryTreeArtifact()) + .build(); + Spawn coveragePostProcessingSpawn = createCoveragePostProcessingSpawn( actionExecutionContext, @@ -759,7 +765,7 @@ private TestAttemptResult runTestAttempt( ActionExecutionContext coverageActionExecutionContext = actionExecutionContext .withFileOutErr(coverageOutErr) - .withOutputsAsInputs(expandedCoverageDir); + .withOutputsAsInputs(coverageSpawnMetadata); writeOutFile(coverageOutErr.getErrorPath(), coverageOutErr.getOutputPath()); appendCoverageLog(coverageOutErr, fileOutErr); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java index 8c333be0553f73..d8a77b26c6245e 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java @@ -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; @@ -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; @@ -392,7 +395,7 @@ public String getName() { protected ImmutableSet getWritableDirs( Path sandboxExecRoot, Path withinSandboxExecRoot, Map env) throws IOException { - ImmutableSet.Builder writableDirs = ImmutableSet.builder(); + Set writableDirs = new TreeSet<>(); writableDirs.addAll(super.getWritableDirs(sandboxExecRoot, withinSandboxExecRoot, env)); if (getSandboxOptions().memoryLimitMb > 0) { CgroupsInfo cgroupsInfo = CgroupsInfo.getInstance(); @@ -402,7 +405,23 @@ protected ImmutableSet 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 prepareAndGetBindMounts( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 79f864f541aba4..eef14e85af0ff8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -1246,8 +1246,7 @@ private R accumulateInputs( topLevelFilesets, input, value, - env, - skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput()); + env); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java index 8971039c3a0a69..515056c489094b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java @@ -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; @@ -48,8 +49,7 @@ static void addToMap( Map> topLevelFilesets, Artifact key, SkyValue value, - Environment env, - boolean requiresTreeMetadataWhenTreeFileIsInput) + Environment env) throws InterruptedException { addToMap( inputMap, @@ -60,8 +60,7 @@ static void addToMap( key, value, env, - MetadataConsumerForMetrics.NO_OP, - requiresTreeMetadataWhenTreeFileIsInput); + MetadataConsumerForMetrics.NO_OP); } /** @@ -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 @@ -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); @@ -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 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()); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java index 9bd62c7d886f97..127fc19a374a99 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java @@ -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, @@ -220,8 +219,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) topLevelFilesets, input, artifactValue, - env, - skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput()); + env); } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 81a2ef092f6ecf..6809ca303aa19c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -350,10 +350,6 @@ boolean useArchivedTreeArtifacts(ActionAnalysisMetadata action) { .test(action.getMnemonic()); } - boolean requiresTreeMetadataWhenTreeFileIsInput() { - return actionInputPrefetcher.requiresTreeMetadataWhenTreeFileIsInput(); - } - boolean publishTargetSummaries() { return options.getOptions(BuildEventProtocolOptions.class).publishTargetSummary; }